>
> +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.
Yeah - I had the if (old) patch internally - just 1 hour before you sent yours :)
Thing is that I don't understand how it even helps since you lock only on side of the path? At least for u-apsd / ps-poll deliver I don't see how that'd help?
I mean that uapsd / ps-poll code dequeues packets without taking the lock.
>
> 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
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
