Message-ID: <aG2OUoDD2m5MqdSz@pop-os.localdomain>
Date: Tue, 8 Jul 2025 14:32:02 -0700
From: Cong Wang <xiyou.wangcong@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.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, linux-kernel@...r.kernel.org,
	torvalds@...ux-foundation.org
Subject: Re: This breaks netem use cases

(Cc Linus Torvalds)

On Tue, Jul 08, 2025 at 04:35:37PM -0400, Jamal Hadi Salim wrote:
> On Tue, Jul 8, 2025 at 3:42 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
> >
> > (Cc LKML for more audience, since this clearly breaks potentially useful
> > use cases)
> >
> > On Tue, Jul 08, 2025 at 04:43:26PM +0000, William Liu wrote:
> > > netem_enqueue's duplication prevention logic breaks when a netem
> > > resides in a qdisc tree with other netems - this can lead to a
> > > soft lockup and OOM loop in netem_dequeue, as seen in [1].
> > > Ensure that a duplicating netem cannot exist in a tree with other
> > > netems.
> >
> > As I already warned in your previous patchset, this breaks the following
> > potentially useful use case:
> >
> > sudo tc qdisc add dev eth0 root handle 1: mq
> > sudo tc qdisc add dev eth0 parent 1:1 handle 10: netem duplicate 100%
> > sudo tc qdisc add dev eth0 parent 1:2 handle 20: netem duplicate 100%
> >
> > I don't see any logical problem of such use case, therefore we should
> > consider it as valid, we can't break it.
> >
> 
> I thought we are trying to provide an intermediate solution to plug an
> existing hole and come up with a longer term solution.

Breaking valid use cases even for a short period is still no way to go.
Sorry, Jamal. Since I can't convince you, please ask Linus.

Also, I don't see you have proposed any long term solution. If you
really have one, please state it clearly and provide a clear timeline to
users.

> If there are users of such a "potential setup" you show above we are
> going to find out very quickly.

Please read the above specific example. It is more than just valid, it
is very reasonable, installing netem for each queue is the right way of
using netem duplication to avoid the global root spinlock in a multiqueue
setup.

Breaking users and letting them complain is not a good strategy either.

On the other hand, thanks for acknowledging it breaks users, which
confirms my point.

I will wait for Linus' response.

> We are working against security people who are finding all sorts of
> "potential use cases" to create CVEs.

I seriouly doubt the urgency of those CVE's, because none of them can be
triggered without root. Please don't get me wrong, I already fixed many of
them, but I believe they can wait, false urgency does not help anything.

> 
> > >
> > > Previous approaches suggested in discussions in chronological order:
> > >
> > > 1) Track duplication status or ttl in the sk_buff struct. Considered
> > > too specific a use case to extend such a struct, though this would
> > > be a resilient fix and address other previous and potential future
> > > DOS bugs like the one described in loopy fun [2].
> >
> > The link you provid is from 8 years ago, since then the redirection
> > logic has been improved. I am not sure why it helps to justify your
> > refusal of this approach.
> >
> > I also strongly disagree with "too specific a use case to extend such
> > a struct", we simply have so many use-case-specific fields within
> > sk_buff->cb. For example, the tc_skb_cb->zone is very specific
> > for act_ct.
> >
> > skb->cb is precisely designed to be use-case-specific and layer-specific.
> >
> > None of the above points stands.
> >
> 
> I doubt you have looked at the code based on how you keep coming back
> with the same points.

Please avoid personal attacks. It helps nothing to your argument here,
in fact, it will only weaken your arguments.

> > >
> > > 2) Restrict netem_enqueue recursion depth like in act_mirred with a
> > > per cpu variable. However, netem_dequeue can call enqueue on its
> > > child, and the depth restriction could be bypassed if the child is a
> > > netem.
> > >
> > > 3) Use the same approach as in 2, but add metadata in netem_skb_cb
> > > to handle the netem_dequeue case and track a packet's involvement
> > > in duplication. This is an overly complex approach, and Jamal
> > > notes that the skb cb can be overwritten to circumvent this
> > > safeguard.
> >
> > This is not true, except qdisc_skb_cb(skb)->data, other area of
> > skb->cb is preserved within Qdisc layer.
> >
> 
> Your approach has issues as i pointed out. At minimum invest the time
> please and look at code.

Sure, no one's patch is perfect. I am open to improve any of my patch.
First, let's discard this user-breaking patch for disatractions and
start focusing other solutions (not necessarily mine).

If it could help you, I can set the author to be you. I have no interest
to take any credit out of here, the reason why I sent out a patch is only
because you asked me to help.

> I am certain you could keep changing other code outside of netem and
> fix all issues you are exposing.
> We agreed this is for a short term solution and needs to be contained

I never agreed with you on breaking users, to me it is out of table for
discussion. Just to clarify.

> on just netem, what is the point of this whole long discussion really?

To defend users, obviously.

> We have spent over a month already..

Sorry to hear that, I think you are going to a wrong direction. The
earlier you switch to a right direction, the sooner we will have a right
solution without breaking any users.

Once again, please let me know how I specifically can help you out of
this situation. I am here to solve problems, not to bring one.

(If you need a video call for help, my calendar is open.)

Thanks for your understanding!

Powered by blists - more mailing lists