[PATCH v2] mac80211: add missing locking in ieee80211_reconfig

When suspending an associated system, and then resuming,
the station vif is being reconfigured without taking the
sdata->u.mgd.mtx lock, which results in the following warning:

WARNING: at net/mac80211/mlme.c:101 ieee80211_ap_probereq_get+0x58/0xb8 [mac80211]()
Modules linked in: wl12xx_sdio wl12xx firmware_class crc7 mac80211 cfg80211 [last unloaded: crc7]
Backtrace:
[<c005432c>] (dump_backtrace+0x0/0x118) from [<c0376e28>] (dump_stack+0x20/0x24)
 r7:00000000 r6:bf12d6ec r5:bf154aac r4:00000065
[<c0376e08>] (dump_stack+0x0/0x24) from [<c0079104>] (warn_slowpath_common+0x5c/0x74)
[<c00790a8>] (warn_slowpath_common+0x0/0x74) from [<c0079148>] (warn_slowpath_null+0x2c/0x34)
 r9:000024ff r8:cd006460 r7:00000001 r6:00000000 r5:00000000
r4:cf1394a0
[<c007911c>] (warn_slowpath_null+0x0/0x34) from [<bf12d6ec>] (ieee80211_ap_probereq_get+0x58/0xb8 [mac80211])
[<bf12d694>] (ieee80211_ap_probereq_get+0x0/0xb8 [mac80211]) from [<bf19cd04>] (wl1271_cmd_build_ap_probe_req+0x30/0xf8 [wl12xx])
 r4:cd007440
[<bf19ccd4>] (wl1271_cmd_build_ap_probe_req+0x0/0xf8 [wl12xx]) from [<bf1995f4>] (wl1271_op_bss_info_changed+0x4c4/0x808 [wl12xx])
 r5:cd007440 r4:000003b4
[<bf199130>] (wl1271_op_bss_info_changed+0x0/0x808 [wl12xx]) from [<bf122168>] (ieee80211_bss_info_change_notify+0x1a4/0x1f8 [mac80211])
[<bf121fc4>] (ieee80211_bss_info_change_notify+0x0/0x1f8 [mac80211]) from [<bf141e80>] (ieee80211_reconfig+0x4d0/0x668 [mac80211])
 r8:cf0eeea4 r7:cd00671c r6:00000000 r5:cd006460 r4:cf1394a0
[<bf1419b0>] (ieee80211_reconfig+0x0/0x668 [mac80211]) from [<bf137dd4>] (ieee80211_resume+0x60/0x70 [mac80211])
[<bf137d74>] (ieee80211_resume+0x0/0x70 [mac80211]) from [<bf0eb930>] (wiphy_resume+0x6c/0x7c [cfg80211])
 r5:cd006248 r4:cd006110
[<bf0eb8c4>] (wiphy_resume+0x0/0x7c [cfg80211]) from [<c0241024>] (legacy_resume+0x38/0x70)
 r7:00000000 r6:00000000 r5:cd006248 r4:cd0062fc
[<c0240fec>] (legacy_resume+0x0/0x70) from [<c0241478>] (device_resume+0x168/0x1a0)
 r8:c04ca8d8 r7:cd00627c r6:00000010 r5:cd006248 r4:cd0062fc
[<c0241310>] (device_resume+0x0/0x1a0) from [<c0241600>] (dpm_resume_end+0xf8/0x3bc)
 r7:00000000 r6:00000005 r5:cd006248 r4:cd0062fc
[<c0241508>] (dpm_resume_end+0x0/0x3bc) from [<c00b2a24>] (suspend_devices_and_enter+0x1b0/0x204)
[<c00b2874>] (suspend_devices_and_enter+0x0/0x204) from [<c00b2b68>] (enter_state+0xf0/0x148)
 r7:c037e978 r6:00000003 r5:c043d807 r4:00000000
[<c00b2a78>] (enter_state+0x0/0x148) from [<c00b20a4>] (state_store+0xa4/0xcc)
 r7:c037e978 r6:00000003 r5:00000003 r4:c043d807
[<c00b2000>] (state_store+0x0/0xcc) from [<c01fc90c>] (kobj_attr_store+0x20/0x24)
[<c01fc8ec>] (kobj_attr_store+0x0/0x24) from [<c0157120>] (sysfs_write_file+0x11c/0x150)
[<c0157004>] (sysfs_write_file+0x0/0x150) from [<c0100f84>] (vfs_write+0xc0/0x14c)
[<c0100ec4>] (vfs_write+0x0/0x14c) from [<c01010e4>] (sys_write+0x4c/0x78)
 r8:40126000 r7:00000004 r6:cf1a7c80 r5:00000000 r4:00000000
[<c0101098>] (sys_write+0x0/0x78) from [<c00500c0>] (ret_fast_syscall+0x0/0x30)
 r8:c00502c8 r7:00000004 r6:403525e8 r5:40126000 r4:00000004

Signed-off-by: Eliad Peller <redacted>
---
v1 -> v2: don't truncate warning lines

 net/mac80211/util.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index cf68700..d036597 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c

@@ -1210,7 +1210,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 		switch (sdata->vif.type) {
 		case NL80211_IFTYPE_STATION:
 			changed |= BSS_CHANGED_ASSOC;
+			mutex_lock(&sdata->u.mgd.mtx);
 			ieee80211_bss_info_change_notify(sdata, changed);
+			mutex_unlock(&sdata->u.mgd.mtx);
 			break;
 		case NL80211_IFTYPE_ADHOC:
 			changed |= BSS_CHANGED_IBSS;

-- 
1.7.0.4

On Tue, 2011-02-08 at 18:43 +0200, Eliad Peller wrote:

When suspending an associated system, and then resuming,
the station vif is being reconfigured without taking the
sdata->u.mgd.mtx lock, which results in the following warning:

...

Oi, this is a complicated locking rule.

Can you please stick a

lockdep_assert_held(...)

in the right place to make sure we catch it even on drivers that don't
use the probe_req function?

johannes

On Mon, Feb 14, 2011 at 12:09 PM, Johannes Berg
[off-list ref] wrote:

On Tue, 2011-02-08 at 18:43 +0200, Eliad Peller wrote:

quoted

When suspending an associated system, and then resuming,
the station vif is being reconfigured without taking the
sdata->u.mgd.mtx lock, which results in the following warning:

...

quoted

 net/mac80211/util.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index cf68700..d036597 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c

@@ -1210,7 +1210,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)

              switch (sdata->vif.type) {
              case NL80211_IFTYPE_STATION:
                      changed |= BSS_CHANGED_ASSOC;
+                     mutex_lock(&sdata->u.mgd.mtx);
                      ieee80211_bss_info_change_notify(sdata, changed);
+                     mutex_unlock(&sdata->u.mgd.mtx);
                      break;
              case NL80211_IFTYPE_ADHOC:
                      changed |= BSS_CHANGED_IBSS;

Oi, this is a complicated locking rule.

Can you please stick a

lockdep_assert_held(...)

in the right place to make sure we catch it even on drivers that don't
use the probe_req function?

well, i'm not sure what is the right place / when this mutex should be taken.
when doing something like:

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 1c507c6..ab1843d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c

@@ -191,6 +191,7 @@ void ieee80211_bss_info_change_notify(struct

ieee80211_sub_if_data *sdata,
                return;

        if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+               lockdep_assert_held(&sdata->u.mgd.mtx);
                /*
                 * While not associated, claim a BSSID of all-zeroes
                 * so that drivers don't do any weird things with the

i got several warnings from ieee80211_open, ieee80211_recalc_idle, etc.
should this mutex be taken if the psm has been changed (for example)?
or should we check the mutex only if certain flags has been changed?

Eliad.

On Thu, 2011-02-17 at 02:51 +0200, Eliad Peller wrote:

Yikes!! So you're saying that the locking is conditional on a certain
flag? That makes the driver API a lot more complex, and the locking
rules get really icky. I really don't like that.

I think the answer to that is that we must rewrite the entire API. My
suggestion would be:
 * remove ieee80211_ap_probereq_get
 * add a new driver op "void set_ap_probe_req(..., skb)"
 * call this in the few appropriate places, but not from
   ieee80211_bss_info_change_notify
 * make sure the locking for it is correct

johannes

On Thu, Feb 17, 2011 at 10:24 AM, Johannes Berg
[off-list ref] wrote:

On Thu, 2011-02-17 at 02:51 +0200, Eliad Peller wrote:

quoted

quoted

quoted

                      changed |= BSS_CHANGED_ASSOC;
+                     mutex_lock(&sdata->u.mgd.mtx);
                      ieee80211_bss_info_change_notify(sdata, changed);
+                     mutex_unlock(&sdata->u.mgd.mtx);
                      break;
              case NL80211_IFTYPE_ADHOC:
                      changed |= BSS_CHANGED_IBSS;

Oi, this is a complicated locking rule.

Can you please stick a

lockdep_assert_held(...)

in the right place to make sure we catch it even on drivers that don't
use the probe_req function?

well, i'm not sure what is the right place / when this mutex should be taken.
when doing something like:

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 1c507c6..ab1843d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c

@@ -191,6 +191,7 @@ void ieee80211_bss_info_change_notify(struct

ieee80211_sub_if_data *sdata,
                return;

        if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+               lockdep_assert_held(&sdata->u.mgd.mtx);
                /*
                 * While not associated, claim a BSSID of all-zeroes
                 * so that drivers don't do any weird things with the

i got several warnings from ieee80211_open, ieee80211_recalc_idle, etc.
should this mutex be taken if the psm has been changed (for example)?
or should we check the mutex only if certain flags has been changed?

Yikes!! So you're saying that the locking is conditional on a certain
flag? That makes the driver API a lot more complex, and the locking
rules get really icky. I really don't like that.

me neither :)

I think the answer to that is that we must rewrite the entire API. My
suggestion would be:
 * remove ieee80211_ap_probereq_get
 * add a new driver op "void set_ap_probe_req(..., skb)"
 * call this in the few appropriate places, but not from
  ieee80211_bss_info_change_notify
 * make sure the locking for it is correct

such solution will simplify the locking.
however, it does require some additional work, and i'm pretty loaded right now.
i'll add it to my TODO list.

Eliad.