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

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

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