netdev - Re: Incomplete fix for recent bug in tc / hfsc

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: <aGGZBpA3Pn4ll7FO@pop-os.localdomain>
Date: Sun, 29 Jun 2025 12:50:30 -0700
From: Cong Wang <xiyou.wangcong@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Lion Ackermann <nnamrec@...il.com>, netdev@...r.kernel.org,
	Jiri Pirko <jiri@...nulli.us>, Mingi Cho <mincho@...ori.io>
Subject: Re: Incomplete fix for recent bug in tc / hfsc

On Sun, Jun 29, 2025 at 10:29:44AM -0400, Jamal Hadi Salim wrote:
> > On "What do you think the root cause is here?"
> >
> > I believe the root cause is that qdiscs like hfsc and qfq are dropping
> > all packets in enqueue (mostly in relation to peek()) and that result
> > is not being reflected in the return code returned to its parent
> > qdisc.
> > So, in the example you described in this thread, drr is oblivious to
> > the fact that the child qdisc dropped its packet because the call to
> > its child enqueue returned NET_XMIT_SUCCESS. This causes drr to
> > activate a class that shouldn't have been activated at all.
> >
> > You can argue that drr (and other similar qdiscs) may detect this by
> > checking the call to qlen_notify (as the drr patch was
> > doing), but that seems really counter-intuitive. Imagine writing a new
> > qdisc and having to check for that every time you call a child's
> > enqueue. Sure  your patch solves this, but it also seems like it's not
> > fixing the underlying issue (which is drr activating the class in the
> > first place). Your patch is simply removing all the classes from their
> > active lists when you delete them. And your patch may seem ok for now,
> > but I am worried it might break something else in the future that we
> > are not seeing.
> >
> > And do note: All of the examples of the hierarchy I have seen so far,
> > that put us in this situation, are nonsensical
> >
> 
> At this point my thinking is to apply your patch and then we discuss a
> longer term solution. Cong?

I agree. If Lion's patch works, it is certainly much better as a bug fix
for both -net and -stable.

Also for all of those ->qlen_notify() craziness, I think we need to
rethink about the architecture, _maybe_ there are better architectural
solutions.

Thanks!