Message-ID: <CAM0EoMkhASg-NVegj77+Gj+snmWog69ebHYEj3Rcj41hiUBf_A@mail.gmail.com>
Date: Tue, 1 Jul 2025 10:15:10 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: William Liu <will@...lsroot.io>, 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 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.

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

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

cheers,
jamal

Powered by blists - more mailing lists