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

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

[chia-yu.chang]

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")
Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
---
 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);
+		}
 	}
 	qdisc_tree_reduce_backlog(sch, old_qlen - qdisc_qlen(sch),
 				  old_backlog - sch->qstats.backlog);
-- 
2.34.1

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

[Paolo Abeni]

On 4/13/26 6:37 PM, 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")
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  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 says:
---
The drop counter is incremented for the L-queue via rtnl_qdisc_drop(),
but it appears the drop counter for the parent qdisc (sch) is not updated.
Will this cause user-facing statistics for the overall dualpi2 qdisc to
underreport drops?
---

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

[Chia-Yu Chang (Nokia)]

> -----Original Message-----
> From: Paolo Abeni <pabeni@redhat.com> 
> Sent: Thursday, April 16, 2026 3:26 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.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; 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 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 4/13/26 6:37 PM, 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")
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > ---
> >  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 says:
> ---
> The drop counter is incremented for the L-queue via rtnl_qdisc_drop(), but it appears the drop counter for the parent qdisc (sch) is not updated.
> Will this cause user-facing statistics for the overall dualpi2 qdisc to underreport drops?
> ---

Hi Paolo,

You are right, this is my miss.
I will add "qdisc_qstats_drop(sch)" for the L-queue dropping case.

Thanks!
Chia-Yu

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

[Victor Nogueira]

On 13/04/2026 13:37, 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.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

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

[Chia-Yu Chang (Nokia)]

> -----Original Message-----
> From: Victor Nogueira <victor@mojatatu.com>
> Sent: Thursday, April 16, 2026 4:27 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.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 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, 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.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