Message-ID: <gM-JGSLu4nHZhYs2dX6HRSjzJ_lTno28spo53-24pSDBk-sc3ygkQDrJEwxabhqB5PLf1IUJ1M64pEuj2t51mXDCUeqfzPhhHeu78P5oYrk=@willsroot.io>
Date: Tue, 01 Jul 2025 17:37:25 +0000
From: William Liu <will@...lsroot.io>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org, victor@...atatu.com, pctammela@...atatu.com, pabeni@...hat.com, kuba@...nel.org, stephen@...workplumber.org, dcaratti@...hat.com, savy@...t3mfailure.io, jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com, horms@...nel.org
Subject: Re: [PATCH net v4 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree






On Tuesday, July 1st, 2025 at 5:31 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:

> 
> 
> On Tue, Jul 01, 2025 at 10:15:10AM -0400, Jamal Hadi Salim wrote:
> 
> > On Mon, Jun 30, 2025 at 6:39 PM Cong Wang xiyou.wangcong@...il.com wrote:
> > 
> > > On Mon, Jun 30, 2025 at 07:32:48AM -0400, Jamal Hadi Salim wrote:
> > > 
> > > > On Sun, Jun 29, 2025 at 4:16 PM Cong Wang xiyou.wangcong@...il.com wrote:
> > > > 
> > > > > On Sat, Jun 28, 2025 at 05:25:25PM -0400, Jamal Hadi Salim wrote:
> > > > > 
> > > > > > your approach was to overwrite the netem specific cb which is exposed
> > > > > > via the cb ->data that can be overwritten for example by a trivial
> > > > > > ebpf program attach to any level of the hierarchy. This specific
> > > > > > variant from Cong is not accessible to ebpf but as i expressed my view
> > > > > > in other email i feel it is not a good solution.
> > > > > > 
> > > > > > https://lore.kernel.org/netdev/CAM0EoMk4dxOFoN_=3yOy+XrtU=yvjJXAw3fVTmN9=M=R=vtbxA@mail.gmail.com/
> > > > > 
> > > > > Hi Jamal,
> > > > > 
> > > > > I have two concerns regarding your/Will's proposal:
> > > > > 
> > > > > 1) I am not sure whether disallowing such case is safe. From my
> > > > > understanding this case is not obviously or logically wrong. So if we
> > > > > disallow it, we may have a chance to break some application.
> > > > 
> > > > I dont intentionaly creating a loop-inside-a-loop as being correct.
> > > > Stephen, is this a legit use case?
> > > > Agreed that we need to be careful about some corner cases which may
> > > > look crazy but are legit.
> > > 
> > > Maybe I misunderstand your patch, to me duplicating packets in
> > > parallel sub-hierarchy is not wrong, may be even useful.
> > 
> > TBH, there's no real world value for that specific config/repro and
> > worse that it causes the infinite loop.
> > I also cant see a good reason to have multiple netem children that all
> > loop back to root.
> > If there is one, we are going to find out when the patch goes in and
> > someone complains.
> 
> 
> I tend to be conservative here since breaking potential users is not a
> good practice. It takes a long time for regular users to realize this
> get removed since many of them use long term stable releases rather than
> the latest release.
> 
> Also, the patch using qdisc_skb_cb() looks smaller than this one,
> which means it is easier to review.
> 
> > > > > 2) Singling out this case looks not elegant to me.
> > > > 
> > > > My thinking is to long term disallow all nonsense hierarchy use cases,
> > > > such as this one, with some
> > > > "feature bits". ATM, it's easy to catch the bad configs within a
> > > > single qdisc in ->init() but currently not possible if it affects a
> > > > hierarchy.
> > > 
> > > The problem with this is it becomes harder to get a clear big picture,
> > > today netem, tomorrow maybe hfsc etc.? We could end up with hiding such
> > > bad-config-prevention code in different Qdisc's.
> > > 
> > > With the approach I suggested, we have a central place (probably
> > > sch_api.c) to have all the logics, nothing is hidden, easier to
> > > understand and easier to introduce more bad-config-prevention code.
> > > 
> > > I hope this makes sense to you.
> > 
> > To me the most positive outcome from the bounty hunters is getting
> > clarity that we not only need a per-qdisc validation as we do today,
> > but per-hierarchy as well; however, that is a different discussion we
> > can have after.
> > 
> > IIUC, we may be saying the same thing - a generic way to do hierarchy
> > validation. I even had a patch which i didnt think was the right thing
> > to do at the time. We can have that discussion.
> 
> 
> Why not? It is not even necessarily more complex to have a generic
> solution. With AI copilot, it is pretty quick. :)
> 
> FYI: I wrote the GSO segmentation patches with AI, they work well to fix
> the UAF report by Mingi. I can post them at any time, just waiting for
> Mingi's response to decide whether they are for -net or -net-next. I
> hope this more complicated case could convince you to use AI to write
> kernel code, if you still haven't.
> 
> > But let's please move forward with this patch, it fixes the
> > outstanding issues then we can discuss the best path forward more
> > calmly. The issue this patch fixes can be retrofitted into whatever
> > new scheme that we agree on after (and we may have to undo all the
> > backlog fixes as well).
> 
> 
> Sure, I will send out a patch to use qdisc_skb_cb() to help everyone out
> of this situation.
> 

I can send a patch with that variant for you all to compare sometime in the next 24 hrs - I have been working on fixing this back and forth with Jamal for the past month.

Best,
Will

Powered by blists - more mailing lists