On Wed, 2013-06-12 at 11:21 -0700, Ben Greear wrote:
> In ieee80211_start_tx_ba_session we are accessing and assigning the tid_start_tx
> without holding the ampdu_mlme.mtx mutex.
>
> spin_lock_bh(&sta->lock);
> .....
> tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
> /* check if the TID is not in aggregation flow already */
> if (tid_tx || sta->ampdu_mlme.tid_start_tx[tid]) {
>
> ....
>
> /*
> * Finally, assign it to the start array; the work item will
> * collect it and move it to the normal array.
> */
> sta->ampdu_mlme.tid_start_tx[tid] = tid_tx;
>
>
> Elsewhere, in ieee80211_ba_session_work, we access the tid_start_tx
> without the sta->lock held, but with the ampdu_mlme.mtx held.
Yeah, that seems wrong.
> I think we should probably hold ampdu_mlme.mtx in ieee80211_start_tx_ba_session
> or make sure we hold sta->lock in ieee80211_ba_session_work.
Can't hold the mutex there, but we can do the lock (I'll comment on your
patch separately)
> unreferenced object 0xffff880219b4de40 (size 192):
> comm "softirq", pid 0, jiffies 4296416789 (age 1257.971s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff815bc02c>] kmemleak_alloc+0x73/0x98
> [<ffffffff8117d4b4>] slab_post_alloc_hook+0x28/0x2a
> [<ffffffff8117f4a6>] kmem_cache_alloc_trace+0xa5/0xcc
> [<ffffffffa0365221>] ieee80211_start_tx_ba_session+0x24b/0x360 [mac80211]
> [<ffffffffa03a98f3>] minstrel_ht_tx_status+0x79a/0x7a9 [mac80211]
> [<ffffffffa035d1cd>] ieee80211_tx_status+0x3af/0x947 [mac80211]
When did this report get printed?
I have a feeling what happens is that start is requested, and then
before ieee80211_ba_session_work() gets a chance to run the station is
destroyed.
Should probably have something like this:
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index b429798..aaf68d2 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -149,6 +149,7 @@ static void cleanup_single_sta(struct sta_info *sta)
* directly by station destruction.
*/
for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
+ kfree(sta->ampdu_mlme.tid_start_tx[i]);
tid_tx = rcu_dereference_raw(sta->ampdu_mlme.tid_tx[i]);
if (!tid_tx)
continue;
johannes
--
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
