On Thu, 16 Apr 2026, Chia-Yu Chang (Nokia) wrote: > > -----Original Message----- > > From: Stephen Hemminger <[email protected]> > > Sent: Thursday, April 16, 2026 7:55 PM > > To: Chia-Yu Chang (Nokia) <[email protected]> > > Cc: [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; Koen De Schepper (Nokia) > > <[email protected]>; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected] > > Subject: Re: [PATCH v2 net 1/1] net/sched: sch_dualpi2: fix limit/memlimit > > enforcement when dequeueing L-queue > > > > > > CAUTION: This is an external email. Please be very careful when clicking > > links or opening attachments. See the URL nok.it/ext for additional > > information. > > > > > > > > On Thu, 16 Apr 2026 19:09:06 +0200 > > [email protected] wrote: > > > > > From: Chia-Yu Chang <[email protected]> > > > > > > Fix dualpi2_change() to correctly enforce updated limit and memlimit > > > values after a configuration change of the dualpi2 qdisc. > > > > > > Before this patch, dualpi2_change() always attempted to dequeue > > > packets via the root qdisc (C-queue) when reducing backlog or memory > > > usage, and unconditionally assumed that a valid skb will be returned. > > > When traffic classification results in packets being queued in the > > > L-queue while the C-queue is empty, this leads to a NULL skb > > > dereference during limit or memlimit enforcement. > > > > > > This is fixed by first dequeuing from the C-queue path if it is non-empty. > > > Once the C-queue is empty, packets are dequeued directly from the L-queue. > > > Return values from qdisc_dequeue_internal() are checked for both > > > queues. When dequeuing from the L-queue, the parent qdisc qlen and > > > backlog counters are updated explicitly to keep overall qdisc statistics > > > consistent. > > > > > > Fixes: 320d031ad6e4 ("sched: Struct definition and parsing of dualpi2 > > > qdisc") > > > Reported-by: "Kito Xu (veritas501)" <[email protected]> > > > Signed-off-by: Chia-Yu Chang <[email protected]> > > > --- > > > > I was a little concerned about the complexity of managing qlen here. > > But could not find anything obvious. > > Hi Stephen, > > This fix relies on some existing assmuptions of DualPI2. > > > > > Turned to AI review and it found some things: > > > > Right fix direction and the reported crash is real. A few issues before > > this is ready: > > > > 1. The `c_len` construction is fragile. Declared `int`, initialized from a > > `u32 - u32`. If the invariant `qdisc_qlen(sch) >= qdisc_qlen(q->l_queue)` > > is ever violated, you get a large positive value, the C-queue branch is > > taken on an empty C-queue, `qdisc_dequeue_internal()` returns NULL, and the > > loop breaks out without draining the L-queue -- leaving the qdisc over > > limit. Simpler and more robust to just compare the two qlens directly and > > drop the delta variable entirely. > > > > In current dequeue_packet() of DualPI2, we also calculate c_len via the same > approach (line 524). > > As we only have queue length of L-queue and both C- and L-queues, so this is > the way we derive the queue length of C-queue. > > > 2. Missing else/termination. If both branches' conditions are false > > (neither `c_len` nor `qdisc_qlen(q->l_queue)`) but the outer `while` > > still holds because `memory_used > memory_limit`, the loop spins > > forever. An explicit `else break;` guards against an accounting > > desync becoming a hang. > > This shall not happen, but adding an extra else guard indeed is > definitely a good suggestion.
Hi, Maybe also add WARN_ON_ONCE() there so that such a problem would be exposed if it ever happens. -- i.
