[PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Stanislaw Gruszka]

As we check sta->ps_tx_buf[ac] length without lock, we can get not
updated value. In the worst scenario queue could be currently empty
and we can see length bigger than STA_MAX_TX_BUFFER. When this happen
skb_dequeue() will return NULL and we will crash trying to free NULL
skb:

[ 6335.697678] BUG: unable to handle kernel NULL pointer dereference at 000000ac
[ 6335.697858] IP: [<fefcdc70>] ieee80211_report_used_skb+0x10/0x1e0 [mac80211]
...
[ 6335.700223] Call Trace:
[ 6335.700599]  [<fefcde55>] ieee80211_free_txskb+0x15/0x20 [mac80211]
[ 6335.700697]  [<fefef242>] invoke_tx_handlers+0x1322/0x1370 [mac80211]
[ 6335.700787]  [<c13551b6>] ? dev_hard_start_xmit+0x2b6/0x510
[ 6335.700879]  [<fefef3fb>] ? ieee80211_tx_prepare+0x16b/0x330 [mac80211]
[ 6335.700981]  [<fefef626>] ieee80211_tx+0x66/0xe0 [mac80211]
[ 6335.701072]  [<fefeff43>] ieee80211_xmit+0x93/0xf0 [mac80211]
[ 6335.701163]  [<feff0d45>] ieee80211_subif_start_xmit+0xab5/0xbc0 [mac80211]
[ 6335.701258]  [<c13551b6>] dev_hard_start_xmit+0x2b6/0x510

To fix this problem, recheck queue length with lock taken.

Bug report:
http://bugzilla.kernel.org/show_bug.cgi?id=70551#c11

Reported-and-tested-by: Max Sydorenko <maxim.stargazer@gmail.com>
Fixes: c3e7724b6b ("mac80211: use ieee80211_free_txskb to fix possible skb leaks")
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 net/mac80211/tx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 722151f..85b9b8e 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -472,17 +472,25 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
 		      test_sta_flag(sta, WLAN_STA_PS_DRIVER)) &&
 		     !(info->flags & IEEE80211_TX_CTL_NO_PS_BUFFER))) {
 		int ac = skb_get_queue_mapping(tx->skb);
+		struct sk_buff *old_skb = NULL;
+		unsigned long flags;
 
 		ps_dbg(sta->sdata, "STA %pM aid %d: PS buffer for AC %d\n",
 		       sta->sta.addr, sta->sta.aid, ac);
 		if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
 			purge_old_ps_buffers(tx->local);
 		if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
-			struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
+			spin_lock_irqsave(&sta->ps_tx_buf[ac].lock, flags);
+			/* queue could be modified, recheck length with lock taken */
+			if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER)
+				old_skb = __skb_dequeue(&sta->ps_tx_buf[ac]);
+			spin_unlock_irqrestore(&sta->ps_tx_buf[ac].lock, flags);
+		}
+		if (old_skb) {
 			ps_dbg(tx->sdata,
 			       "STA %pM TX buffer for AC %d full - dropping oldest frame\n",
 			       sta->sta.addr, ac);
-			ieee80211_free_txskb(&local->hw, old);
+			ieee80211_free_txskb(&local->hw, old_skb);
 		} else
 			tx->local->total_ps_buffered++;
 
-- 
1.7.11.7

[PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Stanislaw Gruszka]

Similar change as on current patch "mac80211: fix calling
ieee80211_free_txskb with NULL skb", but for multicast queue. Patch does
not prevent crash, as dev_kfree_skb() checks against NULL skb, but it
help to prevent not necessary frame drop, when bc_buf queue was
partially flushed and no longer exceeds AP_MAX_BC_BUFFER .

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 net/mac80211/tx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 85b9b8e..78bca8a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -387,6 +387,8 @@ ieee80211_tx_h_multicast_ps_buf(struct ieee80211_tx_data *tx)
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
 	struct ps_data *ps;
+	struct sk_buff *old_skb = NULL;
+	unsigned long flags;
 
 	/*
 	 * broadcast/multicast frame
@@ -432,6 +434,13 @@ ieee80211_tx_h_multicast_ps_buf(struct ieee80211_tx_data *tx)
 		purge_old_ps_buffers(tx->local);
 
 	if (skb_queue_len(&ps->bc_buf) >= AP_MAX_BC_BUFFER) {
+		spin_lock_irqsave(&ps->bc_buf.lock, flags);
+		/* queue could be modified, recheck length with lock taken */
+		if (skb_queue_len(&ps->bc_buf) >= AP_MAX_BC_BUFFER)
+			old_skb = __skb_dequeue(&ps->bc_buf);
+		spin_unlock_irqrestore(&ps->bc_buf.lock, flags);
+	}
+	if (old_skb) {
 		ps_dbg(tx->sdata,
 		       "BC TX buffer full - dropping the oldest frame\n");
 		dev_kfree_skb(skb_dequeue(&ps->bc_buf));
-- 
1.7.11.7

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Johannes Berg]

On Wed, 2014-02-19 at 13:28 +0100, Stanislaw Gruszka wrote:
> Similar change as on current patch "mac80211: fix calling
> ieee80211_free_txskb with NULL skb", but for multicast queue. Patch does
> not prevent crash, as dev_kfree_skb() checks against NULL skb, but it
> help to prevent not necessary frame drop, when bc_buf queue was
> partially flushed and no longer exceeds AP_MAX_BC_BUFFER .

I don't think this makes sense. It doesn't really change anything,
holding a spinlock isn't something magic that makes other things go
away, so instead of

 * check queue length, is >= limit
 * free frame from queue, even if somebody else is dequeuing as well

as before, you'd just have
 * check queue length, is >= limit
 * take lock
 * check queue length, is >= limit
 * drop frame
 * unlock
 * somebody else who was dequeuing now wakes up from waiting on the lock
and
   finds no frame there

It ultimately makes no difference at all, it just makes this code more
difficult to read and understand.

johannes

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Stanislaw Gruszka]

On Wed, Feb 19, 2014 at 02:14:09PM +0100, Johannes Berg wrote:
> On Wed, 2014-02-19 at 13:28 +0100, Stanislaw Gruszka wrote:
> > Similar change as on current patch "mac80211: fix calling
> > ieee80211_free_txskb with NULL skb", but for multicast queue. Patch does
> > not prevent crash, as dev_kfree_skb() checks against NULL skb, but it
> > help to prevent not necessary frame drop, when bc_buf queue was
> > partially flushed and no longer exceeds AP_MAX_BC_BUFFER .
> 
> I don't think this makes sense. It doesn't really change anything,
> holding a spinlock isn't something magic that makes other things go
> away, so instead of
>
>  * check queue length, is >= limit
>  * free frame from queue, even if somebody else is dequeuing as well
> 
> as before, you'd just have
>  * check queue length, is >= limit
>  * take lock
>  * check queue length, is >= limit
>  * drop frame
>  * unlock
>  * somebody else who was dequeuing now wakes up from waiting on the lock
> and
>    finds no frame there
> 
> It ultimately makes no difference at all, it just makes this code more
> difficult to read and understand.

It make difference when queue length value is modified on different CPU
and read on different CPU. Without lock you can 'see' old length value
on CPU that run ieee80211_tx_h_multicast_ps_buf() for undefined
period of time (ok maybe not undefined on x86), and current oldest
frame can be not necessarily dropped.

I can remove first call skb_queue_len(&ps->bc_buf) and take spinlock
unconditionally, will that help with code readability ? 

Stanislaw

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Johannes Berg]

On Wed, 2014-02-19 at 14:35 +0100, Stanislaw Gruszka wrote:

> > It ultimately makes no difference at all, it just makes this code more
> > difficult to read and understand.
> 
> It make difference when queue length value is modified on different CPU
> and read on different CPU. Without lock you can 'see' old length value
> on CPU that run ieee80211_tx_h_multicast_ps_buf() for undefined
> period of time (ok maybe not undefined on x86), and current oldest
> frame can be not necessarily dropped.

I don't see how that can be true, since the modifications of the queue
length are under spinlock with the implied memory barriers.

johannes

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Stanislaw Gruszka]

On Wed, Feb 19, 2014 at 03:51:43PM +0100, Johannes Berg wrote:
> On Wed, 2014-02-19 at 14:35 +0100, Stanislaw Gruszka wrote:
> 
> > > It ultimately makes no difference at all, it just makes this code more
> > > difficult to read and understand.
> > 
> > It make difference when queue length value is modified on different CPU
> > and read on different CPU. Without lock you can 'see' old length value
> > on CPU that run ieee80211_tx_h_multicast_ps_buf() for undefined
> > period of time (ok maybe not undefined on x86), and current oldest
> > frame can be not necessarily dropped.
> 
> I don't see how that can be true, since the modifications of the queue
> length are under spinlock with the implied memory barriers.

On processor that read value there is no memory barrier. If you do:

CPU1				CPU2

				b = a;
spin_lock(a_lock)
a = 1;				...
spin_unlock(a_lock)

				b = a;

There is nothing that guarantee that on CPU2 b will be 1.

Stanislaw

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Johannes Berg]

On Wed, 2014-02-19 at 16:09 +0100, Stanislaw Gruszka wrote:
> On Wed, Feb 19, 2014 at 03:51:43PM +0100, Johannes Berg wrote:
> > On Wed, 2014-02-19 at 14:35 +0100, Stanislaw Gruszka wrote:
> > 
> > > > It ultimately makes no difference at all, it just makes this code more
> > > > difficult to read and understand.
> > > 
> > > It make difference when queue length value is modified on different CPU
> > > and read on different CPU. Without lock you can 'see' old length value
> > > on CPU that run ieee80211_tx_h_multicast_ps_buf() for undefined
> > > period of time (ok maybe not undefined on x86), and current oldest
> > > frame can be not necessarily dropped.
> > 
> > I don't see how that can be true, since the modifications of the queue
> > length are under spinlock with the implied memory barriers.
> 
> On processor that read value there is no memory barrier. If you do:
> 
> CPU1				CPU2
> 
> 				b = a;
> spin_lock(a_lock)
> a = 1;				...
> spin_unlock(a_lock)
> 
> 				b = a;
> 
> There is nothing that guarantee that on CPU2 b will be 1.

Well, OK, so there's some small window of time where the drop code isn't
perfect. Does it matter? I'd argue it doesn't - if you're getting close
to the drop case then the client is probably misbehaving anyway.

johannes

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Stanislaw Gruszka]

On Wed, Feb 19, 2014 at 05:36:58PM +0100, Johannes Berg wrote:
> Well, OK, so there's some small window of time where the drop code isn't
> perfect. Does it matter? I'd argue it doesn't - if you're getting close
> to the drop case then the client is probably misbehaving anyway.

Ok, let drop those patches. Do you want me to repost first patch with
just if(old) check to fix kaboom, or you prefer to problem be solved
properly at once ?

Stanislaw  

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Johannes Berg]

On Thu, 2014-02-20 at 08:56 +0100, Stanislaw Gruszka wrote:
> On Wed, Feb 19, 2014 at 05:36:58PM +0100, Johannes Berg wrote:
> > Well, OK, so there's some small window of time where the drop code isn't
> > perfect. Does it matter? I'd argue it doesn't - if you're getting close
> > to the drop case then the client is probably misbehaving anyway.
> 
> Ok, let drop those patches. Do you want me to repost first patch with
> just if(old) check to fix kaboom, or you prefer to problem be solved
> properly at once ?

Emmanuel posted his patch, so I think I'll just pick that up. Care to
review it?

johannes

Re: [PATCH 2/2] mac80211: protect skb_queue_len(&ps->bc_buf) by lock

[Stanislaw Gruszka]

On Thu, Feb 20, 2014 at 08:56:39AM +0100, Stanislaw Gruszka wrote:
> On Wed, Feb 19, 2014 at 05:36:58PM +0100, Johannes Berg wrote:
> > Well, OK, so there's some small window of time where the drop code isn't
> > perfect. Does it matter? I'd argue it doesn't - if you're getting close
> > to the drop case then the client is probably misbehaving anyway.
> 
> Ok, let drop those patches. Do you want me to repost first patch with
> just if(old) check to fix kaboom, or you prefer to problem be solved
> properly at once ?

Nevermind, I just saw Emmanuel posted proper patch.

Stanislaw

Re: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Johannes Berg]

+Emmanuel

Interesting. We just ran into the same issue as well.

> +		struct sk_buff *old_skb = NULL;
> +		unsigned long flags;
>  
>  		ps_dbg(sta->sdata, "STA %pM aid %d: PS buffer for AC %d\n",
>  		       sta->sta.addr, sta->sta.aid, ac);
>  		if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
>  			purge_old_ps_buffers(tx->local);
>  		if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
> -			struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
> +			spin_lock_irqsave(&sta->ps_tx_buf[ac].lock, flags);
> +			/* queue could be modified, recheck length with lock taken */
> +			if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER)
> +				old_skb = __skb_dequeue(&sta->ps_tx_buf[ac]);
> +			spin_unlock_irqrestore(&sta->ps_tx_buf[ac].lock, flags);
> +		}
> +		if (old_skb) {

I think that's pointless, you can just say "if (old)" instead as the
dequeue would return NULL.

In any case, while this solves the crash which is a good thing, it still
leaves the code buggy. This crash seems to occur in the following racy
scenario:

 * station is sleeping
 * frame TX to station begins
 * station wakes up
 * frame TX goes into the queue length check, finds long queue
 * pending frames are transmitted
 * queue is now empty
 * old = skb_dequeue() returns NULL
 * *kaboom*

The problem is that you're just fixing the "*kaboom*" part, so the code
will continue like this:

 * old is NULL
 * no kaboom
 * new frame is queued on ps_tx_buf queue
 * frame never gets transmitted

johannes

RE: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Grumbach, Emmanuel]

PiANCj4gK0VtbWFudWVsDQo+IA0KPiBJbnRlcmVzdGluZy4gV2UganVzdCByYW4gaW50byB0aGUg
c2FtZSBpc3N1ZSBhcyB3ZWxsLg0KPiANCj4gPiArCQlzdHJ1Y3Qgc2tfYnVmZiAqb2xkX3NrYiA9
IE5VTEw7DQo+ID4gKwkJdW5zaWduZWQgbG9uZyBmbGFnczsNCj4gPg0KPiA+ICAJCXBzX2RiZyhz
dGEtPnNkYXRhLCAiU1RBICVwTSBhaWQgJWQ6IFBTIGJ1ZmZlciBmb3IgQUMNCj4gJWRcbiIsDQo+
ID4gIAkJICAgICAgIHN0YS0+c3RhLmFkZHIsIHN0YS0+c3RhLmFpZCwgYWMpOw0KPiA+ICAJCWlm
ICh0eC0+bG9jYWwtPnRvdGFsX3BzX2J1ZmZlcmVkID49DQo+IFRPVEFMX01BWF9UWF9CVUZGRVIp
DQo+ID4gIAkJCXB1cmdlX29sZF9wc19idWZmZXJzKHR4LT5sb2NhbCk7DQo+ID4gIAkJaWYgKHNr
Yl9xdWV1ZV9sZW4oJnN0YS0+cHNfdHhfYnVmW2FjXSkgPj0NCj4gU1RBX01BWF9UWF9CVUZGRVIp
IHsNCj4gPiAtCQkJc3RydWN0IHNrX2J1ZmYgKm9sZCA9IHNrYl9kZXF1ZXVlKCZzdGEtDQo+ID5w
c190eF9idWZbYWNdKTsNCj4gPiArCQkJc3Bpbl9sb2NrX2lycXNhdmUoJnN0YS0+cHNfdHhfYnVm
W2FjXS5sb2NrLCBmbGFncyk7DQo+ID4gKwkJCS8qIHF1ZXVlIGNvdWxkIGJlIG1vZGlmaWVkLCBy
ZWNoZWNrIGxlbmd0aCB3aXRoIGxvY2sNCj4gdGFrZW4gKi8NCj4gPiArCQkJaWYgKHNrYl9xdWV1
ZV9sZW4oJnN0YS0+cHNfdHhfYnVmW2FjXSkgPj0NCj4gU1RBX01BWF9UWF9CVUZGRVIpDQo+ID4g
KwkJCQlvbGRfc2tiID0gX19za2JfZGVxdWV1ZSgmc3RhLQ0KPiA+cHNfdHhfYnVmW2FjXSk7DQo+
ID4gKwkJCXNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJnN0YS0+cHNfdHhfYnVmW2FjXS5sb2NrLA0K
PiBmbGFncyk7DQo+ID4gKwkJfQ0KPiA+ICsJCWlmIChvbGRfc2tiKSB7DQo+IA0KPiBJIHRoaW5r
IHRoYXQncyBwb2ludGxlc3MsIHlvdSBjYW4ganVzdCBzYXkgImlmIChvbGQpIiBpbnN0ZWFkIGFz
IHRoZSBkZXF1ZXVlDQo+IHdvdWxkIHJldHVybiBOVUxMLg0KDQpZZWFoIC0gSSBoYWQgdGhlIGlm
IChvbGQpIHBhdGNoIGludGVybmFsbHkgLSBqdXN0IDEgaG91ciBiZWZvcmUgeW91IHNlbnQgeW91
cnMgOikNCg0KVGhpbmcgaXMgdGhhdCBJIGRvbid0IHVuZGVyc3RhbmQgaG93IGl0IGV2ZW4gaGVs
cHMgc2luY2UgeW91IGxvY2sgb25seSBvbiBzaWRlIG9mIHRoZSBwYXRoPyBBdCBsZWFzdCBmb3Ig
dS1hcHNkIC8gcHMtcG9sbCBkZWxpdmVyIEkgZG9uJ3Qgc2VlIGhvdyB0aGF0J2QgaGVscD8NCkkg
bWVhbiB0aGF0IHVhcHNkIC8gcHMtcG9sbCBjb2RlIGRlcXVldWVzIHBhY2tldHMgd2l0aG91dCB0
YWtpbmcgdGhlIGxvY2suDQoNCg0KPiANCj4gSW4gYW55IGNhc2UsIHdoaWxlIHRoaXMgc29sdmVz
IHRoZSBjcmFzaCB3aGljaCBpcyBhIGdvb2QgdGhpbmcsIGl0IHN0aWxsIGxlYXZlcyB0aGUNCj4g
Y29kZSBidWdneS4gVGhpcyBjcmFzaCBzZWVtcyB0byBvY2N1ciBpbiB0aGUgZm9sbG93aW5nIHJh
Y3kNCj4gc2NlbmFyaW86DQo+IA0KPiAgKiBzdGF0aW9uIGlzIHNsZWVwaW5nDQo+ICAqIGZyYW1l
IFRYIHRvIHN0YXRpb24gYmVnaW5zDQo+ICAqIHN0YXRpb24gd2FrZXMgdXANCj4gICogZnJhbWUg
VFggZ29lcyBpbnRvIHRoZSBxdWV1ZSBsZW5ndGggY2hlY2ssIGZpbmRzIGxvbmcgcXVldWUNCj4g
ICogcGVuZGluZyBmcmFtZXMgYXJlIHRyYW5zbWl0dGVkDQo+ICAqIHF1ZXVlIGlzIG5vdyBlbXB0
eQ0KPiAgKiBvbGQgPSBza2JfZGVxdWV1ZSgpIHJldHVybnMgTlVMTA0KPiAgKiAqa2Fib29tKg0K
PiANCj4gVGhlIHByb2JsZW0gaXMgdGhhdCB5b3UncmUganVzdCBmaXhpbmcgdGhlICIqa2Fib29t
KiIgcGFydCwgc28gdGhlIGNvZGUgd2lsbA0KPiBjb250aW51ZSBsaWtlIHRoaXM6DQo+IA0KPiAg
KiBvbGQgaXMgTlVMTA0KPiAgKiBubyBrYWJvb20NCj4gICogbmV3IGZyYW1lIGlzIHF1ZXVlZCBv
biBwc190eF9idWYgcXVldWUNCj4gICogZnJhbWUgbmV2ZXIgZ2V0cyB0cmFuc21pdHRlZA0KPiAN
Cj4gam9oYW5uZXMNCg0K

RE: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Grumbach, Emmanuel]

PiA+DQo+ID4gK0VtbWFudWVsDQo+ID4NCj4gPiBJbnRlcmVzdGluZy4gV2UganVzdCByYW4gaW50
byB0aGUgc2FtZSBpc3N1ZSBhcyB3ZWxsLg0KPiA+DQo+ID4gPiArCQlzdHJ1Y3Qgc2tfYnVmZiAq
b2xkX3NrYiA9IE5VTEw7DQo+ID4gPiArCQl1bnNpZ25lZCBsb25nIGZsYWdzOw0KPiA+ID4NCj4g
PiA+ICAJCXBzX2RiZyhzdGEtPnNkYXRhLCAiU1RBICVwTSBhaWQgJWQ6IFBTIGJ1ZmZlciBmb3Ig
QUMNCj4gPiAlZFxuIiwNCj4gPiA+ICAJCSAgICAgICBzdGEtPnN0YS5hZGRyLCBzdGEtPnN0YS5h
aWQsIGFjKTsNCj4gPiA+ICAJCWlmICh0eC0+bG9jYWwtPnRvdGFsX3BzX2J1ZmZlcmVkID49DQo+
ID4gVE9UQUxfTUFYX1RYX0JVRkZFUikNCj4gPiA+ICAJCQlwdXJnZV9vbGRfcHNfYnVmZmVycyh0
eC0+bG9jYWwpOw0KPiA+ID4gIAkJaWYgKHNrYl9xdWV1ZV9sZW4oJnN0YS0+cHNfdHhfYnVmW2Fj
XSkgPj0NCj4gPiBTVEFfTUFYX1RYX0JVRkZFUikgew0KPiA+ID4gLQkJCXN0cnVjdCBza19idWZm
ICpvbGQgPSBza2JfZGVxdWV1ZSgmc3RhLQ0KPiA+ID5wc190eF9idWZbYWNdKTsNCj4gPiA+ICsJ
CQlzcGluX2xvY2tfaXJxc2F2ZSgmc3RhLT5wc190eF9idWZbYWNdLmxvY2ssIGZsYWdzKTsNCj4g
PiA+ICsJCQkvKiBxdWV1ZSBjb3VsZCBiZSBtb2RpZmllZCwgcmVjaGVjayBsZW5ndGggd2l0aCBs
b2NrDQo+ID4gdGFrZW4gKi8NCj4gPiA+ICsJCQlpZiAoc2tiX3F1ZXVlX2xlbigmc3RhLT5wc190
eF9idWZbYWNdKSA+PQ0KPiA+IFNUQV9NQVhfVFhfQlVGRkVSKQ0KPiA+ID4gKwkJCQlvbGRfc2ti
ID0gX19za2JfZGVxdWV1ZSgmc3RhLQ0KPiA+ID5wc190eF9idWZbYWNdKTsNCj4gPiA+ICsJCQlz
cGluX3VubG9ja19pcnFyZXN0b3JlKCZzdGEtPnBzX3R4X2J1ZlthY10ubG9jaywNCj4gPiBmbGFn
cyk7DQo+ID4gPiArCQl9DQo+ID4gPiArCQlpZiAob2xkX3NrYikgew0KPiA+DQo+ID4gSSB0aGlu
ayB0aGF0J3MgcG9pbnRsZXNzLCB5b3UgY2FuIGp1c3Qgc2F5ICJpZiAob2xkKSIgaW5zdGVhZCBh
cyB0aGUNCj4gPiBkZXF1ZXVlIHdvdWxkIHJldHVybiBOVUxMLg0KPiANCj4gWWVhaCAtIEkgaGFk
IHRoZSBpZiAob2xkKSBwYXRjaCBpbnRlcm5hbGx5IC0ganVzdCAxIGhvdXIgYmVmb3JlIHlvdSBz
ZW50IHlvdXJzIDopDQo+IA0KPiBUaGluZyBpcyB0aGF0IEkgZG9uJ3QgdW5kZXJzdGFuZCBob3cg
aXQgZXZlbiBoZWxwcyBzaW5jZSB5b3UgbG9jayBvbmx5IG9uIHNpZGUNCj4gb2YgdGhlIHBhdGg/
IEF0IGxlYXN0IGZvciB1LWFwc2QgLyBwcy1wb2xsIGRlbGl2ZXIgSSBkb24ndCBzZWUgaG93IHRo
YXQnZCBoZWxwPw0KPiBJIG1lYW4gdGhhdCB1YXBzZCAvIHBzLXBvbGwgY29kZSBkZXF1ZXVlcyBw
YWNrZXRzIHdpdGhvdXQgdGFraW5nIHRoZSBsb2NrLg0KPiANCg0KT2ggd2VsbCAtIHNvcnJ5LiBJ
IG92ZXJsb29rZWQgeW91ciBwYXRjaCBhbmQgZGlkbid0IG5vdGljZSB0aGF0IHlvdSB0b29rIHRo
ZSBza2JfaGVhZF9saXN0ICppbnRlcm5hbCogbG9jay4NCg0KPiANCj4gPg0KPiA+IEluIGFueSBj
YXNlLCB3aGlsZSB0aGlzIHNvbHZlcyB0aGUgY3Jhc2ggd2hpY2ggaXMgYSBnb29kIHRoaW5nLCBp
dA0KPiA+IHN0aWxsIGxlYXZlcyB0aGUgY29kZSBidWdneS4gVGhpcyBjcmFzaCBzZWVtcyB0byBv
Y2N1ciBpbiB0aGUNCj4gPiBmb2xsb3dpbmcgcmFjeQ0KPiA+IHNjZW5hcmlvOg0KPiA+DQo+ID4g
ICogc3RhdGlvbiBpcyBzbGVlcGluZw0KPiA+ICAqIGZyYW1lIFRYIHRvIHN0YXRpb24gYmVnaW5z
DQo+ID4gICogc3RhdGlvbiB3YWtlcyB1cA0KPiA+ICAqIGZyYW1lIFRYIGdvZXMgaW50byB0aGUg
cXVldWUgbGVuZ3RoIGNoZWNrLCBmaW5kcyBsb25nIHF1ZXVlDQo+ID4gICogcGVuZGluZyBmcmFt
ZXMgYXJlIHRyYW5zbWl0dGVkDQo+ID4gICogcXVldWUgaXMgbm93IGVtcHR5DQo+ID4gICogb2xk
ID0gc2tiX2RlcXVldWUoKSByZXR1cm5zIE5VTEwNCj4gPiAgKiAqa2Fib29tKg0KPiA+DQo+ID4g
VGhlIHByb2JsZW0gaXMgdGhhdCB5b3UncmUganVzdCBmaXhpbmcgdGhlICIqa2Fib29tKiIgcGFy
dCwgc28gdGhlDQo+ID4gY29kZSB3aWxsIGNvbnRpbnVlIGxpa2UgdGhpczoNCj4gPg0KPiA+ICAq
IG9sZCBpcyBOVUxMDQo+ID4gICogbm8ga2Fib29tDQo+ID4gICogbmV3IGZyYW1lIGlzIHF1ZXVl
ZCBvbiBwc190eF9idWYgcXVldWUNCj4gPiAgKiBmcmFtZSBuZXZlciBnZXRzIHRyYW5zbWl0dGVk
DQo+ID4NCj4gPiBqb2hhbm5lcw0KDQo=

Re: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Stanislaw Gruszka]

On Wed, Feb 19, 2014 at 01:33:38PM +0100, Johannes Berg wrote:
> +Emmanuel
> 
> Interesting. We just ran into the same issue as well.
> 
> > +		struct sk_buff *old_skb = NULL;
> > +		unsigned long flags;
> >  
> >  		ps_dbg(sta->sdata, "STA %pM aid %d: PS buffer for AC %d\n",
> >  		       sta->sta.addr, sta->sta.aid, ac);
> >  		if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
> >  			purge_old_ps_buffers(tx->local);
> >  		if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
> > -			struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
> > +			spin_lock_irqsave(&sta->ps_tx_buf[ac].lock, flags);
> > +			/* queue could be modified, recheck length with lock taken */
> > +			if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER)
> > +				old_skb = __skb_dequeue(&sta->ps_tx_buf[ac]);
> > +			spin_unlock_irqrestore(&sta->ps_tx_buf[ac].lock, flags);
> > +		}
> > +		if (old_skb) {
> 
> I think that's pointless, you can just say "if (old)" instead as the
> dequeue would return NULL.

I think it helps if queue was only partially empted, then we do not
drop oldest frame.

> In any case, while this solves the crash which is a good thing, it still
> leaves the code buggy. This crash seems to occur in the following racy
> scenario:
> 
>  * station is sleeping
>  * frame TX to station begins
>  * station wakes up
>  * frame TX goes into the queue length check, finds long queue
>  * pending frames are transmitted
>  * queue is now empty
>  * old = skb_dequeue() returns NULL
>  * *kaboom*
> 
> The problem is that you're just fixing the "*kaboom*" part, so the code
> will continue like this:
> 
>  * old is NULL
>  * no kaboom
>  * new frame is queued on ps_tx_buf queue
>  * frame never gets transmitted

When started to look at that code I found at least 3 bugs, but miss
this one :-)

Why frame will not be transmitted, we are disabling PS, but buffers
stays not empty ?

Stanislaw

Re: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Stanislaw Gruszka]

On Wed, Feb 19, 2014 at 02:21:35PM +0100, Stanislaw Gruszka wrote:
> > In any case, while this solves the crash which is a good thing, it still
> > leaves the code buggy. This crash seems to occur in the following racy
> > scenario:
> > 
> >  * station is sleeping
> >  * frame TX to station begins
> >  * station wakes up
> >  * frame TX goes into the queue length check, finds long queue
> >  * pending frames are transmitted
> >  * queue is now empty
> >  * old = skb_dequeue() returns NULL
> >  * *kaboom*
> > 
> > The problem is that you're just fixing the "*kaboom*" part, so the code
> > will continue like this:
> > 
> >  * old is NULL
> >  * no kaboom
> >  * new frame is queued on ps_tx_buf queue
> >  * frame never gets transmitted
> 
> When started to look at that code I found at least 3 bugs, but miss
> this one :-)
> 
> Why frame will not be transmitted, we are disabling PS, but buffers
> stays not empty ?

Ok, I think I see this, it seems to be race condition in 
ieee80211_sta_ps_deliver_wakeup().

Perhaps it could be solved by modifying
ieee80211_add_pending_skbs_fn() to take list of queues as argument,
that function seems properly stop queues, add buffered frames to
pending queue, clear WLAN_STA_PS_STA and then wake up queues. Or
just  stop using ieee80211_add_pending_skbs_fn() and do the same
sequence directly on ieee80211_sta_ps_deliver_wakeup() .
 
Stanislaw

Re: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Johannes Berg]

On Wed, 2014-02-19 at 15:48 +0100, Stanislaw Gruszka wrote:

> Ok, I think I see this, it seems to be race condition in 
> ieee80211_sta_ps_deliver_wakeup().
> 
> Perhaps it could be solved by modifying
> ieee80211_add_pending_skbs_fn() to take list of queues as argument,
> that function seems properly stop queues, add buffered frames to
> pending queue, clear WLAN_STA_PS_STA and then wake up queues. Or
> just  stop using ieee80211_add_pending_skbs_fn() and do the same
> sequence directly on ieee80211_sta_ps_deliver_wakeup() .

That might work, but we were considering just locking the sta->lock
around the whole "pending" thing inside deliver_wakeup() and in the "is
station sleeping" part. Then you also need to re-check ("is it still
sleeping?") inside the lock.

johannes

Re: [PATCH 1/2] mac80211: fix calling ieee80211_free_txskb with NULL skb

[Stanislaw Gruszka]

On Wed, Feb 19, 2014 at 03:50:14PM +0100, Johannes Berg wrote:
> On Wed, 2014-02-19 at 15:48 +0100, Stanislaw Gruszka wrote:
> 
> > Ok, I think I see this, it seems to be race condition in 
> > ieee80211_sta_ps_deliver_wakeup().
> > 
> > Perhaps it could be solved by modifying
> > ieee80211_add_pending_skbs_fn() to take list of queues as argument,
> > that function seems properly stop queues, add buffered frames to
> > pending queue, clear WLAN_STA_PS_STA and then wake up queues. Or
> > just  stop using ieee80211_add_pending_skbs_fn() and do the same
> > sequence directly on ieee80211_sta_ps_deliver_wakeup() .
> 
> That might work, but we were considering just locking the sta->lock
> around the whole "pending" thing inside deliver_wakeup() and in the "is
> station sleeping" part. Then you also need to re-check ("is it still
> sleeping?") inside the lock.

Looks good to me, feel free to post that.

Stanislaw