> -----Original Message----- > From: Victor Nogueira <[email protected]> > Sent: Thursday, April 16, 2026 4:27 PM > To: Chia-Yu Chang (Nokia) <[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 v1 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 13/04/2026 13:37, [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.s 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. > > [...] > > --- > > net/sched/sch_dualpi2.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c index > > 6d7e6389758d..56d4422970b6 100644 > > --- a/net/sched/sch_dualpi2.c > > +++ b/net/sched/sch_dualpi2.c > > @@ -872,11 +872,25 @@ static int dualpi2_change(struct Qdisc *sch, struct > > nlattr *opt, > > old_backlog = sch->qstats.backlog; > > while (qdisc_qlen(sch) > sch->limit || > > q->memory_used > q->memory_limit) { > > - struct sk_buff *skb = qdisc_dequeue_internal(sch, true); > > - > > - q->memory_used -= skb->truesize; > > - qdisc_qstats_backlog_dec(sch, skb); > > - rtnl_qdisc_drop(skb, sch); > > + int c_len = qdisc_qlen(sch) - qdisc_qlen(q->l_queue); > > + struct sk_buff *skb = NULL; > > + > > + if (c_len) { > > + 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; > > + q->memory_used -= skb->truesize; > > + rtnl_qdisc_drop(skb, q->l_queue); > > + /* Keep the overall qdisc stats consistent */ > > + --sch->q.qlen; > > + qdisc_qstats_backlog_dec(sch, skb); > > Sashiko is hallucinating saying this will cause a UAF, it won't. > However it is good to maintain a consistent order here. > For example, see how sch_choke is doing [1]. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/net/sched/sch_choke.c?id=1f5ffc672165ff851063a5fd044b727ab2517ae3#n394 > > cheers, > Victor
Hi Victor, Thanks for the pointer to sch_choke, it follows the order: (1) qdisc_qstats_backlog_dec(), (2) reduce qlen, and (3) rtnl_qdisc_drop(). But I've also checked sch_codel, its order is: (1) reduce qlen, (2) qdisc_qstats_backlog_dec(), and (3) rtnl_qdisc_drop(). So, the key is to place rtnl_qdisc_drop() after the reduction of qstats_backlog as well as qlen. Then, I will follow the same order for dualpi2 in next version: 1. qdisc_dequeue_internal(q->l_queue), including (a) --q->l_queue->q.qlen, and (2) qdisc_qstats_backlog_dec(q->l_queue) 2. --sch->q.qlen 3. qdisc_qstats_backlog_dec(sch) 4. rtnl_qdisc_drop(skb, q->l_queue), which will do "qdisc_qstats_drop(q->l_queue)" 5. qdisc_qstats_drop(sch) Thanks, Chia-Yu
