On Thu, 2011-11-24 at 19:39 +0100, Johannes Berg wrote:
> > And one more thing, if you do not mind.
> > 'ieee80211_stop_tx_ba_session' tests tx_tid->state for
> > HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
> > '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
> > sta->lock is not being held. It seems to be that this could lead to
> > problems if these two functions get called at about same time (which
> > could easily happen when this patch is applied). Although actual
> > window when lock is not help is really small. But I think the way it
> > is currently done could still lead to problems. I think
> > 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
> > to be covered by spin lock. I can include this as a part of next
> > version of my patch. I'd really appreciate your thoughts about this.
>
> Yeah that does indeed seem like an issue
Actually I think the right way to fix it would be the change below, do
you agree? I think just moving the set_bit doesn't help because other
code might call right into ___ieee80211_stop_tx_ba_session (or with just
two underscores on front)
johannes
--- wireless-testing.orig/net/mac80211/agg-tx.c 2011-11-09 10:07:34.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c 2011-11-24 19:44:51.000000000 +0100
@@ -162,6 +162,12 @@ int ___ieee80211_stop_tx_ba_session(stru
return -ENOENT;
}
+ /* if we're already stopping ignore any new requests to stop */
+ if (test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
+ spin_unlock_bh(&sta->lock);
+ return -EALREADY;
+ }
+
if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
/* not even started yet! */
ieee80211_assign_tid_tx(sta, tid, NULL);
@@ -170,6 +176,8 @@ int ___ieee80211_stop_tx_ba_session(stru
return 0;
}
+ set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
+
spin_unlock_bh(&sta->lock);
#ifdef CONFIG_MAC80211_HT_DEBUG
@@ -177,8 +185,6 @@ int ___ieee80211_stop_tx_ba_session(stru
sta->sta.addr, tid);
#endif /* CONFIG_MAC80211_HT_DEBUG */
- set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
-
del_timer_sync(&tid_tx->addba_resp_timer);
/*
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
