RE: [PATCH v2 net 1/1] net/sched: sch_dualpi2: fix limit/memlimit enforcement when dequeueing L-queue

["Ilpo Järvinen" <ij@kernel.org>]

On Thu, 16 Apr 2026, Chia-Yu Chang (Nokia) wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org> 
> > Sent: Thursday, April 16, 2026 7:55 PM
> > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > Cc: victor@mojatatu.com; hxzene@gmail.com; linux-hardening@vger.kernel.org; kees@kernel.org; gustavoars@kernel.org; jhs@mojatatu.com; jiri@resnulli.us; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; horms@kernel.org; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white@cablelabs.com; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel@apple.com
> > 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
> > chia-yu.chang@nokia-bell-labs.com wrote:
> > 
> > > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > >
> > > 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)" <hxzene@gmail.com>
> > > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > ---
> > 
> > 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.