Unfortunately my channel switch announcement patches contained locking issues
which I didn't notice during my test due to bad configuration. Johannes pointed
them out, so here is my attempt to fix them. This series attempts to fix two
issues:
* There are "suspicious rcu_dereference_protected() usage" problems for
ieee80211_assign_beacon and ieee80211_ibss_csa_beacon because the respective
locking was missing
* There is a circular dependency between the wdev/sdata-lock and the
csa_finalize worker lock (see description in patch 3).
The fix is not really trivial and I'm not an expert on the cfg80211/mac80211
locking, so I'd like to request your critical review. Patches are based
on mac80211-next.
Thanks,
Simon
Simon Wunderlich (3):
mac80211: modify beacon using sdata/wdev-lock, not rtnl lock
cfg80211/mac80211/ath6kl: acquire wdev lock outside ch_switch_notify
mac80211: don't cancel csa finalize work within stop_ap
drivers/net/wireless/ath/ath6kl/cfg80211.c | 2 +
include/net/cfg80211.h | 3 +-
net/mac80211/cfg.c | 136 ++++++++++++++++++----------
net/mac80211/ieee80211_i.h | 3 +
net/wireless/nl80211.c | 9 +-
5 files changed, 100 insertions(+), 53 deletions(-)
--
1.7.10.4
The current channel switch code has a potential deadlock:
1) * cfg80211_stop_ap acquires wdev-lock
* ieee80211_stop_ap calls cancel_work_sync for the csa_finalize_work,
which acquires the associated worker-lock
2) * ieee80211_csa_finalize_work holds the worker-lock when run
* it calls cfg80211_ch_switch_notify which will claim the wdev-lock,
and also needs to claim the sdata-lock (which is the same as the
wdev-lock) to modify the beacons.
It is sufficient to just set the channel switch active to false. If the
worker is running later, it will find the channel switch to not be
active anymore and returns immediately without changing anything.
Canceling the worker is done anyway when the interface goes down
(ieee80211_do_stop).
Reported-by: Johannes Berg <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
net/mac80211/cfg.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a715c9b..7315819 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1095,7 +1095,6 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
/* abort any running channel switch */
sdata->vif.csa_active = false;
- cancel_work_sync(&sdata->csa_finalize_work);
cancel_work_sync(&sdata->u.ap.request_smps_work);
/* turn off carrier for this interface and dependent VLANs */
--
1.7.10.4
The csa finalize worker needs to change the beacon information (for
different modes). These are normally protected under rtnl lock, but the
csa finalize worker is called by drivers and should not acquire the RTNL
lock. Therefore change access protection for beacons to sdata/wdev lock.
Reported-by: Johannes Berg <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
net/mac80211/cfg.c | 114 +++++++++++++++++++++++++++++---------------
net/mac80211/ieee80211_i.h | 3 ++
2 files changed, 78 insertions(+), 39 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1f07aa6..433d5cd 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -857,7 +857,7 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
if (!resp || !resp_len)
return 1;
- old = rtnl_dereference(sdata->u.ap.probe_resp);
+ old = sdata_dereference(sdata->u.ap.probe_resp, sdata);
new = kzalloc(sizeof(struct probe_resp) + resp_len, GFP_KERNEL);
if (!new)
@@ -881,7 +881,8 @@ int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
int size, err;
u32 changed = BSS_CHANGED_BEACON;
- old = rtnl_dereference(sdata->u.ap.beacon);
+ old = sdata_dereference(sdata->u.ap.beacon, sdata);
+
/* Need to have a beacon head if we don't have one yet */
if (!params->head && !old)
@@ -958,9 +959,12 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
BSS_CHANGED_P2P_PS;
int err;
- old = rtnl_dereference(sdata->u.ap.beacon);
- if (old)
- return -EALREADY;
+ sdata_lock(sdata);
+ old = sdata_dereference(sdata->u.ap.beacon, sdata);
+ if (old) {
+ err = -EALREADY;
+ goto out;
+ }
/* TODO: make hostapd tell us what it wants */
sdata->smps_mode = IEEE80211_SMPS_OFF;
@@ -970,7 +974,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
err = ieee80211_vif_use_channel(sdata, ¶ms->chandef,
IEEE80211_CHANCTX_SHARED);
if (err)
- return err;
+ goto out;
ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
/*
@@ -1015,16 +1019,17 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
err = ieee80211_assign_beacon(sdata, ¶ms->beacon);
if (err < 0)
- return err;
+ goto out;
changed |= err;
err = drv_start_ap(sdata->local, sdata);
if (err) {
- old = rtnl_dereference(sdata->u.ap.beacon);
+ old = sdata_dereference(sdata->u.ap.beacon, sdata);
+
if (old)
kfree_rcu(old, rcu_head);
RCU_INIT_POINTER(sdata->u.ap.beacon, NULL);
- return err;
+ goto out;
}
ieee80211_bss_info_change_notify(sdata, changed);
@@ -1033,7 +1038,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
netif_carrier_on(vlan->dev);
- return 0;
+ err = 0;
+out:
+ sdata_unlock(sdata);
+ return err;
}
static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
@@ -1045,21 +1053,30 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ sdata_lock(sdata);
/* don't allow changing the beacon while CSA is in place - offset
* of channel switch counter may change
*/
- if (sdata->vif.csa_active)
- return -EBUSY;
+ if (sdata->vif.csa_active) {
+ err = -EBUSY;
+ goto out;
+ }
- old = rtnl_dereference(sdata->u.ap.beacon);
- if (!old)
- return -ENOENT;
+ old = sdata_dereference(sdata->u.ap.beacon, sdata);
+ if (!old) {
+ err = -ENOENT;
+ goto out;
+ }
err = ieee80211_assign_beacon(sdata, params);
if (err < 0)
- return err;
+ goto out;
ieee80211_bss_info_change_notify(sdata, err);
- return 0;
+
+ err = 0;
+out:
+ sdata_unlock(sdata);
+ return err;
}
static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
@@ -1071,10 +1088,10 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
struct probe_resp *old_probe_resp;
struct cfg80211_chan_def chandef;
- old_beacon = rtnl_dereference(sdata->u.ap.beacon);
+ old_beacon = sdata_dereference(sdata->u.ap.beacon, sdata);
if (!old_beacon)
return -ENOENT;
- old_probe_resp = rtnl_dereference(sdata->u.ap.probe_resp);
+ old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
/* abort any running channel switch */
sdata->vif.csa_active = false;
@@ -1974,9 +1991,13 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
enum ieee80211_band band;
u32 changed = 0;
+ int ret;
- if (!rtnl_dereference(sdata->u.ap.beacon))
- return -ENOENT;
+ sdata_lock(sdata);
+ if (!sdata_dereference(sdata->u.ap.beacon, sdata)) {
+ ret = -ENOENT;
+ goto out;
+ }
band = ieee80211_get_sdata_band(sdata);
@@ -2043,8 +2064,11 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
}
ieee80211_bss_info_change_notify(sdata, changed);
+ ret = 0;
+out:
+ sdata_unlock(sdata);
- return 0;
+ return ret;
}
static int ieee80211_set_txq_params(struct wiphy *wiphy,
@@ -3077,8 +3101,11 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
return -EBUSY;
/* don't allow another channel switch if one is already active. */
- if (sdata->vif.csa_active)
- return -EBUSY;
+ sdata_lock(sdata);
+ if (sdata->vif.csa_active) {
+ err = -EBUSY;
+ goto out;
+ }
switch (sdata->vif.type) {
case NL80211_IFTYPE_AP:
@@ -3087,59 +3114,63 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
sdata->csa_counter_offset_presp = params->counter_offset_presp;
sdata->u.ap.next_beacon =
cfg80211_beacon_dup(¶ms->beacon_after);
- if (!sdata->u.ap.next_beacon)
- return -ENOMEM;
+ if (!sdata->u.ap.next_beacon) {
+ err = -ENOMEM;
+ goto out;
+ }
err = ieee80211_assign_beacon(sdata, ¶ms->beacon_csa);
if (err < 0) {
kfree(sdata->u.ap.next_beacon);
- return err;
+ goto out;
}
break;
case NL80211_IFTYPE_ADHOC:
+ err = -EINVAL;
if (!sdata->vif.bss_conf.ibss_joined)
- return -EINVAL;
+ goto out;
if (params->chandef.width != sdata->u.ibss.chandef.width)
- return -EINVAL;
+ goto out;
switch (params->chandef.width) {
case NL80211_CHAN_WIDTH_40:
if (cfg80211_get_chandef_type(¶ms->chandef) !=
cfg80211_get_chandef_type(&sdata->u.ibss.chandef))
- return -EINVAL;
+ goto out;
case NL80211_CHAN_WIDTH_5:
case NL80211_CHAN_WIDTH_10:
case NL80211_CHAN_WIDTH_20_NOHT:
case NL80211_CHAN_WIDTH_20:
break;
default:
- return -EINVAL;
+ goto out;
}
/* changes into another band are not supported */
if (sdata->u.ibss.chandef.chan->band !=
params->chandef.chan->band)
- return -EINVAL;
+ goto out;
err = ieee80211_ibss_csa_beacon(sdata, params);
if (err < 0)
- return err;
+ goto out;
break;
#ifdef CONFIG_MAC80211_MESH
case NL80211_IFTYPE_MESH_POINT:
ifmsh = &sdata->u.mesh;
+ err = -EINVAL;
if (!ifmsh->mesh_id)
- return -EINVAL;
+ goto out;
if (params->chandef.width != sdata->vif.bss_conf.chandef.width)
- return -EINVAL;
+ goto out;
/* changes into another band are not supported */
if (sdata->vif.bss_conf.chandef.chan->band !=
params->chandef.chan->band)
- return -EINVAL;
+ goto out;
ifmsh->chsw_init = true;
if (!ifmsh->pre_value)
@@ -3150,12 +3181,13 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
err = ieee80211_mesh_csa_beacon(sdata, params, true);
if (err < 0) {
ifmsh->chsw_init = false;
- return err;
+ goto out;
}
break;
#endif
default:
- return -EOPNOTSUPP;
+ err = -EOPNOTSUPP;
+ goto out;
}
sdata->csa_radar_required = params->radar_required;
@@ -3171,7 +3203,11 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
ieee80211_bss_info_change_notify(sdata, err);
drv_channel_switch_beacon(sdata, ¶ms->chandef);
- return 0;
+ err = 0;
+out:
+ sdata_unlock(sdata);
+
+ return err;
}
static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 61ccf0d..01e4d9b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -812,6 +812,9 @@ static inline void sdata_unlock(struct ieee80211_sub_if_data *sdata)
__release(&sdata->wdev.mtx);
}
+#define sdata_dereference(p, sdata) \
+ rcu_dereference_protected(p, &sdata->wdev.mtx)
+
static inline void
sdata_assert_lock(struct ieee80211_sub_if_data *sdata)
{
--
1.7.10.4
The channel switch notification should be sent under the
wdev/sdata-lock, preferably in the same moment as the channel change
happens, to avoid races by other callers (e.g. start/stop_ap).
This also adds the previously missing sdata_lock protection in
csa_finalize_work.
Reported-by: Johannes Berg <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
drivers/net/wireless/ath/ath6kl/cfg80211.c | 2 ++
include/net/cfg80211.h | 3 ++-
net/mac80211/cfg.c | 21 +++++++++++++++------
net/wireless/nl80211.c | 9 +++------
4 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index 36dc61d..43a05c9 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -1109,7 +1109,9 @@ void ath6kl_cfg80211_ch_switch_notify(struct ath6kl_vif *vif, int freq,
(mode == WMI_11G_HT20) ?
NL80211_CHAN_HT20 : NL80211_CHAN_NO_HT);
+ mutex_lock(vif->wdev->mtx);
cfg80211_ch_switch_notify(vif->ndev, &chandef);
+ mutex_unlock(vif->wdev->mtx);
}
static int ath6kl_cfg80211_add_key(struct wiphy *wiphy, struct net_device *ndev,
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 633146b..874b5ae 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4286,7 +4286,8 @@ bool cfg80211_reg_can_beacon(struct wiphy *wiphy,
* @dev: the device which switched channels
* @chandef: the new channel definition
*
- * Acquires wdev_lock, so must only be called from sleepable driver context!
+ * Caller must acquire wdev_lock, therefore must only be called from sleepable
+ * driver context!
*/
void cfg80211_ch_switch_notify(struct net_device *dev,
struct cfg80211_chan_def *chandef);
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 433d5cd..a715c9b 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3010,13 +3010,18 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
struct ieee80211_local *local = sdata->local;
int err, changed = 0;
+ sdata_lock(sdata);
+ /* AP might have been stopped while waiting for the lock. */
+ if (!sdata->vif.csa_active)
+ goto unlock;
+
if (!ieee80211_sdata_running(sdata))
- return;
+ goto unlock;
sdata->radar_required = sdata->csa_radar_required;
err = ieee80211_vif_change_channel(sdata, &changed);
if (WARN_ON(err < 0))
- return;
+ goto unlock;
if (!local->use_chanctx) {
local->_oper_chandef = sdata->csa_chandef;
@@ -3025,11 +3030,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
ieee80211_bss_info_change_notify(sdata, changed);
+ sdata->vif.csa_active = false;
switch (sdata->vif.type) {
case NL80211_IFTYPE_AP:
err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
if (err < 0)
- return;
+ goto unlock;
+
changed |= err;
kfree(sdata->u.ap.next_beacon);
sdata->u.ap.next_beacon = NULL;
@@ -3043,20 +3050,22 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
case NL80211_IFTYPE_MESH_POINT:
err = ieee80211_mesh_finish_csa(sdata);
if (err < 0)
- return;
+ goto unlock;
break;
#endif
default:
WARN_ON(1);
- return;
+ goto unlock;
}
- sdata->vif.csa_active = false;
ieee80211_wake_queues_by_reason(&sdata->local->hw,
IEEE80211_MAX_QUEUE_MAP,
IEEE80211_QUEUE_STOP_REASON_CSA);
cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
+
+unlock:
+ sdata_unlock(sdata);
}
static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e39fadc..1355abf 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10804,21 +10804,18 @@ void cfg80211_ch_switch_notify(struct net_device *dev,
struct wiphy *wiphy = wdev->wiphy;
struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+ ASSERT_WDEV_LOCK(wdev);
+
trace_cfg80211_ch_switch_notify(dev, chandef);
- wdev_lock(wdev);
-
if (WARN_ON(wdev->iftype != NL80211_IFTYPE_AP &&
wdev->iftype != NL80211_IFTYPE_P2P_GO &&
wdev->iftype != NL80211_IFTYPE_ADHOC &&
wdev->iftype != NL80211_IFTYPE_MESH_POINT))
- goto out;
+ return;
wdev->channel = chandef->chan;
nl80211_ch_switch_notify(rdev, dev, chandef, GFP_KERNEL);
-out:
- wdev_unlock(wdev);
- return;
}
EXPORT_SYMBOL(cfg80211_ch_switch_notify);
--
1.7.10.4