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] [day] [month] [year] [list]

Message-ID: <YycoAPI6tDiI1T8q-sQI6nAfIVk7E3AeX-6Zx9oERXXbdvHvIdO1zDqVAIAC1BUQaKnMRpAG842fAV94F7OZP7gKLcAx2ZrrEoDdQzZDt-M=@willsroot.io>
Date: Tue, 12 Aug 2025 16:59:59 +0000
From: William Liu <will@...lsroot.io>
To: Jakub Kicinski <kuba@...nel.org>
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 Tuesday, August 12th, 2025 at 2:38 PM, Jakub Kicinski <kuba@...nel.org> wrote:

> 
> 
> 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.


Ah ok, this makes much more sense now! Thank you for explaining - I will get a v5 in for this patch with your proposed fix soonish then.