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. 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. 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. 3. Whitespace: two lines in the L-queue branch use spaces instead of tabs -- + q->memory_used -= skb->truesize; + rtnl_qdisc_drop(skb, q->l_queue); checkpatch will flag this. 4. Comment style. The three-line comment at the end of the L-queue branch doesn't follow the net subsystem multi-line comment style (leading ' * ' on continuation lines, closing ' */' on its own line). Once the code is cleaner, the comment could also just be dropped or shortened to one line. 5. The accounting in the L-queue branch is correct, but only if you trace the enqueue invariants carefully: L-queue packets are counted in *both* `sch` and `q->l_queue` on enqueue (see dualpi2_enqueue_skb lines 413-423), `qdisc_dequeue_internal(q->l_queue, true)` adjusts l_queue's side, and the explicit `--sch->q.qlen` + `qdisc_qstats_backlog_dec(sch, skb)` adjusts sch's side. Separately, the C-queue branch now quietly relies on the post-CVE-2025-39677 semantics of `qdisc_dequeue_internal()` handling parent backlog -- which is why the pre-patch `qdisc_qstats_backlog_dec(sch, skb)` could be removed. Neither of these load-bearing invariants is documented in the code or the commit message. Please add an inline comment in the L-queue branch explaining the double-count-on-enqueue, and mention the qdisc_dequeue_internal() dependency in the commit log. 6. Commit message / subject. Subject reads as if only the L-queue path changed, but the whole drain loop was restructured. Something like "sch_dualpi2: drain both C-queue and L-queue in dualpi2_change()" would describe it better. Also, on NULL return from qdisc_dequeue_internal() the loop silently breaks -- if that ever triggers it means qdisc_qlen() > 0 but dequeue returned NULL, which is a real invariant violation. > Worth a WARN_ON_ONCE(). Suggested shape: while (qdisc_qlen(sch) > sch->limit || q->memory_used > q->memory_limit) { struct sk_buff *skb; if (qdisc_qlen(sch) > qdisc_qlen(q->l_queue)) { skb = qdisc_dequeue_internal(sch, true); if (!skb) break; q->memory_used -= skb->truesize; rtnl_qdisc_drop(skb, sch); } else if (qdisc_qlen(q->l_queue)) { skb = qdisc_dequeue_internal(q->l_queue, true); if (!skb) break; /* L-queue packets are counted in both sch and * l_queue on enqueue; qdisc_dequeue_internal() * handled l_queue, account sch here. */ sch->q.qlen--; qdisc_qstats_backlog_dec(sch, skb); q->memory_used -= skb->truesize; rtnl_qdisc_drop(skb, q->l_queue); qdisc_qstats_drop(sch); } else { break; } } As with any AI feedback, expect it to generate hints but also be wrong.
