Thread (62 messages) 62 messages, 2 authors, 2012-12-20

Re: [PATCH 18/24] regulatory: fix reg_is_valid_request handling

From: Luis R. Rodriguez <hidden>
Date: 2012-12-13 21:27:35

On Thu, Dec 6, 2012 at 8:47 AM, Johannes Berg [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Johannes Berg <redacted>

There's a bug with the world regulatory domain, it
can be updated any time which is different from all
other regdomains that can only be updated once after
a request for them. Fix this by adding a check for
"processed" to the reg_is_valid_request() function
and clear that when doing a request.

While looking at this I also found another locking
bug, last_request is protected by the reg_mutex not
the cfg80211_mutex so the code in nl80211 is racy.
Remove that code as it only tries to prevent an
allocation in an error case, which isn't necessary.
Then the function can also become static and locking
in nl80211 can have a smaller scope.

Also change __set_regdom() to do the checks earlier
and not different for world/other regdomains.

Signed-off-by: Johannes Berg <redacted>
---
 net/wireless/nl80211.c |  9 ++-------
 net/wireless/reg.c     | 21 +++++++++++----------
 net/wireless/reg.h     |  1 -
 3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 260b5d8..370cd31 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4333,6 +4326,8 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
                }
        }

+       mutex_lock(&cfg80211_mutex);
+
        r = set_regdom(rd);
        rd = NULL;
Unless you removed it from previous patches in the series the
"bad_reg" goto label has an unlock but above you removed the top
locking call so I think you just forgot to remove the unlock on the
goto label.
quoted hunk ↗ jump to hunk
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index dfab71b..10d8edb 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -426,12 +426,16 @@ static int call_crda(const char *alpha2)
        return kobject_uevent(&reg_pdev->dev.kobj, KOBJ_CHANGE);
 }

-/* Used by nl80211 before kmalloc'ing our regulatory domain */
-bool reg_is_valid_request(const char *alpha2)
+static bool reg_is_valid_request(const char *alpha2)
 {
+       assert_reg_lock();
+
        if (!last_request)
                return false;

+       if (last_request->processed)
+               return false;
+
        return alpha2_equal(last_request->alpha2, alpha2);
 }
@@ -1471,6 +1475,7 @@ new_request:

        last_request = pending_request;
        last_request->intersect = intersect;
+       last_request->processed = false;
The clearing you are doing is on the last_request but it comes from
the pending regulatory request which is always false given that the
request is kzalloc'd, not sure if that was needed or how that helped.

Apart from that looks good.

  Luis
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help