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: <CAM0EoMmoQuRER=eBUO+Th02yJUYvfCKu_g7Ppcg0trnA_m6v1Q@mail.gmail.com>
Date: Mon, 30 Jun 2025 07:34:08 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Lion Ackermann <nnamrec@...il.com>
Cc: Cong Wang <xiyou.wangcong@...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

Hi,

On Mon, Jun 30, 2025 at 5:04 AM Lion Ackermann <nnamrec@...il.com> wrote:
>
> Hi,
>
> On 6/29/25 9:50 PM, Cong Wang wrote:
> > 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!
>
> Just for the record, I agree with all your points and as was stated this
> patch really only does damage prevention. Your proposal of preventing
> hierarchies sounds useful in the long run to keep the backlogs sane.
>
> I did run all the tdc tests on the latest net tree and they passed. Also
> my HFSC reproducer does not trigger with the proposed patch. I do not have
> a simple reproducer at hand for the QFQ tree case that you mentioned. So
> please verify this too if you can.
>
> Otherwise please feel free to go forward with the patch. If I can add
> anything else to the discussion please let me know.
>

Please post the patch formally as per Cong request. A tdc test case of
the reproducer would also help.

cheers,
jamal