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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMkUi470+z86ztEMAGfYcG8aYiC2e5pP0z1BHz82O4RCPg@mail.gmail.com>
Date: Sat, 28 Jun 2025 17:25:25 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: William Liu <will@...lsroot.io>
Cc: Cong Wang <xiyou.wangcong@...il.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 Sat, Jun 28, 2025 at 12:23 AM William Liu <will@...lsroot.io> wrote:
>
> On Saturday, June 28th, 2025 at 12:15 AM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> >
> >
> > On Fri, Jun 27, 2025 at 06:17:31AM +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.
> >
> >
> > Thanks for providing more details.
> >
> > > 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].
> > >
> > > 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 approach looks most elegant to me since it is per-skb and only
> > contained for netem. Since netem_skb_cb is shared among qdisc's, what
> > about just extending qdisc_skb_cb? Something like:
> >
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index 638948be4c50..4c5505661986 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -436,6 +436,7 @@ struct qdisc_skb_cb {
> > unsigned int pkt_len;
> > u16 slave_dev_queue_mapping;
> > u16 tc_classid;
> > + u32 reserved;
> > };
> > #define QDISC_CB_PRIV_LEN 20
> > unsigned char data[QDISC_CB_PRIV_LEN];
> >
> >
> > Then we just set and check it for duplicated skbs:
> >
> >
> > diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> > index fdd79d3ccd8c..4290f8fca0e9 100644
> > --- a/net/sched/sch_netem.c
> > +++ b/net/sched/sch_netem.c
> > @@ -486,7 +486,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> > * If we need to duplicate packet, then clone it before
> > * original is modified.
> > */
> > - if (count > 1)
> >
> > + if (count > 1 && !qdisc_skb_cb(skb)->reserved)
> >
> > skb2 = skb_clone(skb, GFP_ATOMIC);
> >
> > /*
> > @@ -540,9 +540,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> > struct Qdisc rootq = qdisc_root_bh(sch);
> > u32 dupsave = q->duplicate; / prevent duplicating a dup... */
> >
> >
> > - q->duplicate = 0;
> >
> > + qdisc_skb_cb(skb2)->reserved = dupsave;
> >
> > rootq->enqueue(skb2, rootq, to_free);
> >
> > - q->duplicate = dupsave;
> >
> > skb2 = NULL;
> > }
> >
> >
> > Could this work? It looks even shorter than your patch. :-)
> >
> > Note, I don't even compile test it, I just show it to you for discussion.
> >
>
> Thank you for the suggestion. Jamal, would this work in this case? I recall you mentioning that the cb approach can be circumvented by zeroing the cb at the root:

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.

cheers,
jamal

https://lore.kernel.org/netdev/CAM0EoMk4dxOFoN_=3yOy+XrtU=yvjJXAw3fVTmN9=M=R=vtbxA@mail.gmail.com/
>
> I'm not sure if qdisc_skb_cb would be different than the case for private data for qdiscs.
>