[RFC] cfg80211: fix race between sysfs and cfg80211

[RFC] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

device_add() is called before adding the phy to the cfg80211 device
list.

So if a userspace program uses sysfs uevents to detect new phy
devices, and queries nl80211 to get phy info, it can get ENODEV even
though the phy exists in sysfs.

An easy workaround is to hold the cfg80211 mutex until the phy is
present both in sysfs and cfg80211 device list.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
---
 net/wireless/core.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 6ac70c1..8952ec4 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -472,15 +472,19 @@ int wiphy_register(struct wiphy *wiphy)
 	/* check and set up bitrates */
 	ieee80211_set_bitrate_flags(wiphy);
 
+	mutex_lock(&cfg80211_mutex);
+
 	res = device_add(&rdev->wiphy.dev);
-	if (res)
+	if (res) {
+		mutex_unlock(&cfg80211_mutex);
 		return res;
+	}
 
 	res = rfkill_register(rdev->rfkill);
-	if (res)
+	if (res) {
+		mutex_unlock(&cfg80211_mutex);
 		goto out_rm_dev;
-
-	mutex_lock(&cfg80211_mutex);
+	}
 
 	/* set up regulatory info */
 	wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE);
-- 
1.7.1



-- 
Maxime

Re: [RFC] cfg80211: fix race between sysfs and cfg80211

[Johannes Berg]

On Sat, 2010-07-17 at 05:13 +0200, Maxime Bizon wrote:
> device_add() is called before adding the phy to the cfg80211 device
> list.
> 
> So if a userspace program uses sysfs uevents to detect new phy
> devices, and queries nl80211 to get phy info, it can get ENODEV even
> though the phy exists in sysfs.
> 
> An easy workaround is to hold the cfg80211 mutex until the phy is
> present both in sysfs and cfg80211 device list.

Maybe we should hold the mutex around the debugfs stuff as well? Then
tools could even access that race-free, and we can simplify the code by
having an "out_unlock" label.

johannes

Re: [RFC] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

On Sat, 2010-07-17 at 21:29 +0200, Johannes Berg wrote:

> Maybe we should hold the mutex around the debugfs stuff as well? Then
> tools could even access that race-free, and we can simplify the code by
> having an "out_unlock" label.

device_add() is called before adding the phy to the cfg80211 device
list.

So if a userspace program uses sysfs uevents to detect new phy
devices, and queries nl80211 to get phy info, it can get ENODEV even
though the phy exists in sysfs.

An easy workaround is to hold the cfg80211 mutex until the phy is
present both in sysfs and cfg80211 device list.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
---
 net/wireless/core.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 6ac70c1..fd164db 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -472,24 +472,22 @@ int wiphy_register(struct wiphy *wiphy)
 	/* check and set up bitrates */
 	ieee80211_set_bitrate_flags(wiphy);
 
+	mutex_lock(&cfg80211_mutex);
+
 	res = device_add(&rdev->wiphy.dev);
 	if (res)
-		return res;
+		goto out_unlock;
 
 	res = rfkill_register(rdev->rfkill);
 	if (res)
 		goto out_rm_dev;
 
-	mutex_lock(&cfg80211_mutex);
-
 	/* set up regulatory info */
 	wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE);
 
 	list_add_rcu(&rdev->list, &cfg80211_rdev_list);
 	cfg80211_rdev_list_generation++;
 
-	mutex_unlock(&cfg80211_mutex);
-
 	/* add to debugfs */
 	rdev->wiphy.debugfsdir =
 		debugfs_create_dir(wiphy_name(&rdev->wiphy),
@@ -509,11 +507,15 @@ int wiphy_register(struct wiphy *wiphy)
 	}
 
 	cfg80211_debugfs_rdev_add(rdev);
+	mutex_unlock(&cfg80211_mutex);
 
 	return 0;
 
- out_rm_dev:
+out_rm_dev:
 	device_del(&rdev->wiphy.dev);
+
+out_unlock:
+	mutex_unlock(&cfg80211_mutex);
 	return res;
 }
 EXPORT_SYMBOL(wiphy_register);
-- 
1.7.1




-- 
Maxime

Re: [RFC] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

On Sat, 2010-07-17 at 21:29 +0200, Johannes Berg wrote:

Hi Johannes,

> Maybe we should hold the mutex around the debugfs stuff as well? Then
> tools could even access that race-free, and we can simplify the code by
> having an "out_unlock" label.

Any comment on the last patch I sent ?

Thanks !

-- 
Maxime

[PATCH] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

From: Maxime Bizon <mbizon@freebox.fr>

device_add() is called before adding the phy to the cfg80211 device
list.

So if a userspace program uses sysfs uevents to detect new phy
devices, and queries nl80211 to get phy info, it can get ENODEV even
though the phy exists in sysfs.

An easy workaround is to hold the cfg80211 mutex until the phy is
present in sysfs/cfg80211/debugfs.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
---
 net/wireless/core.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 6ac70c1..fd164db 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -472,24 +472,22 @@ int wiphy_register(struct wiphy *wiphy)
 	/* check and set up bitrates */
 	ieee80211_set_bitrate_flags(wiphy);
 
+	mutex_lock(&cfg80211_mutex);
+
 	res = device_add(&rdev->wiphy.dev);
 	if (res)
-		return res;
+		goto out_unlock;
 
 	res = rfkill_register(rdev->rfkill);
 	if (res)
 		goto out_rm_dev;
 
-	mutex_lock(&cfg80211_mutex);
-
 	/* set up regulatory info */
 	wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE);
 
 	list_add_rcu(&rdev->list, &cfg80211_rdev_list);
 	cfg80211_rdev_list_generation++;
 
-	mutex_unlock(&cfg80211_mutex);
-
 	/* add to debugfs */
 	rdev->wiphy.debugfsdir =
 		debugfs_create_dir(wiphy_name(&rdev->wiphy),
@@ -509,11 +507,15 @@ int wiphy_register(struct wiphy *wiphy)
 	}
 
 	cfg80211_debugfs_rdev_add(rdev);
+	mutex_unlock(&cfg80211_mutex);
 
 	return 0;
 
- out_rm_dev:
+out_rm_dev:
 	device_del(&rdev->wiphy.dev);
+
+out_unlock:
+	mutex_unlock(&cfg80211_mutex);
 	return res;
 }
 EXPORT_SYMBOL(wiphy_register);
-- 
1.7.1



-- 
Maxime

Re: [PATCH] cfg80211: fix race between sysfs and cfg80211

[Luis R. Rodriguez]

On Wed, Jul 21, 2010 at 8:21 AM, Maxime Bizon <mbizon@freebox.fr> wrote:
> From: Maxime Bizon <mbizon@freebox.fr>
>
> device_add() is called before adding the phy to the cfg80211 device
> list.
>
> So if a userspace program uses sysfs uevents to detect new phy
> devices, and queries nl80211 to get phy info, it can get ENODEV even
> though the phy exists in sysfs.
>
> An easy workaround is to hold the cfg80211 mutex until the phy is
> present in sysfs/cfg80211/debugfs.
>
> Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
> ---

You should resubmit as a new e-mail with a subject of [PATCH v2].

  Luis

[PATCH v2] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

From: Maxime Bizon <mbizon@freebox.fr>

device_add() is called before adding the phy to the cfg80211 device
list.

So if a userspace program uses sysfs uevents to detect new phy
devices, and queries nl80211 to get phy info, it can get ENODEV even
though the phy exists in sysfs.

An easy workaround is to hold the cfg80211 mutex until the phy is
present in sysfs/cfg80211/debugfs.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
---
 net/wireless/core.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 6ac70c1..fd164db 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -472,24 +472,22 @@ int wiphy_register(struct wiphy *wiphy)
 	/* check and set up bitrates */
 	ieee80211_set_bitrate_flags(wiphy);
 
+	mutex_lock(&cfg80211_mutex);
+
 	res = device_add(&rdev->wiphy.dev);
 	if (res)
-		return res;
+		goto out_unlock;
 
 	res = rfkill_register(rdev->rfkill);
 	if (res)
 		goto out_rm_dev;
 
-	mutex_lock(&cfg80211_mutex);
-
 	/* set up regulatory info */
 	wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE);
 
 	list_add_rcu(&rdev->list, &cfg80211_rdev_list);
 	cfg80211_rdev_list_generation++;
 
-	mutex_unlock(&cfg80211_mutex);
-
 	/* add to debugfs */
 	rdev->wiphy.debugfsdir =
 		debugfs_create_dir(wiphy_name(&rdev->wiphy),
@@ -509,11 +507,15 @@ int wiphy_register(struct wiphy *wiphy)
 	}
 
 	cfg80211_debugfs_rdev_add(rdev);
+	mutex_unlock(&cfg80211_mutex);
 
 	return 0;
 
- out_rm_dev:
+out_rm_dev:
 	device_del(&rdev->wiphy.dev);
+
+out_unlock:
+	mutex_unlock(&cfg80211_mutex);
 	return res;
 }
 EXPORT_SYMBOL(wiphy_register);
-- 
1.7.1



-- 
Maxime

Re: [PATCH v2] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

On Fri, 2010-07-23 at 04:29 +0200, Maxime Bizon wrote:

Hi,

> From: Maxime Bizon <mbizon@freebox.fr>

Any comment/ACK/NACK on this ?

Thanks,

-- 
Maxime

Re: [PATCH v2] cfg80211: fix race between sysfs and cfg80211

[Johannes Berg]

On Thu, 2010-08-12 at 20:41 +0200, Maxime Bizon wrote:
> On Fri, 2010-07-23 at 04:29 +0200, Maxime Bizon wrote:
> 
> Hi,
> 
> > From: Maxime Bizon <mbizon@freebox.fr>
> 
> Any comment/ACK/NACK on this ?

Err, it was merged a long time ago

johannes

Re: [PATCH v2] cfg80211: fix race between sysfs and cfg80211

[Maxime Bizon]

On Thu, 2010-08-12 at 20:42 +0200, Johannes Berg wrote:

> Err, it was merged a long time ago

I'm hitting another race now, it appeared after I disabled
CONFIG_CRYPTO_MANAGER_TESTS (new option in 2.6.35.2).

I have a userspace daemon that listen for new phy event, and create and
configure an AP interface just after it gets the event.


The driver is still inside ieee80211_register_hw(), but the userspace is
able to create the AP interface before the call is over. Once
wiphy_register() is done, no lock seems to prevent this from happening.

ieee80211_init_rate_ctrl_alg() then fails with EBUSY, and
ieee80211_register_hw() starts unregistering the phy.

Is that expected ?

I was able to get past it by taking rtnl lock before wiphy_register()

-- 
Maxime

Re: [PATCH v2] cfg80211: fix race between sysfs and cfg80211

[Johannes Berg]

On Sat, 2010-08-14 at 08:31 +0200, Maxime Bizon wrote:

> The driver is still inside ieee80211_register_hw(), but the userspace is
> able to create the AP interface before the call is over. Once
> wiphy_register() is done, no lock seems to prevent this from happening.
> 
> ieee80211_init_rate_ctrl_alg() then fails with EBUSY, and
> ieee80211_register_hw() starts unregistering the phy.
> 
> Is that expected ?

Oops. No, not really, but I think the problem is that we do
wiphy_register far too early. I think we should see if we can reorder
the code in register_hw(). It won't be perfect wrt. debugfs, but I don't
think that is a problem.

johannes