Message-ID: <CAM0EoMnjS0kaNDttQtCZ+=hq9egOiRDANN+oQcMOBRnXLVjgRw@mail.gmail.com>
Date: Mon, 30 Jun 2025 07:32:48 -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 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.

> 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.
For starters this is the first one such "deny" feature specific to
netem to not allow hierarchies where we can have a netem loop inside a
loop. It didnt seem like we need the infrastructure just to handle one
case and for that reason i thought William's solution was reasonable.
I could swear i posted a sample patch a while back but i cant find it
now.
The other obvious case seems to be  ->qlen_notify() effect (but there
may be other solutions to that one which we can discuss).

The problem we have is all the bounty hunting is focussed on finding
such nonsensical hierarchy (and it is getting exhausting). We can keep
fixing things that break because the bounty hunters find another
netlink message to send that will put it back into an awkward
position.

> Even _if_ we really
> want to disallow such case, we still need to find a better and more
> elegant way to do so, for example, adding a new ops for netem and calling
> it in sch_api.c. Something like below:
>
> // Implement netem_avoid_duplicate()
> // ...
>
> static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
>   .avoid_duplicate = netem_avoid_duplicate,
> };
>
> // In sch_api.c
> // traverse the Qdisch hierarch and call ->avoid_duplicate()
>
> What do you think?
>

Could this not be circumvented by some filter setting this to some bad
state? The answer seems to be "yes" but i may be misreading.
If you can isolate the solution to just netem and netem fields then it
should be fine. What i am not comfortable with is adding some feature
or metadata that is available across all qdiscs just to solve this
(nonsensical) hierarchy use case.

cheers,
jamal

> Thanks.

Powered by blists - more mailing lists