Hi
2011/11/24 Johannes Berg <johannes@xxxxxxxxxxxxxxxx>:
> 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);
>
> /*
>
>
>
Yes, this patch does make sense to me.
--
Truthfully yours,
Martynov Nikolay.
Email: mar.kolya@xxxxxxxxx
--
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
