[PATCH wireless-2.6] mac80211: delete AddBA response timer

[PATCH wireless-2.6] mac80211: delete AddBA response timer

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

We never delete the addBA response timer, which
is typically fine, but if the station it belongs
to is deleted very quickly after starting the BA
session, before the peer had a chance to reply,
the timer may fire after the station struct has
been freed already. Therefore, we need to delete
the timer in a suitable spot -- best when the
session is being stopped (which will happen even
then) in which case the delete will be a no-op
most of the time.

I've reproduced the scenario and tested the fix.

Cc: stable@kernel.org
Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
This might be applicable to stable, but the code
has changed significantly, I'd appreciate any help
analysing and backporting it.

 net/mac80211/agg-tx.c |    2 ++
 1 file changed, 2 insertions(+)

--- wireless-testing.orig/net/mac80211/agg-tx.c	2010-10-05 21:00:59.000000000 +0200
+++ wireless-testing/net/mac80211/agg-tx.c	2010-10-05 21:01:11.000000000 +0200
@@ -176,6 +176,8 @@ int ___ieee80211_stop_tx_ba_session(stru
 
 	set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
 
+	del_timer_sync(&tid_tx->addba_resp_timer);
+
 	/*
 	 * After this packets are no longer handed right through
 	 * to the driver but are put onto tid_tx->pending instead,

Re: [PATCH wireless-2.6] mac80211: delete AddBA response timer

[Johannes Berg]

On Tue, 2010-10-05 at 21:40 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> We never delete the addBA response timer, which
> is typically fine, but if the station it belongs
> to is deleted very quickly after starting the BA
> session, before the peer had a chance to reply,
> the timer may fire after the station struct has
> been freed already. Therefore, we need to delete
> the timer in a suitable spot -- best when the
> session is being stopped (which will happen even
> then) in which case the delete will be a no-op
> most of the time.
> 
> I've reproduced the scenario and tested the fix.

Ok, can you add:

This fixes the crash reported at
http://mid.gmane.org/4CAB6F96.6090701@candelatech.com

to the changelog?

johannes

Re: [PATCH wireless-2.6] mac80211: delete AddBA response timer

[Ben Greear]

On 10/05/2010 01:10 PM, Johannes Berg wrote:
> On Tue, 2010-10-05 at 21:40 +0200, Johannes Berg wrote:
>> From: Johannes Berg<johannes.berg@intel.com>
>>
>> We never delete the addBA response timer, which
>> is typically fine, but if the station it belongs
>> to is deleted very quickly after starting the BA
>> session, before the peer had a chance to reply,
>> the timer may fire after the station struct has
>> been freed already. Therefore, we need to delete
>> the timer in a suitable spot -- best when the
>> session is being stopped (which will happen even
>> then) in which case the delete will be a no-op
>> most of the time.
>>
>> I've reproduced the scenario and tested the fix.
>
> Ok, can you add:
>
> This fixes the crash reported at
> http://mid.gmane.org/4CAB6F96.6090701@candelatech.com

I can no longer reproduce that problem, so it looks fixed
to me.

The data corruption issue still exists, however...

Thanks!
Ben

>
> to the changelog?
>
> 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


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH wireless-2.6] mac80211: delete AddBA response timer

[Johannes Berg]

On Tue, 2010-10-05 at 13:36 -0700, Ben Greear wrote:

> >> I've reproduced the scenario and tested the fix.

> > This fixes the crash reported at
> > http://mid.gmane.org/4CAB6F96.6090701@candelatech.com
> 
> I can no longer reproduce that problem, so it looks fixed
> to me.

Yeah .. I did actually go and reproduce the crash by strategically
adding an msleep(200) to the mac80211 code and then doing some things
from userspace :-)

> The data corruption issue still exists, however...

That looks very much like an ath9k issue, it seems like it's doing DMA
into already freed SKBs. Come to think of it, maybe that wmb() patch
might help?:

johannes

Re: [PATCH wireless-2.6] mac80211: delete AddBA response timer

[Ben Greear]

On 10/05/2010 01:41 PM, Johannes Berg wrote:
> On Tue, 2010-10-05 at 13:36 -0700, Ben Greear wrote:
>
>>>> I've reproduced the scenario and tested the fix.
>
>>> This fixes the crash reported at
>>> http://mid.gmane.org/4CAB6F96.6090701@candelatech.com
>>
>> I can no longer reproduce that problem, so it looks fixed
>> to me.
>
> Yeah .. I did actually go and reproduce the crash by strategically
> adding an msleep(200) to the mac80211 code and then doing some things
> from userspace :-)
>
>> The data corruption issue still exists, however...
>
> That looks very much like an ath9k issue, it seems like it's doing DMA
> into already freed SKBs. Come to think of it, maybe that wmb() patch
> might help?:

I can try it.  I've been trying to crawl through the rx path line by line,
and once you get into mac80211, it's a real slow slog for me :)

For what it's worth, ath9k looked pretty clean to me, but if it's
something clever like wmb() stuff, then I wouldn't notice anyway...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com