Possible leak in the ampdu aggregation code?

Possible leak in the ampdu aggregation code?

[Nishant Sarmukadam]

Hi,


I wanted to get some views on a possible issue while using mac80211 ampdu support. 
skb's from pending queue for a TID are spliced onto the local pending queue when tearing down a block ack session.
If aggregation is stopped before the ampdu state becomes HT_AGG_STATE_OPERATIONAL say on addba timer expiry or if the addba request is declined, the state is changed to HT_AGG_STATE_REQ_STOP_BA_MSK |(initiator <<
HT_AGG_STATE_INITIATOR_SHIFT) in ___ieee80211_stop_tx_ba_session. 
After commit 416fbdff2137e8d8cc8f23f517bee3a26b11526f, the ampdu state needs to have HT_ADDBA_REQUESTED_MSK set, else the skb's are not spliced.
Since the ampdu state got changed in ___ieee80211_stop_tx_ba_session, this condition is not met due to which the skb's are not spliced.
tid_tx[tid] which has a pointer to the pending skb queue then gets freed leaving the skb's in the pending queue allocated forever resulting in a memory leak. Does this make sense? If yes, one way to fix the issue is modify the state in ___ieee80211_stop_tx_ba_session preserving the earlier state. This way HT_ADDBA_REQUESTED_MSK will be set and skb's will be spliced. Any other way to fix this issue? Thoughts?


Regards,
Nishant

Re: Possible leak in the ampdu aggregation code?

[Johannes Berg]

On Wed, 2010-06-09 at 01:35 -0700, Nishant Sarmukadam wrote:

> I wanted to get some views on a possible issue while using mac80211
> ampdu support. 
> skb's from pending queue for a TID are spliced onto the local pending
> queue when tearing down a block ack session.

Right.

> If aggregation is stopped before the ampdu state becomes
> HT_AGG_STATE_OPERATIONAL say on addba timer expiry or if the addba
> request is declined, the state is changed to
> HT_AGG_STATE_REQ_STOP_BA_MSK |(initiator <<
> HT_AGG_STATE_INITIATOR_SHIFT) in ___ieee80211_stop_tx_ba_session. 

Right.

> After commit 416fbdff2137e8d8cc8f23f517bee3a26b11526f, the ampdu state
> needs to have HT_ADDBA_REQUESTED_MSK set, else the skb's are not
> spliced.

Bleh. That commit is bogus. I didn't even know it existed. It looks like
that commit was trying to fix the issue fixed by
827d42c9ac91ddd728e4f4a31fefb906ef2ceff7 but ended up just hiding it
enough that we didn't really find the cause of issue for a long time
still... we shouldn't even have been stopping the aggregation session
when it was never started!!

I think that commit should just be reverted.

johannes

RE: Possible leak in the ampdu aggregation code?

[Nishant Sarmukadam]

> I think that commit should just be reverted.

Ok, I will send a patch to revert commit 416fbdff2137e8d8cc8f23f517bee3a26b11526f.

Nishant

-----Original Message-----
From: Johannes Berg [mailto:johannes@sipsolutions.net] 
Sent: Wednesday, June 09, 2010 5:09 PM
To: Nishant Sarmukadam
Cc: linux-wireless@vger.kernel.org; Luis R. Rodriguez
Subject: Re: Possible leak in the ampdu aggregation code?

On Wed, 2010-06-09 at 01:35 -0700, Nishant Sarmukadam wrote:

> I wanted to get some views on a possible issue while using mac80211
> ampdu support. 
> skb's from pending queue for a TID are spliced onto the local pending
> queue when tearing down a block ack session.

Right.

> If aggregation is stopped before the ampdu state becomes
> HT_AGG_STATE_OPERATIONAL say on addba timer expiry or if the addba
> request is declined, the state is changed to
> HT_AGG_STATE_REQ_STOP_BA_MSK |(initiator <<
> HT_AGG_STATE_INITIATOR_SHIFT) in ___ieee80211_stop_tx_ba_session. 

Right.

> After commit 416fbdff2137e8d8cc8f23f517bee3a26b11526f, the ampdu state
> needs to have HT_ADDBA_REQUESTED_MSK set, else the skb's are not
> spliced.

Bleh. That commit is bogus. I didn't even know it existed. It looks like
that commit was trying to fix the issue fixed by
827d42c9ac91ddd728e4f4a31fefb906ef2ceff7 but ended up just hiding it
enough that we didn't really find the cause of issue for a long time
still... we shouldn't even have been stopping the aggregation session
when it was never started!!

I think that commit should just be reverted.

johannes