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

[Johannes Berg <johannes@sipsolutions.net>]

+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