Patrick McHardy wrote:
quoted text
> Never mind, I found the reason. When enqueuing the packet, sfq_enqueue
> contains an off-by-one in the limit check (which IIRC is there for a
> reason, but I can't remember right now) and drops the packet again.
> dev_queue_xmit() calls qdisc_run() anyway and the empty qdisc is
> dequeued, which is not handled by SFQ.
> 
> I see three possibilities to fix this (in my preferred order):
> 
> 1) figure out why the off-by-one is there, if not needed remove
> 2) don't dequeue qdiscs even once if empty
> 3) check for NULL in sfq_dequeue
> 
> So I'll try to remeber why the off-by-one is there ..


OK the off-by-one prevents an out-of-bounds array access, which
would cause a crash itself. Despite what I said above, sfq does
try to handle dequeues while empty, but forgets to update q->tail
when dropping the last packet from the only active queue, probably
because it wasn't expected that the queue length is too small to
queue even a single packet (and that really doesn't make much sense).

So one possibility for fixing this is to update q->tail in sfq_drop
when dropping the last packet, but that would still leave the qdisc
non-functional because of the off-by-one. I chose a different way:
cap the limit at SFQ_DEPTH-1 and remove the off-by-one, which should
have no effect on the max (still 127), but prevents the crash since
we can now queue at least a single packet and q->tail is properly
updated in sfq_dequeue().

CCed Alexey just to be safe, but I think the patch should be fine.

Signed-off-by: Patrick McHardy <kaber@trash.net>