netdev - Re: [PATCH net v4 1/2] net/sched: Fix backlog accounting in qdisc_dequeue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250812073802.28f86ab2@kernel.org>
Date: Tue, 12 Aug 2025 07:38:02 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: William Liu <will@...lsroot.io>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
 pabeni@...hat.com, jiri@...nulli.us, davem@...emloft.net,
 edumazet@...gle.com, horms@...nel.org, savy@...t3mfailure.io,
 victor@...atatu.com
Subject: Re: [PATCH net v4 1/2] net/sched: Fix backlog accounting in
 qdisc_dequeue_internal

On Tue, 12 Aug 2025 02:10:02 +0000 William Liu wrote:
> > AFAICT only if the backlog adjustment is using the prev_qlen,
> > prev_backlog approach, which snapshots the backlog. In that case,
> > yes, the "internal drops" will mess up the count.  
> 
> Yep, that's why I added the dropped_qlen and dropped_backlog
> variables, though that is not a very pretty solution.
> 
> But even looking at the method you suggested (copy pasting for
> reference):
> 
> 	pkts = 0;
> 	bytes = 0;
>  	while (sch->q.qlen > sch->limit ||
>  	       q->memory_usage > q->memory_limit) {
>  		struct sk_buff *skb = qdisc_dequeue_internal(sch, false); 
> 		pkts++;
> 		bytes += qdisc_pkt_len(skb);
>  		rtnl_kfree_skbs(skb, skb);
>  	}
> 	qdisc_tree_reduce_backlog(sch, pkts, bytes);
> 
> qdisc_dequeue_internal can trigger fq_codel_dequeue, which can
> trigger qdisc_tree_reduce_backlog before returning (the only qdisc
> out of these that does so in its dequeue handler). 
> 
> Let's say the limit only goes down by one, and packet A is at the
> front of the queue. qdisc_dequeue_internal takes the dequeue path,
> and fq_codel_dequeue triggers a qdisc_tree_reduce_backlog from that
> packet before returning the skb. Would this final
> qdisc_tree_reduce_backlog after the limit drop not double count? 

The packets that got counted in qdisc_tree_reduce_backlog() inside
->dequeue are freed immediately via

  drop_func()
    kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_CONGESTED);

in the scenario you're describing ->dequeue should return NULL.
If that's possible, then we have another bug here :$

Normally backlogs get adjusted as the packet travels down the hierarchy
thru the parent chain. ->dequeue is part of this normal path so skbs it
returns are still in the parent's backlogs. qdisc_tree_reduce_backlog()
is only called when we need to do something to an skb outside of the
normal ->enqueue/->dequeue flow that iterates the hierarchy.