Re: [PATCH net v4 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree

[Cong Wang <xiyou.wangcong@gmail.com>]

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

Thanks!