[Patch v2 net 0/2] netem: Fix skb duplication logic to prevent infinite loops

[Patch v2 net 0/2] netem: Fix skb duplication logic to prevent infinite loops

[Cong Wang]

This patchset fixes the infinite loops due to skb duplication in netem.

This replaces the patches from William, with much less code and without
any workaround. More importantly, this does not break any use case at
all.

Note: This patch only aims to fix the infinite loops, nothing else. If
there is other issue with netem duplication, it needs to be addressed
separately.

---
v2: fixed a typo
    improved tdc selftest to check sent bytes

Cong Wang (2):
  netem: Fix skb duplication logic to prevent infinite loops
  selftests/tc-testing: Add a nested netem duplicate test

 include/net/sch_generic.h                     |  1 +
 net/sched/sch_netem.c                         |  7 +++---
 .../tc-testing/tc-tests/qdiscs/netem.json     | 25 +++++++++++++++++++
 3 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.34.1

[Patch v2 net 1/2] netem: Fix skb duplication logic to prevent infinite loops

[Cong Wang]

This patch refines the packet duplication handling in netem_enqueue() to ensure
that only newly cloned skbs are marked as duplicates. This prevents scenarios
where nested netem qdiscs with 100% duplication could cause infinite loops of
skb duplication.

By ensuring the duplicate flag is properly managed, this patch maintains skb
integrity and avoids excessive packet duplication in complex qdisc setups.

Now we could also get rid of the ugly temporary overwrite of
q->duplicate.

Fixes: 0afb51e72855 ("[PKT_SCHED]: netem: reinsert for duplication")
Reported-by: William Liu <will@willsroot.io>
Reported-by: Savino Dicanosa <savy@syst3mfailure.io>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h | 1 +
 net/sched/sch_netem.c     | 7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 638948be4c50..595b24180d62 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1067,6 +1067,7 @@ struct tc_skb_cb {
 	u8 post_ct:1;
 	u8 post_ct_snat:1;
 	u8 post_ct_dnat:1;
+	u8 duplicate:1;
 };
 
 static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fdd79d3ccd8c..249095ba7f98 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -460,7 +460,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	skb->prev = NULL;
 
 	/* Random duplication */
-	if (q->duplicate && q->duplicate >= get_crandom(&q->dup_cor, &q->prng))
+	if (!tc_skb_cb(skb)->duplicate &&
+	    q->duplicate >= get_crandom(&q->dup_cor, &q->prng))
 		++count;
 
 	/* Drop packet? */
@@ -538,11 +539,9 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	 */
 	if (skb2) {
 		struct Qdisc *rootq = qdisc_root_bh(sch);
-		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
 
-		q->duplicate = 0;
+		tc_skb_cb(skb2)->duplicate = 1;
 		rootq->enqueue(skb2, rootq, to_free);
-		q->duplicate = dupsave;
 		skb2 = NULL;
 	}
 
-- 
2.34.1

[Patch v2 net 2/2] selftests/tc-testing: Add a nested netem duplicate test

[Cong Wang]

Integrate the reproducer from William into tc-testing and use scapy
to generate packets for testing:

 # ./tdc.py -e 1676
  -- ns/SubPlugin.__init__
  -- scapy/SubPlugin.__init__
 Test 1676: NETEM nested duplicate 100%
 [ 1154.071135] v0p0id1676: entered promiscuous mode
 [ 1154.101066] virtio_net virtio0 enp1s0: entered promiscuous mode
 [ 1154.146301] virtio_net virtio0 enp1s0: left promiscuous mode
 .
 Sent 1 packets.
 [ 1154.173695] v0p0id1676: left promiscuous mode
 [ 1154.216159] v0p0id1676: entered promiscuous mode
 .
 Sent 1 packets.
 [ 1154.238398] v0p0id1676: left promiscuous mode
 [ 1154.260909] v0p0id1676: entered promiscuous mode
 .
 Sent 1 packets.
 [ 1154.282708] v0p0id1676: left promiscuous mode
 [ 1154.309399] v0p0id1676: entered promiscuous mode
 .
 Sent 1 packets.
 [ 1154.337949] v0p0id1676: left promiscuous mode
 [ 1154.360924] v0p0id1676: entered promiscuous mode
 .
 Sent 1 packets.
 [ 1154.383522] v0p0id1676: left promiscuous mode

 All test results:

 1..1
 ok 1 1676 - NETEM nested duplicate 100%

Reported-by: William Liu <will@willsroot.io>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/qdiscs/netem.json     | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/netem.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/netem.json
index 3c4444961488..1211497b2acc 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/netem.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/netem.json
@@ -336,5 +336,30 @@
         "teardown": [
             "$TC qdisc del dev $DUMMY handle 1: root"
         ]
+    },
+    {
+        "id": "1676",
+        "name": "NETEM nested duplicate 100%",
+        "category": ["qdisc", "netem"],
+        "setup": [
+            "$TC qdisc add dev $DEV1 root handle 1: netem limit 1 duplicate 100%",
+            "$TC qdisc add dev $DEV1 parent 1: handle 2: netem gap 1 limit 1 duplicate 100% delay 1us reorder 100%"
+        ],
+        "teardown": [
+            "$TC qdisc del dev $DEV1 root"
+        ],
+        "plugins": {
+            "requires": ["nsPlugin", "scapyPlugin"]
+        },
+        "scapy": {
+            "iface": "$DEV0",
+            "count": 5,
+            "packet": "Ether()/IP(dst='10.10.10.1', src='10.10.10.10')/TCP(sport=12345, dport=80)"
+        },
+        "cmdUnderTest": "$TC -s qdisc show dev $DEV1",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DEV1",
+        "matchPattern": "Sent [0-9]+ bytes [0-9]+ pkt",
+        "matchCount": "2"
     }
 ]
-- 
2.34.1

Re: [Patch v2 net 1/2] netem: Fix skb duplication logic to prevent infinite loops

[Simon Horman]

On Mon, Jul 07, 2025 at 12:50:14PM -0700, Cong Wang wrote:
> This patch refines the packet duplication handling in netem_enqueue() to ensure
> that only newly cloned skbs are marked as duplicates. This prevents scenarios
> where nested netem qdiscs with 100% duplication could cause infinite loops of
> skb duplication.
> 
> By ensuring the duplicate flag is properly managed, this patch maintains skb
> integrity and avoids excessive packet duplication in complex qdisc setups.
> 
> Now we could also get rid of the ugly temporary overwrite of
> q->duplicate.
> 
> Fixes: 0afb51e72855 ("[PKT_SCHED]: netem: reinsert for duplication")
> Reported-by: William Liu <will@willsroot.io>
> Reported-by: Savino Dicanosa <savy@syst3mfailure.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [Patch v2 net 2/2] selftests/tc-testing: Add a nested netem duplicate test

[Simon Horman]

On Mon, Jul 07, 2025 at 12:50:15PM -0700, Cong Wang wrote:
> Integrate the reproducer from William into tc-testing and use scapy
> to generate packets for testing:
> 
>  # ./tdc.py -e 1676
>   -- ns/SubPlugin.__init__
>   -- scapy/SubPlugin.__init__
>  Test 1676: NETEM nested duplicate 100%
>  [ 1154.071135] v0p0id1676: entered promiscuous mode
>  [ 1154.101066] virtio_net virtio0 enp1s0: entered promiscuous mode
>  [ 1154.146301] virtio_net virtio0 enp1s0: left promiscuous mode
>  .
>  Sent 1 packets.
>  [ 1154.173695] v0p0id1676: left promiscuous mode
>  [ 1154.216159] v0p0id1676: entered promiscuous mode
>  .
>  Sent 1 packets.
>  [ 1154.238398] v0p0id1676: left promiscuous mode
>  [ 1154.260909] v0p0id1676: entered promiscuous mode
>  .
>  Sent 1 packets.
>  [ 1154.282708] v0p0id1676: left promiscuous mode
>  [ 1154.309399] v0p0id1676: entered promiscuous mode
>  .
>  Sent 1 packets.
>  [ 1154.337949] v0p0id1676: left promiscuous mode
>  [ 1154.360924] v0p0id1676: entered promiscuous mode
>  .
>  Sent 1 packets.
>  [ 1154.383522] v0p0id1676: left promiscuous mode
> 
>  All test results:
> 
>  1..1
>  ok 1 1676 - NETEM nested duplicate 100%
> 
> Reported-by: William Liu <will@willsroot.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [Patch v2 net 1/2] netem: Fix skb duplication logic to prevent infinite loops

[Jamal Hadi Salim]

On Tue, Jul 8, 2025 at 9:18 AM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jul 07, 2025 at 12:50:14PM -0700, Cong Wang wrote:
> > This patch refines the packet duplication handling in netem_enqueue() to ensure
> > that only newly cloned skbs are marked as duplicates. This prevents scenarios
> > where nested netem qdiscs with 100% duplication could cause infinite loops of
> > skb duplication.
> >
> > By ensuring the duplicate flag is properly managed, this patch maintains skb
> > integrity and avoids excessive packet duplication in complex qdisc setups.
> >
> > Now we could also get rid of the ugly temporary overwrite of
> > q->duplicate.
> >
> > Fixes: 0afb51e72855 ("[PKT_SCHED]: netem: reinsert for duplication")
> > Reported-by: William Liu <will@willsroot.io>
> > Reported-by: Savino Dicanosa <savy@syst3mfailure.io>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> Reviewed-by: Simon Horman <horms@kernel.org>

Simon,
This patch wont work - see the sample config i presented.

cheers,
jamal

Re: [Patch v2 net 1/2] netem: Fix skb duplication logic to prevent infinite loops

[William Liu]

On Tuesday, July 8th, 2025 at 1:18 PM, Simon Horman <horms@kernel.org> wrote:

> 
> 
> On Mon, Jul 07, 2025 at 12:50:14PM -0700, Cong Wang wrote:
> 
> > This patch refines the packet duplication handling in netem_enqueue() to ensure
> > that only newly cloned skbs are marked as duplicates. This prevents scenarios
> > where nested netem qdiscs with 100% duplication could cause infinite loops of
> > skb duplication.
> > 
> > By ensuring the duplicate flag is properly managed, this patch maintains skb
> > integrity and avoids excessive packet duplication in complex qdisc setups.
> > 
> > Now we could also get rid of the ugly temporary overwrite of
> > q->duplicate.
> > 
> > Fixes: 0afb51e72855 ("[PKT_SCHED]: netem: reinsert for duplication")
> > Reported-by: William Liu will@willsroot.io
> > Reported-by: Savino Dicanosa savy@syst3mfailure.io
> > Signed-off-by: Cong Wang xiyou.wangcong@gmail.com
> 
> 
> Reviewed-by: Simon Horman horms@kernel.org

From what Jakub and Jamal suggested, the patch for review is now https://patchwork.kernel.org/project/netdevbpf/list/?series=976473&state=*

It was marked for "Changes Requested" about a week ago but I don't recall any specific changes people wanted for v4 of that patch, other than the attempt at this alternative approach in this patchset thread.

Re: [Patch v2 net 1/2] netem: Fix skb duplication logic to prevent infinite loops

[Jakub Kicinski]

On Tue, 08 Jul 2025 15:56:29 +0000 William Liu wrote:
> From what Jakub and Jamal suggested, the patch for review is now https://patchwork.kernel.org/project/netdevbpf/list/?series=976473&state=*
> 
> It was marked for "Changes Requested" about a week ago but I don't recall any specific changes people wanted for v4 of that patch, other than the attempt at this alternative approach in this patchset thread.

Please repost to get it back into the review queue

Re: [Patch v2 net 1/2] netem: Fix skb duplication logic to prevent infinite loops

[Simon Horman]

On Tue, Jul 08, 2025 at 09:19:50AM -0400, Jamal Hadi Salim wrote:
> On Tue, Jul 8, 2025 at 9:18 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Mon, Jul 07, 2025 at 12:50:14PM -0700, Cong Wang wrote:
> > > This patch refines the packet duplication handling in netem_enqueue() to ensure
> > > that only newly cloned skbs are marked as duplicates. This prevents scenarios
> > > where nested netem qdiscs with 100% duplication could cause infinite loops of
> > > skb duplication.
> > >
> > > By ensuring the duplicate flag is properly managed, this patch maintains skb
> > > integrity and avoids excessive packet duplication in complex qdisc setups.
> > >
> > > Now we could also get rid of the ugly temporary overwrite of
> > > q->duplicate.
> > >
> > > Fixes: 0afb51e72855 ("[PKT_SCHED]: netem: reinsert for duplication")
> > > Reported-by: William Liu <will@willsroot.io>
> > > Reported-by: Savino Dicanosa <savy@syst3mfailure.io>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> >
> > Reviewed-by: Simon Horman <horms@kernel.org>
> 
> Simon,
> This patch wont work - see the sample config i presented.

Sorry, I convinced myself it would.
But it seems I was wrong.