2011-11-28 21:31:15

by Luis R. Rodriguez

[permalink] [raw]
Subject: [PATCH v2 0/2] cfg80211: reg fixes

Here are a series of reg fixes, I'm sending these separate from the
other patches given that we need these fixes merged soon. I'm sending
this on top of what I see right now on wireless-testing which is
that Johannes' patch is not yet reverted so ammending his fix.

I address the issue pointed out by Timo by passing a parameter
for reset_regdomains() and only do a full reset on
restore_regulatory_settings() and on __exit().

Luis R. Rodriguez (2):
cfg80211: fix race on init and driver registration
cfg80211: amend regulatory NULL dereference fix

net/wireless/reg.c | 53 ++++++++++++++++++++++++++++++++++-----------------
1 files changed, 35 insertions(+), 18 deletions(-)

--
1.7.4.15.g7811d



2011-11-28 21:44:57

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] cfg80211: fix race on init and driver registration

On Mon, Nov 28, 2011 at 4:31 PM, Luis R. Rodriguez
<[email protected]> wrote:

> @@ -1823,6 +1837,10 @@ static void restore_regulatory_settings(bool reset_user)
>        /* First restore to the basic regulatory settings */
>        cfg80211_regdomain = cfg80211_world_regdom;
>
> +       if (last_request != &core_request_world)
> +               kfree(last_request);
> +       last_request = &core_request_world;
> +
>        mutex_unlock(&reg_mutex);
>        mutex_unlock(&cfg80211_mutex);
>

This hunk is redundant given that we call reset_regdomains() above,
will send a v3..

Luis

2011-11-28 21:31:18

by Luis R. Rodriguez

[permalink] [raw]
Subject: [PATCH v2 1/2] cfg80211: fix race on init and driver registration

There is a theoretical race that if hit will trigger
a crash. The race is between when we issue the first
regulatory hint, regulatory_hint_core(), gets processed
by the workqueue and between when the first device
gets registered to the wireless core. This is not easy
to reproduce but it was easy to do so through the
regulatory simulator I have been working on. This
is a port of the fix I implemented there [1].

[1] https://github.com/mcgrof/regsim/commit/a246ccf81f059cb662eee288aa13100f631e4cc8

Cc: [email protected]
Cc: Johannes Berg <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
net/wireless/reg.c | 47 +++++++++++++++++++++++++++++++----------------
1 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 76b35df..09b753b 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -57,8 +57,17 @@
#define REG_DBG_PRINT(args...)
#endif

+static struct regulatory_request core_request_world = {
+ .initiator = NL80211_REGDOM_SET_BY_CORE,
+ .alpha2[0] = '0',
+ .alpha2[1] = '0',
+ .intersect = false,
+ .processed = true,
+ .country_ie_env = ENVIRON_ANY,
+};
+
/* Receipt of information from last regulatory request */
-static struct regulatory_request *last_request;
+static struct regulatory_request *last_request = &core_request_world;

/* To trigger userspace events */
static struct platform_device *reg_pdev;
@@ -150,7 +159,7 @@ static char user_alpha2[2];
module_param(ieee80211_regdom, charp, 0444);
MODULE_PARM_DESC(ieee80211_regdom, "IEEE 802.11 regulatory domain code");

-static void reset_regdomains(void)
+static void reset_regdomains(bool full_reset)
{
/* avoid freeing static information or freeing something twice */
if (cfg80211_regdomain == cfg80211_world_regdom)
@@ -165,6 +174,13 @@ static void reset_regdomains(void)

cfg80211_world_regdom = &world_regdom;
cfg80211_regdomain = NULL;
+
+ if (!full_reset)
+ return;
+
+ if (last_request != &core_request_world)
+ kfree(last_request);
+ last_request = &core_request_world;
}

/*
@@ -175,7 +191,7 @@ static void update_world_regdomain(const struct ieee80211_regdomain *rd)
{
BUG_ON(!last_request);

- reset_regdomains();
+ reset_regdomains(false);

cfg80211_world_regdom = rd;
cfg80211_regdomain = rd;
@@ -1409,7 +1425,8 @@ static int __regulatory_hint(struct wiphy *wiphy,
}

new_request:
- kfree(last_request);
+ if (last_request != &core_request_world)
+ kfree(last_request);

last_request = pending_request;
last_request->intersect = intersect;
@@ -1579,9 +1596,6 @@ static int regulatory_hint_core(const char *alpha2)
{
struct regulatory_request *request;

- kfree(last_request);
- last_request = NULL;
-
request = kzalloc(sizeof(struct regulatory_request),
GFP_KERNEL);
if (!request)
@@ -1779,7 +1793,7 @@ static void restore_regulatory_settings(bool reset_user)
mutex_lock(&cfg80211_mutex);
mutex_lock(&reg_mutex);

- reset_regdomains();
+ reset_regdomains(true);
restore_alpha2(alpha2, reset_user);

/*
@@ -1823,6 +1837,10 @@ static void restore_regulatory_settings(bool reset_user)
/* First restore to the basic regulatory settings */
cfg80211_regdomain = cfg80211_world_regdom;

+ if (last_request != &core_request_world)
+ kfree(last_request);
+ last_request = &core_request_world;
+
mutex_unlock(&reg_mutex);
mutex_unlock(&cfg80211_mutex);

@@ -2085,7 +2103,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
int r;

if (last_request->initiator != NL80211_REGDOM_SET_BY_DRIVER) {
- reset_regdomains();
+ reset_regdomains(false);
cfg80211_regdomain = rd;
return 0;
}
@@ -2106,7 +2124,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
if (r)
return r;

- reset_regdomains();
+ reset_regdomains(false);
cfg80211_regdomain = rd;
return 0;
}
@@ -2131,7 +2149,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)

rd = NULL;

- reset_regdomains();
+ reset_regdomains(false);
cfg80211_regdomain = intersected_rd;

return 0;
@@ -2151,7 +2169,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
kfree(rd);
rd = NULL;

- reset_regdomains();
+ reset_regdomains(false);
cfg80211_regdomain = intersected_rd;

return 0;
@@ -2304,11 +2322,8 @@ void /* __init_or_exit */ regulatory_exit(void)
mutex_lock(&cfg80211_mutex);
mutex_lock(&reg_mutex);

- reset_regdomains();
-
- kfree(last_request);
+ reset_regdomains(true);

- last_request = NULL;
dev_set_uevent_suppress(&reg_pdev->dev, true);

platform_device_unregister(reg_pdev);
--
1.7.4.15.g7811d


2011-11-28 21:31:21

by Luis R. Rodriguez

[permalink] [raw]
Subject: [PATCH v2 2/2] cfg80211: amend regulatory NULL dereference fix

Johannes' patch for "cfg80211: fix regulatory NULL dereference"
broke user regulaotry hints and it did not address the fact that
last_request was left populated even if the previous regulatory
hint was stale due to the wiphy disappearing.

Fix user reguluatory hints by only bailing out if for those
regulatory hints where a request_wiphy is expected. The stale last_request
considerations are addressed through the previous fixes on last_request
where we reset the last_request to a static world regdom request upon
reset_regdomains(). In this case though we further enhance the effect
by simply restoring reguluatory settings completely.

Cc: [email protected]
Cc: Johannes Berg <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
net/wireless/reg.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 09b753b..d9ba091 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2094,8 +2094,10 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
}

request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
- if (!request_wiphy) {
- reg_set_request_processed();
+ if (!request_wiphy &&
+ (last_request->initiator == NL80211_REGDOM_SET_BY_DRIVER ||
+ last_request->initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE)) {
+ schedule_delayed_work(&reg_timeout, 0);
return -ENODEV;
}

--
1.7.4.15.g7811d