Re: [PATCH net] net/sched: sch_dualpi2: avoid double memory accounting on peek

[Simon Horman <horms@kernel.org>]

On Mon, Nov 09, 2026 at 04:36:55PM +0000, shaos wrote:
> qdisc_dequeue_internal() removes skbs from gso_skb without invoking
> the qdisc dequeue callback. Their truesize was already subtracted
> when they were first dequeued for peek or requeued after a transmit
> failure. dualpi2_change() currently subtracts it again while trimming
> the queue, wrapping the u32 memory_used counter.
> 
> Skip the second decrement when the skb comes from gso_skb. Packets
> dequeued directly from the C-queue still need the accounting here;
> the L-queue path is unchanged.
> 
> The issue was identified by LLM-assisted source analysis and
> reproduced with a purpose-built rtnetlink/AF_PACKET test in QEMU.
> The test uses CAP_NET_ADMIN in a user-created network namespace and
> CAP_NET_RAW for its AF_PACKET sender. It does not modify the kernel
> or use error injection.
> 
> On Linux 7.4-rc2, the unpatched reproducer wrapped memory_used to
> 4294964992 and subsequent packets remained rejected after restoring
> the memory limit. With this patch, the counter remains valid and a
> packet is accepted after restoring the limit.
> 
> The modified sch_dualpi2.o builds with W=1 under allmodconfig and
> allyesconfig. Full-tree W=1 builds stop on unrelated warnings in
> lib/tests/stackinit_kunit.c, arch/x86/kernel/callthunks.c, and
> net/8021q/vlan.c. The QEMU regression passes; tc-testing was not run.
> 
> Fixes: 8f9516daedd6 ("sched: Add enqueue/dequeue of dualpi2 qdisc")
> Link: https://lkml.iu.edu/hypermail/linux/kernel/2610.2/16461.html
> Signed-off-by: shaos <shawnq1@163.com>
> ---
>  net/sched/sch_dualpi2.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
> index 4947def7c49ef..ccf6359eeebc3 100644
> --- a/net/sched/sch_dualpi2.c
> +++ b/net/sched/sch_dualpi2.c
> @@ -909,14 +909,20 @@ static int dualpi2_change(struct Qdisc *sch, struct nlattr *opt,
>  	while (qdisc_qlen(sch) > sch->limit ||
>  	       q->memory_used > q->memory_limit) {
>  		struct sk_buff *skb = NULL;
> +		bool already_dequeued;
>  
>  		if (qdisc_qlen(sch) > qdisc_qlen(q->l_queue)) {
> +			already_dequeued = skb_peek(&sch->gso_skb);
> +
>  			skb = qdisc_dequeue_internal(sch, true);
>  			if (unlikely(!skb)) {
>  				WARN_ON_ONCE(1);
>  				break;
>  			}
> -			WRITE_ONCE(q->memory_used, q->memory_used - skb->truesize);
> +			/* A peeked or requeued skb was already accounted. */
> +			if (!already_dequeued)
> +				WRITE_ONCE(q->memory_used,
> +					   q->memory_used - skb->truesize);

It strikes me that this change is incomplete.

I think the loads and stores of q->memory_used and q->memory_limit
should be consistently annotated, stashing the value of reads
in local variables to avoid re-reads as appropriate.

Perhaps the resulting change is too wide to be a strict fix for the bug
at hand. But that does at least seem necessary within this function.

>  			rtnl_qdisc_drop(skb, sch);
>  		} else if (qdisc_qlen(q->l_queue)) {
>  			skb = qdisc_dequeue_internal(q->l_queue, true);
> 

-- 
pw-bot: changes-requested