[PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Victor Nogueira]

As described in Gerrard's report [1], there are cases where netem can
make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
qfq) break whenever the enqueue callback has reentrant behaviour.
This series addresses these issues by adding extra checks that cater for
these reentrant corner cases. This series has passed all relevant test
cases in the TDC suite.

[1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/

v1 -> v2:
- Removed RFC tag
- Added Jamal's Acked-by
- Added TDC tests
- Small cleanups

Victor Nogueira (5):
  net_sched: drr: Fix double list add in class with netem as child qdisc
  net_sched: hfsc: Fix a UAF vulnerability in class with netem as child
    qdisc
  net_sched: ets: Fix double list add in class with netem as child qdisc
  net_sched: qfq: Fix double list add in class with netem as child qdisc
  selftests: tc-testing: Add TDC tests that exercise reentrant enqueue
    behaviour

 net/sched/sch_drr.c                           |   7 +-
 net/sched/sch_ets.c                           |   7 +-
 net/sched/sch_hfsc.c                          |   2 +-
 net/sched/sch_qfq.c                           |   8 +
 .../tc-testing/tc-tests/infra/qdiscs.json     | 148 ++++++++++++++++++
 5 files changed, 169 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH net v2 1/5] net_sched: drr: Fix double list add in class with netem as child qdisc

[Victor Nogueira]

As described in Gerrard's report [1], there are use cases where a netem
child qdisc will make the parent qdisc's enqueue callback reentrant.
In the case of drr, there won't be a UAF, but the code will add the same
classifier to the list twice, which will cause memory corruption.

In addition to checking for qlen being zero, this patch checks whether the
class was already added to the active_list (cl_is_initialised) before
adding to the list to cover for the reentrant case.

[1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/

Fixes: 37d9cf1a3ce3 ("sched: Fix detection of empty queues in child qdiscs")
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/sch_drr.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index e0a81d313aa7..b18b7b739deb 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -35,6 +35,11 @@ struct drr_sched {
 	struct Qdisc_class_hash		clhash;
 };
 
+static bool cl_is_initialised(struct drr_class *cl)
+{
+	return !list_empty(&cl->alist);
+}
+
 static struct drr_class *drr_find_class(struct Qdisc *sch, u32 classid)
 {
 	struct drr_sched *q = qdisc_priv(sch);
@@ -357,7 +362,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return err;
 	}
 
-	if (first) {
+	if (first && !cl_is_initialised(cl)) {
 		list_add_tail(&cl->alist, &q->active);
 		cl->deficit = cl->quantum;
 	}
-- 
2.34.1

Re: [PATCH net v2 1/5] net_sched: drr: Fix double list add in class with netem as child qdisc

[Jakub Kicinski]

On Wed, 16 Apr 2025 07:24:23 -0300 Victor Nogueira wrote:
> +static bool cl_is_initialised(struct drr_class *cl)

cl_is_active() ?
Had to look at the code to figure out what it does, but doesn't seem to
have much to do with being "initialised". The point is that the list
node of this class is not on the list of active classes.

> +	return !list_empty(&cl->alist);
> +}
> +
>  static struct drr_class *drr_find_class(struct Qdisc *sch, u32 classid)
>  {
>  	struct drr_sched *q = qdisc_priv(sch);
> @@ -357,7 +362,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		return err;
>  	}
>  
> -	if (first) {
> +	if (first && !cl_is_initialised(cl)) {

I think we can delete the "first" check and temp variable.
The code under the if() does not touch the packet so it doesn't matter
whether we execute it for the initial or the nested call, right?

Re: [PATCH net v2 1/5] net_sched: drr: Fix double list add in class with netem as child qdisc

[Victor Nogueira]

On 4/22/25 21:44, Jakub Kicinski wrote:
> On Wed, 16 Apr 2025 07:24:23 -0300 Victor Nogueira wrote:
>> +static bool cl_is_initialised(struct drr_class *cl)
> cl_is_active() ?
> Had to look at the code to figure out what it does, but doesn't seem to
> have much to do with being "initialised". The point is that the list
> node of this class is not on the list of active classes.

Ok, I believe cl_is_active describes it better. I'll make that change.

>>   static struct drr_class *drr_find_class(struct Qdisc *sch, u32 classid)
>>   {
>>   	struct drr_sched *q = qdisc_priv(sch);
>> @@ -357,7 +362,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>   		return err;
>>   	}
>>   
>> -	if (first) {
>> +	if (first && !cl_is_initialised(cl)) {
> I think we can delete the "first" check and temp variable.
> The code under the if() does not touch the packet so it doesn't matter
> whether we execute it for the initial or the nested call, right?

I was being more conservative, but your suggestion is a good optimisation.

cheers,
Victor

[PATCH net v2 2/5] net_sched: hfsc: Fix a UAF vulnerability in class with netem as child qdisc

[Victor Nogueira]

As described in Gerrard's report [1], we have a UAF case when an hfsc class
has a netem child qdisc. The crux of the issue is that hfsc is assuming
that checking for cl->qdisc->q.qlen == 0 guarantees that it hasn't inserted
the class in the vttree or eltree (which is not true for the netem
duplicate case).

This patch checks the n_active class variable to make sure that the code
won't insert the class in the vttree or eltree twice, catering for the
reentrant case.

[1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/

Fixes: 37d9cf1a3ce3 ("sched: Fix detection of empty queues in child qdiscs")
Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/sch_hfsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index ce5045eea065..73b0741ffd99 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1564,7 +1564,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 		return err;
 	}
 
-	if (first) {
+	if (first && !cl->cl_nactive) {
 		if (cl->cl_flags & HFSC_RSC)
 			init_ed(cl, len);
 		if (cl->cl_flags & HFSC_FSC)
-- 
2.34.1

[PATCH net v2 3/5] net_sched: ets: Fix double list add in class with netem as child qdisc

[Victor Nogueira]

As described in Gerrard's report [1], there are use cases where a netem
child qdisc will make the parent qdisc's enqueue callback reentrant.
In the case of ets, there won't be a UAF, but the code will add the same
classifier to the list twice, which will cause memory corruption.

In addition to checking for qlen being zero, this patch checks whether
the class was already added to the active_list (cl_is_initialised) before
doing the addition to cater for the reentrant case.

[1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/

Fixes: 37d9cf1a3ce3 ("sched: Fix detection of empty queues in child qdiscs")
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/sch_ets.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index c3bdeb14185b..af5827377ebc 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -74,6 +74,11 @@ static const struct nla_policy ets_class_policy[TCA_ETS_MAX + 1] = {
 	[TCA_ETS_QUANTA_BAND] = { .type = NLA_U32 },
 };
 
+static bool cl_is_initialised(struct ets_class *cl)
+{
+	return !list_empty(&cl->alist);
+}
+
 static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
 			     unsigned int *quantum,
 			     struct netlink_ext_ack *extack)
@@ -436,7 +441,7 @@ static int ets_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return err;
 	}
 
-	if (first && !ets_class_is_strict(q, cl)) {
+	if (first && !cl_is_initialised(cl) && !ets_class_is_strict(q, cl)) {
 		list_add_tail(&cl->alist, &q->active);
 		cl->deficit = cl->quantum;
 	}
-- 
2.34.1

[PATCH net v2 4/5] net_sched: qfq: Fix double list add in class with netem as child qdisc

[Victor Nogueira]

As described in Gerrard's report [1], there are use cases where a netem
child qdisc will make the parent qdisc's enqueue callback reentrant.
In the case of qfq, there won't be a UAF, but the code will add the same
classifier to the list twice, which will cause memory corruption.

This patch checks whether the class was already added to the agg->active
list (cl_is_initialised) before doing the addition to cater for the
reentrant case.

[1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/

Fixes: 37d9cf1a3ce3 ("sched: Fix detection of empty queues in child qdiscs")
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/sch_qfq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 687a932eb9b2..b7767b105506 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -202,6 +202,11 @@ struct qfq_sched {
  */
 enum update_reason {enqueue, requeue};
 
+static bool cl_is_initialised(struct qfq_class *cl)
+{
+	return !list_empty(&cl->alist);
+}
+
 static struct qfq_class *qfq_find_class(struct Qdisc *sch, u32 classid)
 {
 	struct qfq_sched *q = qdisc_priv(sch);
@@ -1260,6 +1265,9 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		    == cl && cl->deficit < len)
 			list_move_tail(&cl->alist, &agg->active);
 
+		return err;
+	/* cater for reentrant call */
+	} else if (cl_is_initialised(cl)) {
 		return err;
 	}
 
-- 
2.34.1

[PATCH net v2 5/5] selftests: tc-testing: Add TDC tests that exercise reentrant enqueue behaviour

[Victor Nogueira]

Add 4 TDC tests that exercise the reentrant enqueue behaviour in drr,
ets, qfq, and hfsc:

- Test DRR's enqueue reentrant behaviour with netem (which caused a
  double list add)
- Test ETS's enqueue reentrant behaviour with netem (which caused a double
  list add)
- Test QFQ's enqueue reentrant behaviour with netem (which caused a double
  list add)
- Test HFSC's enqueue reentrant behaviour with netem (which caused a UAF)

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 148 ++++++++++++++++++
 1 file changed, 148 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
index d4ea9cd845a3..19037059e9e4 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -313,5 +313,153 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "90ec",
+        "name": "Test DRR's enqueue reentrant behaviour with netem",
+        "category": [
+            "qdisc",
+            "drr"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1:0 root drr",
+            "$TC class replace dev $DUMMY parent 1:0 classid 1:1 drr",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
+            "$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1"
+        ],
+        "cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
+        "matchJSON": [
+            {
+                "kind": "drr",
+                "handle": "1:",
+                "bytes": 196,
+                "packets": 2
+            }
+        ],
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1:0 root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
+    },
+    {
+        "id": "1f1f",
+        "name": "Test ETS's enqueue reentrant behaviour with netem",
+        "category": [
+            "qdisc",
+            "ets"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1:0 root ets bands 2",
+            "$TC class replace dev $DUMMY parent 1:0 classid 1:1 ets quantum 1500",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
+            "$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1"
+        ],
+        "cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -j -s class show dev $DUMMY",
+        "matchJSON": [
+            {
+                "class": "ets",
+                "handle": "1:1",
+                "stats": {
+                    "bytes": 196,
+                    "packets": 2
+                }
+            }
+        ],
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1:0 root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
+    },
+    {
+        "id": "5e6d",
+        "name": "Test QFQ's enqueue reentrant behaviour with netem",
+        "category": [
+            "qdisc",
+            "qfq"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1:0 root qfq",
+            "$TC class replace dev $DUMMY parent 1:0 classid 1:1 qfq weight 100 maxpkt 1500",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
+            "$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1"
+        ],
+        "cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
+        "matchJSON": [
+            {
+                "kind": "qfq",
+                "handle": "1:",
+                "bytes": 196,
+                "packets": 2
+            }
+        ],
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1:0 root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
+    },
+    {
+        "id": "bf1d",
+        "name": "Test HFSC's enqueue reentrant behaviour with netem",
+        "category": [
+            "qdisc",
+            "hfsc"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1:0 root hfsc",
+            "$TC class add dev $DUMMY parent 1:0 classid 1:1 hfsc ls m2 10Mbit",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
+            "$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip dst 10.10.10.1/32 flowid 1:1",
+            "$TC class add dev $DUMMY parent 1:0 classid 1:2 hfsc ls m2 10Mbit",
+            "$TC qdisc add dev $DUMMY parent 1:2 handle 3:0 netem duplicate 100%",
+            "$TC filter add dev $DUMMY parent 1:0 protocol ip prio 2 u32 match ip dst 10.10.10.2/32 flowid 1:2",
+            "ping -c 1 10.10.10.1 -I$DUMMY > /dev/null || true",
+            "$TC filter del dev $DUMMY parent 1:0 protocol ip prio 1",
+            "$TC class del dev $DUMMY classid 1:1"
+        ],
+        "cmdUnderTest": "ping -c 1 10.10.10.2 -I$DUMMY > /dev/null || true",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
+        "matchJSON": [
+            {
+                "kind": "hfsc",
+                "handle": "1:",
+                "bytes": 392,
+                "packets": 4
+            }
+        ],
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1:0 root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Jakub Kicinski]

On Wed, 16 Apr 2025 07:24:22 -0300 Victor Nogueira wrote:
> As described in Gerrard's report [1], there are cases where netem can
> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
> qfq) break whenever the enqueue callback has reentrant behaviour.
> This series addresses these issues by adding extra checks that cater for
> these reentrant corner cases. This series has passed all relevant test
> cases in the TDC suite.

Sorry for asking this question a bit late, but reentrant enqueue seems
error prone. Is there a clear use case for netem as a child?
If so should we also add some sort of "capability" to avoid new qdiscs
falling into the same trap, without giving the problem any thought?

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Victor Nogueira]

On 4/17/25 13:07, Jakub Kicinski wrote:
> On Wed, 16 Apr 2025 07:24:22 -0300 Victor Nogueira wrote:
>> As described in Gerrard's report [1], there are cases where netem can
>> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
>> qfq) break whenever the enqueue callback has reentrant behaviour.
>> This series addresses these issues by adding extra checks that cater for
>> these reentrant corner cases. This series has passed all relevant test
>> cases in the TDC suite.
> 
> Sorry for asking this question a bit late, but reentrant enqueue seems
> error prone. Is there a clear use case for netem as a child?

We discussed this internally as well before i sent this fix.
The 3 examples are buggy in picking the correct active class in the
corner case the poc produced. So these are bug fixes regardless - and
they happen to fix the issue.
We also wondered why it was not appropriate for netem to always be root
qdisc. Looking around the manpage states that you can have a qdisc
like tbf as a parent. This issue happens only when netem "duplication
feature" is used. Not sure what other use cases are out there that
expect netem to be a child.
+Cc Stephen who can give a more authoritative answer.

> If so should we also add some sort of "capability" to avoid new qdiscs
> falling into the same trap, without giving the problem any thought?
I think the bug needs to be fix regardless, but I think your
suggestion also makes sense and is more future proof.
I can send another patch later that adds a "capability",as you
suggested, (let's say ALLOWS_REENTRANT_ENQ or something similar)
and only allows netem to be a child of qdiscs that have this flag.

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Paolo Abeni]

On 4/17/25 6:07 PM, Jakub Kicinski wrote:
> On Wed, 16 Apr 2025 07:24:22 -0300 Victor Nogueira wrote:
>> As described in Gerrard's report [1], there are cases where netem can
>> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
>> qfq) break whenever the enqueue callback has reentrant behaviour.
>> This series addresses these issues by adding extra checks that cater for
>> these reentrant corner cases. This series has passed all relevant test
>> cases in the TDC suite.
> 
> Sorry for asking this question a bit late, but reentrant enqueue seems
> error prone. Is there a clear use case for netem as a child?
> If so should we also add some sort of "capability" to avoid new qdiscs
> falling into the same trap, without giving the problem any thought?

AFAIK netem use is quite widespread, even outside testing scenarios. I
suspect preventing netem from nesting under arbitrary root could break
multiple ("unusual") users.

Cheers,

Paolo

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Cong Wang]

On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote:
> As described in Gerrard's report [1], there are cases where netem can
> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
> qfq) break whenever the enqueue callback has reentrant behaviour.
> This series addresses these issues by adding extra checks that cater for
> these reentrant corner cases. This series has passed all relevant test
> cases in the TDC suite.
> 
> [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/
> 

I am wondering why we need to enqueue the duplicate skb before enqueuing
the original skb in netem? IOW, why not just swap them?

Thanks

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Victor Nogueira]

On 4/17/25 16:23, Cong Wang wrote:
> On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote:
>> As described in Gerrard's report [1], there are cases where netem can
>> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
>> qfq) break whenever the enqueue callback has reentrant behaviour.
>> This series addresses these issues by adding extra checks that cater for
>> these reentrant corner cases. This series has passed all relevant test
>> cases in the TDC suite.
>>
>> [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/
>>
> 
> I am wondering why we need to enqueue the duplicate skb before enqueuing
> the original skb in netem? IOW, why not just swap them?

I thought of doing what, I think, you are suggesting, but I was afraid
of breaking netem. Stephen any comments?

cheers,
Victor

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Paolo Abeni]

On 4/17/25 9:23 PM, Cong Wang wrote:
> On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote:
>> As described in Gerrard's report [1], there are cases where netem can
>> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
>> qfq) break whenever the enqueue callback has reentrant behaviour.
>> This series addresses these issues by adding extra checks that cater for
>> these reentrant corner cases. This series has passed all relevant test
>> cases in the TDC suite.
>>
>> [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/
>>
> 
> I am wondering why we need to enqueue the duplicate skb before enqueuing
> the original skb in netem? IOW, why not just swap them?

It's not clear to me what you are suggesting, could you please rephrase
and/or expand the above?

When duplication packets, I think we will need to call root->enqueue()
no matter what, to ensure proper accounting, and that would cause the
re-entrancy issue. What I'm missing?

Thanks,

Paolo

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Cong Wang]

On Tue, Apr 22, 2025 at 01:21:22PM +0200, Paolo Abeni wrote:
> On 4/17/25 9:23 PM, Cong Wang wrote:
> > On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote:
> >> As described in Gerrard's report [1], there are cases where netem can
> >> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
> >> qfq) break whenever the enqueue callback has reentrant behaviour.
> >> This series addresses these issues by adding extra checks that cater for
> >> these reentrant corner cases. This series has passed all relevant test
> >> cases in the TDC suite.
> >>
> >> [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/
> >>
> > 
> > I am wondering why we need to enqueue the duplicate skb before enqueuing
> > the original skb in netem? IOW, why not just swap them?
> 
> It's not clear to me what you are suggesting, could you please rephrase
> and/or expand the above?

Sure, below is the change on my mind:

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fdd79d3ccd8c..000f8138f561 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -531,21 +531,6 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return NET_XMIT_DROP;
 	}
 
-	/*
-	 * If doing duplication then re-insert at top of the
-	 * qdisc tree, since parent queuer expects that only one
-	 * skb will be queued.
-	 */
-	if (skb2) {
-		struct Qdisc *rootq = qdisc_root_bh(sch);
-		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
-
-		q->duplicate = 0;
-		rootq->enqueue(skb2, rootq, to_free);
-		q->duplicate = dupsave;
-		skb2 = NULL;
-	}
-
 	qdisc_qstats_backlog_inc(sch, skb);
 
 	cb = netem_skb_cb(skb);
@@ -613,6 +598,21 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		sch->qstats.requeues++;
 	}
 
+	/*
+	 * If doing duplication then re-insert at top of the
+	 * qdisc tree, since parent queuer expects that only one
+	 * skb will be queued.
+	 */
+	if (skb2) {
+		struct Qdisc *rootq = qdisc_root_bh(sch);
+		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
+
+		q->duplicate = 0;
+		rootq->enqueue(skb2, rootq, to_free);
+		q->duplicate = dupsave;
+		skb2 = NULL;
+	}
+
 finish_segs:
 	if (skb2)
 		__qdisc_drop(skb2, to_free);

> 
> When duplication packets, I think we will need to call root->enqueue()
> no matter what, to ensure proper accounting, and that would cause the
> re-entrancy issue. What I'm missing?

The problem here is the ordering, if we enqueue the skb2 (aka the
duplication packet) first (as what it is), the qlen is not yet increased
at this point so the qdisc is technically still empty (as we test qlen).

If we reverse that order, that is, enqueuing skb2 _after_ the original
packet, qlen should be increased by tfifo_enqueue() _before_ skb2 is
enqueue, so the qdisc is not empty for skb2 any more.

This is why I think (meaning I never test it) it could solve the problem
here and is a much simpler fix (0 line of code in delta).

Thanks!

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Cong Wang]

On Wed, Apr 23, 2025 at 01:50:50PM -0700, Cong Wang wrote:
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index fdd79d3ccd8c..000f8138f561 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -531,21 +531,6 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		return NET_XMIT_DROP;
>  	}
>  
> -	/*
> -	 * If doing duplication then re-insert at top of the
> -	 * qdisc tree, since parent queuer expects that only one
> -	 * skb will be queued.
> -	 */
> -	if (skb2) {
> -		struct Qdisc *rootq = qdisc_root_bh(sch);
> -		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
> -
> -		q->duplicate = 0;
> -		rootq->enqueue(skb2, rootq, to_free);
> -		q->duplicate = dupsave;
> -		skb2 = NULL;
> -	}
> -
>  	qdisc_qstats_backlog_inc(sch, skb);
>  
>  	cb = netem_skb_cb(skb);
> @@ -613,6 +598,21 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		sch->qstats.requeues++;
>  	}
>  
> +	/*
> +	 * If doing duplication then re-insert at top of the
> +	 * qdisc tree, since parent queuer expects that only one
> +	 * skb will be queued.
> +	 */
> +	if (skb2) {
> +		struct Qdisc *rootq = qdisc_root_bh(sch);
> +		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
> +
> +		q->duplicate = 0;
> +		rootq->enqueue(skb2, rootq, to_free);
> +		q->duplicate = dupsave;
> +		skb2 = NULL;
> +	}
> +
>  finish_segs:
>  	if (skb2)
>  		__qdisc_drop(skb2, to_free);
> 

Just FYI: I tested this patch, netem duplication still worked, I didn't
see any issue.

Thanks.

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Jakub Kicinski]

On Wed, 23 Apr 2025 16:29:38 -0700 Cong Wang wrote:
> > +	/*
> > +	 * If doing duplication then re-insert at top of the
> > +	 * qdisc tree, since parent queuer expects that only one
> > +	 * skb will be queued.
> > +	 */
> > +	if (skb2) {
> > +		struct Qdisc *rootq = qdisc_root_bh(sch);
> > +		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
> > +
> > +		q->duplicate = 0;
> > +		rootq->enqueue(skb2, rootq, to_free);
> > +		q->duplicate = dupsave;
> > +		skb2 = NULL;
> > +	}
> > +
> >  finish_segs:
> >  	if (skb2)
> >  		__qdisc_drop(skb2, to_free);
> >   
> 
> Just FYI: I tested this patch, netem duplication still worked, I didn't
> see any issue.

Does it still work if you have another layer of qdiscs in the middle?
It works if say DRR is looking at the netem directly as its child when
it does:

	first = cl->qdisc->q.qlen

but there may be another layer, the cl->qdisc may be something that
hasn't incremented its qlen, and something that has netem as its child.

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Jamal Hadi Salim]

On Wed, Apr 23, 2025 at 8:24 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 23 Apr 2025 16:29:38 -0700 Cong Wang wrote:
> > > +   /*
> > > +    * If doing duplication then re-insert at top of the
> > > +    * qdisc tree, since parent queuer expects that only one
> > > +    * skb will be queued.
> > > +    */
> > > +   if (skb2) {
> > > +           struct Qdisc *rootq = qdisc_root_bh(sch);
> > > +           u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
> > > +
> > > +           q->duplicate = 0;
> > > +           rootq->enqueue(skb2, rootq, to_free);
> > > +           q->duplicate = dupsave;
> > > +           skb2 = NULL;
> > > +   }
> > > +
> > >  finish_segs:
> > >     if (skb2)
> > >             __qdisc_drop(skb2, to_free);
> > >
> >
> > Just FYI: I tested this patch, netem duplication still worked, I didn't
> > see any issue.
>
> Does it still work if you have another layer of qdiscs in the middle?
> It works if say DRR is looking at the netem directly as its child when
> it does:
>
>         first = cl->qdisc->q.qlen
>
> but there may be another layer, the cl->qdisc may be something that
> hasn't incremented its qlen, and something that has netem as its child.

Very strong feeling it wont work in that scenario. We can double check.
Regardless - even if it did - what Victor sent is still a fix. Seems
DRR had that bug originally. And then in the true tradition of
TheLinuxWay(tm) was cutnpasted into HFSC and then the others followed.

cheers,
jamal

Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Jamal Hadi Salim]

On Thu, Apr 24, 2025 at 11:22 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Wed, Apr 23, 2025 at 8:24 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 23 Apr 2025 16:29:38 -0700 Cong Wang wrote:
> > > > +   /*
> > > > +    * If doing duplication then re-insert at top of the
> > > > +    * qdisc tree, since parent queuer expects that only one
> > > > +    * skb will be queued.
> > > > +    */
> > > > +   if (skb2) {
> > > > +           struct Qdisc *rootq = qdisc_root_bh(sch);
> > > > +           u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
> > > > +
> > > > +           q->duplicate = 0;
> > > > +           rootq->enqueue(skb2, rootq, to_free);
> > > > +           q->duplicate = dupsave;
> > > > +           skb2 = NULL;
> > > > +   }
> > > > +
> > > >  finish_segs:
> > > >     if (skb2)
> > > >             __qdisc_drop(skb2, to_free);
> > > >
> > >
> > > Just FYI: I tested this patch, netem duplication still worked, I didn't
> > > see any issue.
> >
> > Does it still work if you have another layer of qdiscs in the middle?
> > It works if say DRR is looking at the netem directly as its child when
> > it does:
> >
> >         first = cl->qdisc->q.qlen
> >
> > but there may be another layer, the cl->qdisc may be something that
> > hasn't incremented its qlen, and something that has netem as its child.
>
> Very strong feeling it wont work in that scenario. We can double check.

Verified the following breaks:

drr
  |
  |
class drr
  |
  |
drr
  |
  |
class drr
  |
  |
netem

It's probably a crazy config - but doesnt matter, the bounty types will use it.

cheers,
jamal

> Regardless - even if it did - what Victor sent is still a fix. Seems
> DRR had that bug originally. And then in the true tradition of
> TheLinuxWay(tm) was cutnpasted into HFSC and then the others followed.
>
> cheers,
> jamal