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).
These patches contain changes for issues in the RFC series pointed out by
Johannes. I've also found another possible leak so I added this to the series.
Patches are based on mac80211-next.
Cheers,
Simon
Simon Wunderlich (4):
cfg80211/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
mac80211: don't leak next beacon when csa is aborted
drivers/net/wireless/ath/ath6kl/cfg80211.c | 2 ++
include/net/cfg80211.h | 3 +-
net/mac80211/cfg.c | 47 +++++++++++++++++++---------
net/mac80211/ieee80211_i.h | 3 ++
net/mac80211/iface.c | 7 ++++-
net/wireless/nl80211.c | 31 ++++++++++++------
6 files changed, 67 insertions(+), 26 deletions(-)
--
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]>
---
Changes to RFC:
* move locking from mac80211 to cfg80211 (thanks Johannes)
---
net/mac80211/cfg.c | 20 ++++++++++++--------
net/mac80211/ieee80211_i.h | 3 +++
net/wireless/nl80211.c | 22 +++++++++++++++++++---
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1f07aa6..1cdf2fb 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,7 +959,7 @@ 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);
+ old = sdata_dereference(sdata->u.ap.beacon, sdata);
if (old)
return -EALREADY;
@@ -1020,7 +1021,8 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
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);
@@ -1051,7 +1053,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
if (sdata->vif.csa_active)
return -EBUSY;
- old = rtnl_dereference(sdata->u.ap.beacon);
+ old = sdata_dereference(sdata->u.ap.beacon, sdata);
if (!old)
return -ENOENT;
@@ -1071,10 +1073,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;
@@ -1975,7 +1977,7 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
enum ieee80211_band band;
u32 changed = 0;
- if (!rtnl_dereference(sdata->u.ap.beacon))
+ if (!sdata_dereference(sdata->u.ap.beacon, sdata))
return -ENOENT;
band = ieee80211_get_sdata_band(sdata);
@@ -3045,6 +3047,8 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
struct ieee80211_if_mesh __maybe_unused *ifmsh;
int err, num_chanctx;
+ lockdep_assert_held(&sdata->wdev.mtx);
+
if (!list_empty(&local->roc_list) || local->scanning)
return -EBUSY;
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)
{
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e39fadc..b1db645 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3217,6 +3217,7 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
return PTR_ERR(params.acl);
}
+ wdev_lock(wdev);
err = rdev_start_ap(rdev, dev, ¶ms);
if (!err) {
wdev->preset_chandef = params.chandef;
@@ -3225,6 +3226,7 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
wdev->ssid_len = params.ssid_len;
memcpy(wdev->ssid, params.ssid, wdev->ssid_len);
}
+ wdev_unlock(wdev);
kfree(params.acl);
@@ -3253,7 +3255,11 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
if (err)
return err;
- return rdev_change_beacon(rdev, dev, ¶ms);
+ wdev_lock(wdev);
+ err = rdev_change_beacon(rdev, dev, ¶ms);
+ wdev_unlock(wdev);
+
+ return err;
}
static int nl80211_stop_ap(struct sk_buff *skb, struct genl_info *info)
@@ -4459,7 +4465,9 @@ static int nl80211_set_bss(struct sk_buff *skb, struct genl_info *info)
{
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct net_device *dev = info->user_ptr[1];
+ struct wireless_dev *wdev = dev->ieee80211_ptr;
struct bss_parameters params;
+ int err;
memset(¶ms, 0, sizeof(params));
/* default to not changing parameters */
@@ -4525,7 +4533,11 @@ static int nl80211_set_bss(struct sk_buff *skb, struct genl_info *info)
dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
return -EOPNOTSUPP;
- return rdev_change_bss(rdev, dev, ¶ms);
+ wdev_lock(wdev);
+ err = rdev_change_bss(rdev, dev, ¶ms);
+ wdev_unlock(wdev);
+
+ return err;
}
static const struct nla_policy reg_rule_policy[NL80211_REG_RULE_ATTR_MAX + 1] = {
@@ -5790,7 +5802,11 @@ skip_beacons:
if (info->attrs[NL80211_ATTR_CH_SWITCH_BLOCK_TX])
params.block_tx = true;
- return rdev_channel_switch(rdev, dev, ¶ms);
+ wdev_lock(wdev);
+ err = rdev_channel_switch(rdev, dev, ¶ms);
+ wdev_unlock(wdev);
+
+ return err;
}
static int nl80211_send_bss(struct sk_buff *msg, struct netlink_callback *cb,
--
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 1cdf2fb..a135e6f 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2988,13 +2988,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;
@@ -3003,11 +3008,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;
@@ -3021,20 +3028,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 b1db645..c17c2c3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10820,21 +10820,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
Signed-off-by: Simon Wunderlich <[email protected]>
---
net/mac80211/cfg.c | 5 +++++
net/mac80211/iface.c | 7 ++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 9f19cde..8bf0cc0 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1079,7 +1079,12 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
/* abort any running channel switch */
+ if (sdata->vif.csa_active) {
+ kfree(sdata->u.ap.next_beacon);
+ sdata->u.ap.next_beacon = NULL;
+ }
sdata->vif.csa_active = false;
+
cancel_work_sync(&sdata->u.ap.request_smps_work);
/* turn off carrier for this interface and dependent VLANs */
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index d226751..dce4398 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -820,8 +820,13 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
cancel_work_sync(&local->dynamic_ps_enable_work);
cancel_work_sync(&sdata->recalc_smps);
- sdata->vif.csa_active = false;
cancel_work_sync(&sdata->csa_finalize_work);
+ /* free left-over next beacon if a CSA was aborted */
+ if (sdata->vif.csa_active && sdata->vif.type == NL80211_IFTYPE_AP) {
+ kfree(sdata->u.ap.next_beacon);
+ sdata->u.ap.next_beacon = NULL;
+ }
+ sdata->vif.csa_active = false;
cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
--
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 a135e6f..9f19cde 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1080,7 +1080,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