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: <CAM0EoMnFHWBzRMVWYDZRvJYC+KwGPZhFOMsGn2PGV9RyY3yyiQ@mail.gmail.com>
Date: Sat, 28 Jun 2025 17:15:49 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: Cong Wang <xiyou.wangcong@...il.com>, William Liu <will@...lsroot.io>, netdev@...r.kernel.org, 
	victor@...atatu.com, pctammela@...atatu.com, pabeni@...hat.com, 
	kuba@...nel.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 11:15 AM Stephen Hemminger
<stephen@...workplumber.org> wrote:
>
> On Fri, 27 Jun 2025 17:15:08 -0700
> 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.
> >
> > Regards,
> > Cong Wang
>
> Looks like an ok workaround, but the name 'reserved' is most often used
> as placeholder in API's. Maybe something like 'duplicated'?
>
> Why a whole u32 for one flag?
>
> This increases qdisc_skb_cb from 28 bytes to 32 bytes.
> So still ok, but there should be a build check that it is less than
> space in skb->cb.
>

This is a nonsensical setup (in this case, it is nonsensical to have a
netem duplicate with a child netem in the hierarchy both with
duplicates) and imo adding fields to handle nonsensical setups is not
a good idea. I doubt that code will compile as is for example..

>From my POV Ive had it with these nonsensical setups from the bounty
hunting crowd (the majority of the pawning ones are nonsensical) -
disallowing these setups by adding deny lists is a good approach. I
would not have minded to add the field if this was a legitimate
setup....

cheers,
jamal