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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 27 Jun 2025 17:15:08 -0700
Cong Wang <xiyou.wangcong@gmail.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.