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
--
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
