| Message ID | 1394191196-6425-1-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Mar 7, 2014 at 1:19 PM, Michal Kazior <michal.kazior@tieto.com> wrote: > Channel context refcount is protected by > chanctx_mtx. Accessing the value without holding > the mutex is racy. RCU section didn't guarantee > anything here. > > Theoretically ieee80211_channel_switch() could > fail to see refcount change and read "1" instead > of, e.g. "2". This means mac80211 could accept CSA > even though it shouldn't have. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > --- [...] > @@ -3233,23 +3233,23 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, > &sdata->vif.bss_conf.chandef)) > return -EINVAL; > > - rcu_read_lock(); > + mutex_lock(&local->chanctx_mtx); > chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); this should probably be rcu_dereference_protected now? Eliad. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2014-03-09 at 11:40 +0200, Eliad Peller wrote: > > @@ -3233,23 +3233,23 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, > > &sdata->vif.bss_conf.chandef)) > > return -EINVAL; > > > > - rcu_read_lock(); > > + mutex_lock(&local->chanctx_mtx); > > chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); > this should probably be rcu_dereference_protected now? Yes, in fact, lockdep ought to warn about this usage now, please enable it :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index aaa59d7..a79875c 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -3233,23 +3233,23 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, &sdata->vif.bss_conf.chandef)) return -EINVAL; - rcu_read_lock(); + mutex_lock(&local->chanctx_mtx); chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); if (!chanctx_conf) { - rcu_read_unlock(); + mutex_unlock(&local->chanctx_mtx); return -EBUSY; } /* don't handle for multi-VIF cases */ chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf); if (chanctx->refcount > 1) { - rcu_read_unlock(); + mutex_unlock(&local->chanctx_mtx); return -EBUSY; } num_chanctx = 0; list_for_each_entry_rcu(chanctx, &local->chanctx_list, list) num_chanctx++; - rcu_read_unlock(); + mutex_unlock(&local->chanctx_mtx); if (num_chanctx > 1) return -EBUSY;
Channel context refcount is protected by chanctx_mtx. Accessing the value without holding the mutex is racy. RCU section didn't guarantee anything here. Theoretically ieee80211_channel_switch() could fail to see refcount change and read "1" instead of, e.g. "2". This means mac80211 could accept CSA even though it shouldn't have. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- net/mac80211/cfg.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)