From: Johannes Berg <[email protected]>
The scan code creates an iflist_mtx -> mtx locking dependency,
and a few other places, notably radar detection, were creating
the opposite dependency, causing lockdep to complain. As scan
and radar detection are mutually exclusive, the deadlock can't
really happen in practice, but it's still bad form.
A similar issue exists in the monitor mode code, but this is
only used by channel-context drivers right now and those have
to have hardware scan, so that also can't happen.
Still, fix these issues by making some of the channel context
code require the mtx to be held rather than acquiring it, thus
allowing the monitor/radar callers to keep the iflist_mtx->mtx
lock ordering.
While at it, also fix access to the local->scanning variable
in the radar code, and document that radar_detect_enabled is
now properly protected by the mtx.
Reported-by: Kalle Valo <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/cfg.c | 26 ++++++++++++++++++++++----
net/mac80211/chan.c | 21 +++++++++++----------
net/mac80211/ibss.c | 6 ++++++
net/mac80211/iface.c | 6 ++++++
net/mac80211/mlme.c | 15 ++++++++++++++-
net/mac80211/util.c | 2 ++
6 files changed, 61 insertions(+), 15 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index ac18528..7925881 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -828,6 +828,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
if (cfg80211_chandef_identical(&local->monitor_chandef, chandef))
return 0;
+ mutex_lock(&local->mtx);
mutex_lock(&local->iflist_mtx);
if (local->use_chanctx) {
sdata = rcu_dereference_protected(
@@ -846,6 +847,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
if (ret == 0)
local->monitor_chandef = *chandef;
mutex_unlock(&local->iflist_mtx);
+ mutex_unlock(&local->mtx);
return ret;
}
@@ -951,6 +953,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
struct cfg80211_ap_settings *params)
{
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_local *local = sdata->local;
struct beacon_data *old;
struct ieee80211_sub_if_data *vlan;
u32 changed = BSS_CHANGED_BEACON_INT |
@@ -969,8 +972,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
sdata->needed_rx_chains = sdata->local->rx_chains;
sdata->radar_required = params->radar_required;
+ mutex_lock(&local->mtx);
err = ieee80211_vif_use_channel(sdata, ¶ms->chandef,
IEEE80211_CHANCTX_SHARED);
+ mutex_unlock(&local->mtx);
if (err)
return err;
ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
@@ -1121,7 +1126,9 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
skb_queue_purge(&sdata->u.ap.ps.bc_buf);
ieee80211_vif_copy_chanctx_to_vlans(sdata, true);
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&local->mtx);
return 0;
}
@@ -1944,8 +1951,10 @@ static int ieee80211_join_mesh(struct wiphy *wiphy, struct net_device *dev,
sdata->smps_mode = IEEE80211_SMPS_OFF;
sdata->needed_rx_chains = sdata->local->rx_chains;
+ mutex_lock(&local->mtx);
err = ieee80211_vif_use_channel(sdata, &setup->chandef,
IEEE80211_CHANCTX_SHARED);
+ mutex_unlock(&local->mtx);
if (err)
return err;
@@ -1957,7 +1966,9 @@ static int ieee80211_leave_mesh(struct wiphy *wiphy, struct net_device *dev)
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
ieee80211_stop_mesh(sdata);
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&local->mtx);
return 0;
}
@@ -2895,8 +2906,11 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
unsigned long timeout;
int err;
- if (!list_empty(&local->roc_list) || local->scanning)
- return -EBUSY;
+ mutex_lock(&local->mtx);
+ if (!list_empty(&local->roc_list) || local->scanning) {
+ err = -EBUSY;
+ goto out_unlock;
+ }
/* whatever, but channel contexts should not complain about that one */
sdata->smps_mode = IEEE80211_SMPS_OFF;
@@ -2908,13 +2922,15 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
IEEE80211_CHANCTX_SHARED);
mutex_unlock(&local->iflist_mtx);
if (err)
- return err;
+ goto out_unlock;
timeout = msecs_to_jiffies(IEEE80211_DFS_MIN_CAC_TIME_MS);
ieee80211_queue_delayed_work(&sdata->local->hw,
&sdata->dfs_cac_timer_work, timeout);
- return 0;
+ out_unlock:
+ mutex_unlock(&local->mtx);
+ return err;
}
static struct cfg80211_beacon_data *
@@ -2990,7 +3006,9 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
goto unlock;
sdata->radar_required = sdata->csa_radar_required;
+ mutex_lock(&local->mtx);
err = ieee80211_vif_change_channel(sdata, &changed);
+ mutex_unlock(&local->mtx);
if (WARN_ON(err < 0))
goto unlock;
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f20a98a..f43613a 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -232,8 +232,8 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
if (!local->use_chanctx)
local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
- /* acquire mutex to prevent idle from changing */
- mutex_lock(&local->mtx);
+ /* we hold the mutex to prevent idle from changing */
+ lockdep_assert_held(&local->mtx);
/* turn idle off *before* setting channel -- some drivers need that */
changed = ieee80211_idle_off(local);
if (changed)
@@ -246,19 +246,14 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
err = drv_add_chanctx(local, ctx);
if (err) {
kfree(ctx);
- ctx = ERR_PTR(err);
-
ieee80211_recalc_idle(local);
- goto out;
+ return ERR_PTR(err);
}
}
/* and keep the mutex held until the new chanctx is on the list */
list_add_rcu(&ctx->list, &local->chanctx_list);
- out:
- mutex_unlock(&local->mtx);
-
return ctx;
}
@@ -294,9 +289,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
/* throw a warning if this wasn't the only channel context. */
WARN_ON(check_single_channel && !list_empty(&local->chanctx_list));
- mutex_lock(&local->mtx);
ieee80211_recalc_idle(local);
- mutex_unlock(&local->mtx);
}
static int ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
@@ -364,6 +357,8 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
bool radar_enabled;
lockdep_assert_held(&local->chanctx_mtx);
+ /* for setting local->radar_detect_enabled */
+ lockdep_assert_held(&local->mtx);
radar_enabled = ieee80211_is_radar_required(local);
@@ -518,6 +513,8 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
struct ieee80211_chanctx *ctx;
int ret;
+ lockdep_assert_held(&local->mtx);
+
WARN_ON(sdata->dev && netif_carrier_ok(sdata->dev));
mutex_lock(&local->chanctx_mtx);
@@ -558,6 +555,8 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
int ret;
u32 chanctx_changed = 0;
+ lockdep_assert_held(&local->mtx);
+
/* should never be called if not performing a channel switch. */
if (WARN_ON(!sdata->vif.csa_active))
return -EINVAL;
@@ -655,6 +654,8 @@ void ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
{
WARN_ON(sdata->dev && netif_carrier_ok(sdata->dev));
+ lockdep_assert_held(&sdata->local->mtx);
+
mutex_lock(&sdata->local->chanctx_mtx);
__ieee80211_vif_release_channel(sdata);
mutex_unlock(&sdata->local->chanctx_mtx);
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index d6ba8414..058e178 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -293,6 +293,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
radar_required = true;
}
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
if (ieee80211_vif_use_channel(sdata, &chandef,
ifibss->fixed_channel ?
@@ -301,6 +302,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
sdata_info(sdata, "Failed to join IBSS, no channel context\n");
return;
}
+ mutex_unlock(&local->mtx);
memcpy(ifibss->bssid, bssid, ETH_ALEN);
@@ -363,7 +365,9 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
sdata->vif.bss_conf.ssid_len = 0;
RCU_INIT_POINTER(ifibss->presp, NULL);
kfree_rcu(presp, rcu_head);
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&local->mtx);
sdata_info(sdata, "Failed to join IBSS, driver failure: %d\n",
err);
return;
@@ -747,7 +751,9 @@ static void ieee80211_ibss_disconnect(struct ieee80211_sub_if_data *sdata)
ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED |
BSS_CHANGED_IBSS);
drv_leave_ibss(local, sdata);
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&local->mtx);
}
static void ieee80211_csa_connection_drop_work(struct work_struct *work)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 3d2168c..d2b6bfc 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -418,8 +418,10 @@ int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
return ret;
}
+ mutex_lock(&local->mtx);
ret = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
IEEE80211_CHANCTX_EXCLUSIVE);
+ mutex_unlock(&local->mtx);
if (ret) {
drv_remove_interface(local, sdata);
kfree(sdata);
@@ -456,7 +458,9 @@ void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
synchronize_net();
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&local->mtx);
drv_remove_interface(local, sdata);
@@ -826,9 +830,11 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
if (sdata->wdev.cac_started) {
chandef = sdata->vif.bss_conf.chandef;
WARN_ON(local->suspended);
+ mutex_lock(&local->mtx);
mutex_lock(&local->iflist_mtx);
ieee80211_vif_release_channel(sdata);
mutex_unlock(&local->iflist_mtx);
+ mutex_unlock(&local->mtx);
cfg80211_cac_event(sdata->dev, &chandef,
NL80211_RADAR_CAC_ABORTED,
GFP_KERNEL);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 9c2c7ee..93792d7 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -888,7 +888,9 @@ static void ieee80211_chswitch_work(struct work_struct *work)
if (!ifmgd->associated)
goto out;
+ mutex_lock(&local->mtx);
ret = ieee80211_vif_change_channel(sdata, &changed);
+ mutex_unlock(&local->mtx);
if (ret) {
sdata_info(sdata,
"vif channel switch failed, disconnecting\n");
@@ -1401,7 +1403,9 @@ void ieee80211_dfs_cac_timer_work(struct work_struct *work)
dfs_cac_timer_work);
struct cfg80211_chan_def chandef = sdata->vif.bss_conf.chandef;
+ mutex_lock(&sdata->local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&sdata->local->mtx);
cfg80211_cac_event(sdata->dev, &chandef,
NL80211_RADAR_CAC_FINISHED,
GFP_KERNEL);
@@ -1747,7 +1751,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
ifmgd->have_beacon = false;
ifmgd->flags = 0;
+ mutex_lock(&local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&local->mtx);
sdata->encrypt_headroom = IEEE80211_ENCRYPT_HEADROOM;
}
@@ -2070,7 +2076,9 @@ static void ieee80211_destroy_auth_data(struct ieee80211_sub_if_data *sdata,
memset(sdata->u.mgd.bssid, 0, ETH_ALEN);
ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID);
sdata->u.mgd.flags = 0;
+ mutex_lock(&sdata->local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&sdata->local->mtx);
}
cfg80211_put_bss(sdata->local->hw.wiphy, auth_data->bss);
@@ -2319,7 +2327,9 @@ static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
memset(sdata->u.mgd.bssid, 0, ETH_ALEN);
ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID);
sdata->u.mgd.flags = 0;
+ mutex_lock(&sdata->local->mtx);
ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&sdata->local->mtx);
}
kfree(assoc_data);
@@ -3670,6 +3680,7 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
/* will change later if needed */
sdata->smps_mode = IEEE80211_SMPS_OFF;
+ mutex_lock(&local->mtx);
/*
* If this fails (possibly due to channel context sharing
* on incompatible channels, e.g. 80+80 and 160 sharing the
@@ -3681,13 +3692,15 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
/* don't downgrade for 5 and 10 MHz channels, though. */
if (chandef.width == NL80211_CHAN_WIDTH_5 ||
chandef.width == NL80211_CHAN_WIDTH_10)
- return ret;
+ goto out;
while (ret && chandef.width != NL80211_CHAN_WIDTH_20_NOHT) {
ifmgd->flags |= ieee80211_chandef_downgrade(&chandef);
ret = ieee80211_vif_use_channel(sdata, &chandef,
IEEE80211_CHANCTX_SHARED);
}
+ out:
+ mutex_unlock(&local->mtx);
return ret;
}
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 656648b..bc3685e 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2315,6 +2315,7 @@ void ieee80211_dfs_cac_cancel(struct ieee80211_local *local)
struct ieee80211_sub_if_data *sdata;
struct cfg80211_chan_def chandef;
+ mutex_lock(&local->mtx);
mutex_lock(&local->iflist_mtx);
list_for_each_entry(sdata, &local->interfaces, list) {
cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
@@ -2329,6 +2330,7 @@ void ieee80211_dfs_cac_cancel(struct ieee80211_local *local)
}
}
mutex_unlock(&local->iflist_mtx);
+ mutex_unlock(&local->mtx);
}
void ieee80211_dfs_radar_detected_work(struct work_struct *work)
--
1.8.5.1
One locking oddness below.
/Jones
On 12/18/2013 10:13 AM, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> The scan code creates an iflist_mtx -> mtx locking dependency,
> and a few other places, notably radar detection, were creating
> the opposite dependency, causing lockdep to complain. As scan
> and radar detection are mutually exclusive, the deadlock can't
> really happen in practice, but it's still bad form.
>
> A similar issue exists in the monitor mode code, but this is
> only used by channel-context drivers right now and those have
> to have hardware scan, so that also can't happen.
>
> Still, fix these issues by making some of the channel context
> code require the mtx to be held rather than acquiring it, thus
> allowing the monitor/radar callers to keep the iflist_mtx->mtx
> lock ordering.
>
> While at it, also fix access to the local->scanning variable
> in the radar code, and document that radar_detect_enabled is
> now properly protected by the mtx.
>
> Reported-by: Kalle Valo <[email protected]>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/cfg.c | 26 ++++++++++++++++++++++----
> net/mac80211/chan.c | 21 +++++++++++----------
> net/mac80211/ibss.c | 6 ++++++
> net/mac80211/iface.c | 6 ++++++
> net/mac80211/mlme.c | 15 ++++++++++++++-
> net/mac80211/util.c | 2 ++
> 6 files changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index ac18528..7925881 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -828,6 +828,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> if (cfg80211_chandef_identical(&local->monitor_chandef, chandef))
> return 0;
>
> + mutex_lock(&local->mtx);
> mutex_lock(&local->iflist_mtx);
> if (local->use_chanctx) {
> sdata = rcu_dereference_protected(
> @@ -846,6 +847,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> if (ret == 0)
> local->monitor_chandef = *chandef;
> mutex_unlock(&local->iflist_mtx);
> + mutex_unlock(&local->mtx);
>
> return ret;
> }
> @@ -951,6 +953,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
> struct cfg80211_ap_settings *params)
> {
> struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> + struct ieee80211_local *local = sdata->local;
> struct beacon_data *old;
> struct ieee80211_sub_if_data *vlan;
> u32 changed = BSS_CHANGED_BEACON_INT |
> @@ -969,8 +972,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
> sdata->needed_rx_chains = sdata->local->rx_chains;
> sdata->radar_required = params->radar_required;
>
> + mutex_lock(&local->mtx);
> err = ieee80211_vif_use_channel(sdata, ¶ms->chandef,
> IEEE80211_CHANCTX_SHARED);
> + mutex_unlock(&local->mtx);
> if (err)
> return err;
> ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
> @@ -1121,7 +1126,9 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
> skb_queue_purge(&sdata->u.ap.ps.bc_buf);
>
> ieee80211_vif_copy_chanctx_to_vlans(sdata, true);
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
>
> return 0;
> }
> @@ -1944,8 +1951,10 @@ static int ieee80211_join_mesh(struct wiphy *wiphy, struct net_device *dev,
> sdata->smps_mode = IEEE80211_SMPS_OFF;
> sdata->needed_rx_chains = sdata->local->rx_chains;
>
> + mutex_lock(&local->mtx);
> err = ieee80211_vif_use_channel(sdata, &setup->chandef,
> IEEE80211_CHANCTX_SHARED);
> + mutex_unlock(&local->mtx);
> if (err)
> return err;
>
> @@ -1957,7 +1966,9 @@ static int ieee80211_leave_mesh(struct wiphy *wiphy, struct net_device *dev)
> struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>
> ieee80211_stop_mesh(sdata);
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
>
> return 0;
> }
> @@ -2895,8 +2906,11 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
> unsigned long timeout;
> int err;
>
> - if (!list_empty(&local->roc_list) || local->scanning)
> - return -EBUSY;
> + mutex_lock(&local->mtx);
> + if (!list_empty(&local->roc_list) || local->scanning) {
> + err = -EBUSY;
> + goto out_unlock;
> + }
>
> /* whatever, but channel contexts should not complain about that one */
> sdata->smps_mode = IEEE80211_SMPS_OFF;
> @@ -2908,13 +2922,15 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
> IEEE80211_CHANCTX_SHARED);
> mutex_unlock(&local->iflist_mtx);
> if (err)
> - return err;
> + goto out_unlock;
>
> timeout = msecs_to_jiffies(IEEE80211_DFS_MIN_CAC_TIME_MS);
> ieee80211_queue_delayed_work(&sdata->local->hw,
> &sdata->dfs_cac_timer_work, timeout);
>
> - return 0;
> + out_unlock:
> + mutex_unlock(&local->mtx);
> + return err;
> }
>
> static struct cfg80211_beacon_data *
> @@ -2990,7 +3006,9 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
> goto unlock;
>
> sdata->radar_required = sdata->csa_radar_required;
> + mutex_lock(&local->mtx);
> err = ieee80211_vif_change_channel(sdata, &changed);
> + mutex_unlock(&local->mtx);
> if (WARN_ON(err < 0))
> goto unlock;
>
> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> index f20a98a..f43613a 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -232,8 +232,8 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
> if (!local->use_chanctx)
> local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
>
> - /* acquire mutex to prevent idle from changing */
> - mutex_lock(&local->mtx);
> + /* we hold the mutex to prevent idle from changing */
> + lockdep_assert_held(&local->mtx);
> /* turn idle off *before* setting channel -- some drivers need that */
> changed = ieee80211_idle_off(local);
> if (changed)
> @@ -246,19 +246,14 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
> err = drv_add_chanctx(local, ctx);
> if (err) {
> kfree(ctx);
> - ctx = ERR_PTR(err);
> -
> ieee80211_recalc_idle(local);
> - goto out;
> + return ERR_PTR(err);
> }
> }
>
> /* and keep the mutex held until the new chanctx is on the list */
> list_add_rcu(&ctx->list, &local->chanctx_list);
>
> - out:
> - mutex_unlock(&local->mtx);
> -
> return ctx;
> }
>
> @@ -294,9 +289,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
> /* throw a warning if this wasn't the only channel context. */
> WARN_ON(check_single_channel && !list_empty(&local->chanctx_list));
>
> - mutex_lock(&local->mtx);
> ieee80211_recalc_idle(local);
> - mutex_unlock(&local->mtx);
> }
>
> static int ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
> @@ -364,6 +357,8 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
> bool radar_enabled;
>
> lockdep_assert_held(&local->chanctx_mtx);
> + /* for setting local->radar_detect_enabled */
> + lockdep_assert_held(&local->mtx);
>
> radar_enabled = ieee80211_is_radar_required(local);
>
> @@ -518,6 +513,8 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
> struct ieee80211_chanctx *ctx;
> int ret;
>
> + lockdep_assert_held(&local->mtx);
> +
> WARN_ON(sdata->dev && netif_carrier_ok(sdata->dev));
>
> mutex_lock(&local->chanctx_mtx);
> @@ -558,6 +555,8 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
> int ret;
> u32 chanctx_changed = 0;
>
> + lockdep_assert_held(&local->mtx);
> +
> /* should never be called if not performing a channel switch. */
> if (WARN_ON(!sdata->vif.csa_active))
> return -EINVAL;
> @@ -655,6 +654,8 @@ void ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
> {
> WARN_ON(sdata->dev && netif_carrier_ok(sdata->dev));
>
> + lockdep_assert_held(&sdata->local->mtx);
> +
> mutex_lock(&sdata->local->chanctx_mtx);
> __ieee80211_vif_release_channel(sdata);
> mutex_unlock(&sdata->local->chanctx_mtx);
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index d6ba8414..058e178 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -293,6 +293,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
> radar_required = true;
> }
>
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> if (ieee80211_vif_use_channel(sdata, &chandef,
> ifibss->fixed_channel ?
> @@ -301,6 +302,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
> sdata_info(sdata, "Failed to join IBSS, no channel context\n");
> return;
> }
> + mutex_unlock(&local->mtx);
At least looking at the patch only, the return above without unlock
seems suspicious.
>
> memcpy(ifibss->bssid, bssid, ETH_ALEN);
>
> @@ -363,7 +365,9 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
> sdata->vif.bss_conf.ssid_len = 0;
> RCU_INIT_POINTER(ifibss->presp, NULL);
> kfree_rcu(presp, rcu_head);
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
> sdata_info(sdata, "Failed to join IBSS, driver failure: %d\n",
> err);
> return;
> @@ -747,7 +751,9 @@ static void ieee80211_ibss_disconnect(struct ieee80211_sub_if_data *sdata)
> ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED |
> BSS_CHANGED_IBSS);
> drv_leave_ibss(local, sdata);
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
> }
>
> static void ieee80211_csa_connection_drop_work(struct work_struct *work)
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 3d2168c..d2b6bfc 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -418,8 +418,10 @@ int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
> return ret;
> }
>
> + mutex_lock(&local->mtx);
> ret = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
> IEEE80211_CHANCTX_EXCLUSIVE);
> + mutex_unlock(&local->mtx);
> if (ret) {
> drv_remove_interface(local, sdata);
> kfree(sdata);
> @@ -456,7 +458,9 @@ void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
>
> synchronize_net();
>
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
>
> drv_remove_interface(local, sdata);
>
> @@ -826,9 +830,11 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
> if (sdata->wdev.cac_started) {
> chandef = sdata->vif.bss_conf.chandef;
> WARN_ON(local->suspended);
> + mutex_lock(&local->mtx);
> mutex_lock(&local->iflist_mtx);
> ieee80211_vif_release_channel(sdata);
> mutex_unlock(&local->iflist_mtx);
> + mutex_unlock(&local->mtx);
> cfg80211_cac_event(sdata->dev, &chandef,
> NL80211_RADAR_CAC_ABORTED,
> GFP_KERNEL);
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 9c2c7ee..93792d7 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -888,7 +888,9 @@ static void ieee80211_chswitch_work(struct work_struct *work)
> if (!ifmgd->associated)
> goto out;
>
> + mutex_lock(&local->mtx);
> ret = ieee80211_vif_change_channel(sdata, &changed);
> + mutex_unlock(&local->mtx);
> if (ret) {
> sdata_info(sdata,
> "vif channel switch failed, disconnecting\n");
> @@ -1401,7 +1403,9 @@ void ieee80211_dfs_cac_timer_work(struct work_struct *work)
> dfs_cac_timer_work);
> struct cfg80211_chan_def chandef = sdata->vif.bss_conf.chandef;
>
> + mutex_lock(&sdata->local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&sdata->local->mtx);
> cfg80211_cac_event(sdata->dev, &chandef,
> NL80211_RADAR_CAC_FINISHED,
> GFP_KERNEL);
> @@ -1747,7 +1751,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> ifmgd->have_beacon = false;
>
> ifmgd->flags = 0;
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
>
> sdata->encrypt_headroom = IEEE80211_ENCRYPT_HEADROOM;
> }
> @@ -2070,7 +2076,9 @@ static void ieee80211_destroy_auth_data(struct ieee80211_sub_if_data *sdata,
> memset(sdata->u.mgd.bssid, 0, ETH_ALEN);
> ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID);
> sdata->u.mgd.flags = 0;
> + mutex_lock(&sdata->local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&sdata->local->mtx);
> }
>
> cfg80211_put_bss(sdata->local->hw.wiphy, auth_data->bss);
> @@ -2319,7 +2327,9 @@ static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
> memset(sdata->u.mgd.bssid, 0, ETH_ALEN);
> ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID);
> sdata->u.mgd.flags = 0;
> + mutex_lock(&sdata->local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&sdata->local->mtx);
> }
>
> kfree(assoc_data);
> @@ -3670,6 +3680,7 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
> /* will change later if needed */
> sdata->smps_mode = IEEE80211_SMPS_OFF;
>
> + mutex_lock(&local->mtx);
> /*
> * If this fails (possibly due to channel context sharing
> * on incompatible channels, e.g. 80+80 and 160 sharing the
> @@ -3681,13 +3692,15 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
> /* don't downgrade for 5 and 10 MHz channels, though. */
> if (chandef.width == NL80211_CHAN_WIDTH_5 ||
> chandef.width == NL80211_CHAN_WIDTH_10)
> - return ret;
> + goto out;
>
> while (ret && chandef.width != NL80211_CHAN_WIDTH_20_NOHT) {
> ifmgd->flags |= ieee80211_chandef_downgrade(&chandef);
> ret = ieee80211_vif_use_channel(sdata, &chandef,
> IEEE80211_CHANCTX_SHARED);
> }
> + out:
> + mutex_unlock(&local->mtx);
> return ret;
> }
>
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 656648b..bc3685e 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -2315,6 +2315,7 @@ void ieee80211_dfs_cac_cancel(struct ieee80211_local *local)
> struct ieee80211_sub_if_data *sdata;
> struct cfg80211_chan_def chandef;
>
> + mutex_lock(&local->mtx);
> mutex_lock(&local->iflist_mtx);
> list_for_each_entry(sdata, &local->interfaces, list) {
> cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
> @@ -2329,6 +2330,7 @@ void ieee80211_dfs_cac_cancel(struct ieee80211_local *local)
> }
> }
> mutex_unlock(&local->iflist_mtx);
> + mutex_unlock(&local->mtx);
> }
>
> void ieee80211_dfs_radar_detected_work(struct work_struct *work)
>
> From: Johannes Berg <[email protected]>
>
> The scan code creates an iflist_mtx -> mtx locking dependency,
> and a few other places, notably radar detection, were creating
> the opposite dependency, causing lockdep to complain. As scan
> and radar detection are mutually exclusive, the deadlock can't
> really happen in practice, but it's still bad form.
>
> A similar issue exists in the monitor mode code, but this is
> only used by channel-context drivers right now and those have
> to have hardware scan, so that also can't happen.
>
> Still, fix these issues by making some of the channel context
> code require the mtx to be held rather than acquiring it, thus
> allowing the monitor/radar callers to keep the iflist_mtx->mtx
> lock ordering.
>
> While at it, also fix access to the local->scanning variable
> in the radar code, and document that radar_detect_enabled is
> now properly protected by the mtx.
>
> Reported-by: Kalle Valo <[email protected]>
> Signed-off-by: Johannes Berg <[email protected]>
There are two little issues (see below). Otherwise this appears to fix the
problem Kalle reported, at least I can now scan and start hostap (using ath9k)
without getting this lockdep splat. If you want, add my Tested-by or Acked-by.
> @@ -1944,8 +1951,10 @@ static int ieee80211_join_mesh(struct wiphy *wiphy,
> struct net_device *dev, sdata->smps_mode = IEEE80211_SMPS_OFF;
> sdata->needed_rx_chains = sdata->local->rx_chains;
>
> + mutex_lock(&local->mtx);
> err = ieee80211_vif_use_channel(sdata, &setup->chandef,
> IEEE80211_CHANCTX_SHARED);
> + mutex_unlock(&local->mtx);
> if (err)
> return err;
local is not defined in ieee80211_join_mesh().
>
> @@ -1957,7 +1966,9 @@ static int ieee80211_leave_mesh(struct wiphy *wiphy,
> struct net_device *dev) struct ieee80211_sub_if_data *sdata =
> IEEE80211_DEV_TO_SUB_IF(dev);
>
> ieee80211_stop_mesh(sdata);
> + mutex_lock(&local->mtx);
> ieee80211_vif_release_channel(sdata);
> + mutex_unlock(&local->mtx);
>
> return 0;
> }
Same problem here.
Thanks!
Simon
On Wed, 2013-12-18 at 17:58 +0100, Jones Desougi wrote:
> One locking oddness below.
> > @@ -301,6 +302,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
> > sdata_info(sdata, "Failed to join IBSS, no channel context\n");
> > return;
> > }
> > + mutex_unlock(&local->mtx);
>
> At least looking at the patch only, the return above without unlock
> seems suspicious.
Indeed. But please trim your quotes in the future :)
johannes