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

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

[William Liu]

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.

[1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/

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: William Liu <will@willsroot.io>
Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>
---
 net/sched/sch_netem.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fdd79d3ccd8c..308ce6629d7e 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -973,6 +973,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
 	return 0;
 }
 
+static const struct Qdisc_class_ops netem_class_ops;
+
+static inline bool has_duplication(struct Qdisc *sch)
+{
+	struct netem_sched_data *q = qdisc_priv(sch);
+
+	return q->duplicate != 0;
+}
+
+static int check_netem_in_tree(struct Qdisc *sch, bool only_duplicating,
+			       struct netlink_ext_ack *extack)
+{
+	struct Qdisc *root, *q;
+	unsigned int i;
+
+	root = qdisc_root_sleeping(sch);
+
+	if (sch != root && root->ops->cl_ops == &netem_class_ops) {
+		if (!only_duplicating || has_duplication(root))
+			goto err;
+	}
+
+	if (!qdisc_dev(root))
+		return 0;
+
+	hash_for_each(qdisc_dev(root)->qdisc_hash, i, q, hash) {
+		if (sch != q && q->ops->cl_ops == &netem_class_ops) {
+			if (!only_duplicating || has_duplication(q))
+				goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	NL_SET_ERR_MSG(extack,
+		       "netem: cannot mix duplicating netems with other netems in tree");
+	return -EINVAL;
+}
+
 /* Parse netlink message to set options */
 static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 			struct netlink_ext_ack *extack)
@@ -1031,6 +1071,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	q->gap = qopt->gap;
 	q->counter = 0;
 	q->loss = qopt->loss;
+
+	ret = check_netem_in_tree(sch, qopt->duplicate == 0, extack);
+	if (ret)
+		goto unlock;
+
 	q->duplicate = qopt->duplicate;
 
 	/* for compatibility with earlier versions.
-- 
2.43.0

[PATCH net 2/2] selftests/tc-testing: Add tests for restrictions on netem duplication

[William Liu]

Ensure that a duplicating netem cannot exist in a tree with other netems
in both qdisc addition and change. This is meant to prevent the soft
lockup and OOM loop scenario discussed in [1].

[1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/

Signed-off-by: William Liu <will@willsroot.io>
Reviewed-by: Savino Dicanosa <savy@syst3mfailure.io>
---
 .../tc-testing/tc-tests/qdiscs/netem.json     | 41 +++++++++++++++++++
 1 file changed, 41 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..e46a97e75f4b 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,46 @@
         "teardown": [
             "$TC qdisc del dev $DUMMY handle 1: root"
         ]
+    },
+    {
+        "id": "d34d",
+        "name": "NETEM test qdisc duplication restriction in qdisc tree",
+        "category": ["qdisc", "netem"],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set lo up || true",
+            "$TC qdisc add dev lo root handle 1: netem limit 1 duplicate 100%"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev lo parent 1: handle 2: netem gap 1 limit 1 duplicate 100% delay 1us reorder 100%",
+        "expExitCode": "2",
+        "verifyCmd": "$TC -s qdisc show dev lo",
+        "matchPattern": "qdisc netem",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev lo handle 1:0 root"
+        ]
+    },
+    {
+        "id": "b33f",
+        "name": "NETEM test qdisc duplication restriction in qdisc tree in netem_change",
+        "category": ["qdisc", "netem"],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set lo up || true",
+            "$TC qdisc add dev lo root handle 1: netem limit 1",
+            "$TC qdisc add dev lo parent 1: handle 2: netem limit 1"
+        ],
+        "cmdUnderTest": "$TC qdisc change dev lo handle 1: netem duplicate 50% || $TC qdisc change dev lo handle 2: netem duplicate 50%",
+        "expExitCode": "2",
+        "verifyCmd": "$TC -s qdisc show dev lo",
+        "matchPattern": "qdisc netem",
+        "matchCount": "2",
+        "teardown": [
+            "$TC qdisc del dev lo handle 1:0 root"
+        ]
     }
 ]
-- 
2.43.0

Re: [PATCH net 2/2] selftests/tc-testing: Add tests for restrictions on netem duplication

[Jakub Kicinski]

On Sun, 22 Jun 2025 19:05:31 +0000 William Liu wrote:
> Ensure that a duplicating netem cannot exist in a tree with other netems
> in both qdisc addition and change. This is meant to prevent the soft
> lockup and OOM loop scenario discussed in [1].

An existing test seems to be failing now, please adjust it:

# -----> prepare stage *** Could not execute: "$TC qdisc add dev $DUMMY parent 1:2 handle 3:0 netem duplicate 100%"
# 
# -----> prepare stage *** Error message: "Error: netem: cannot mix duplicating netems with other netems in tree.
# "
# 
# -----> prepare stage *** Aborting test run.
# 
# 
# <_io.BufferedReader name=6> *** stdout ***
# 
# 
# <_io.BufferedReader name=19> *** stderr ***
#   File "/nipa-data/kernel/tools/testing/selftests/tc-testing/./tdc.py", line 535, in test_runner
#     res = run_one_test(pm, args, index, tidx)
#           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#   File "/nipa-data/kernel/tools/testing/selftests/tc-testing/./tdc.py", line 419, in run_one_test
#     prepare_env(tidx, args, pm, 'setup', "-----> prepare stage", tidx["setup"])
#   File "/nipa-data/kernel/tools/testing/selftests/tc-testing/./tdc.py", line 267, in prepare_env
#     raise PluginMgrTestFail(
-- 
pw-bot: cr

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

[Jamal Hadi Salim]

BTW, you did fail to test tdc like i asked you to do. It was a trap
question - if you did run it you would have caught the issue Jakub
just pointed out. Maybe i shouldnt have been so coy/evil..
Please run tdc fully..

On Sun, Jun 22, 2025 at 3:05 PM William Liu <will@willsroot.io> 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.
>
> [1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/
>
> 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: William Liu <will@willsroot.io>
> Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>
> ---
>  net/sched/sch_netem.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index fdd79d3ccd8c..308ce6629d7e 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -973,6 +973,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
>         return 0;
>  }
>
> +static const struct Qdisc_class_ops netem_class_ops;
> +
> +static inline bool has_duplication(struct Qdisc *sch)
> +{
> +       struct netem_sched_data *q = qdisc_priv(sch);
> +
> +       return q->duplicate != 0;

return q->duplicate not good enough?

> +}
> +
> +static int check_netem_in_tree(struct Qdisc *sch, bool only_duplicating,
> +                              struct netlink_ext_ack *extack)
> +{
> +       struct Qdisc *root, *q;
> +       unsigned int i;
> +

"only_duplicating" is very confusing. Why not "duplicates"?

> +       root = qdisc_root_sleeping(sch);
> +
> +       if (sch != root && root->ops->cl_ops == &netem_class_ops) {
> +               if (!only_duplicating || has_duplication(root))
> +                       goto err;
> +       }
> +
> +       if (!qdisc_dev(root))
> +               return 0;
> +
> +       hash_for_each(qdisc_dev(root)->qdisc_hash, i, q, hash) {
> +               if (sch != q && q->ops->cl_ops == &netem_class_ops) {
> +                       if (!only_duplicating || has_duplication(q))

if (duplicates || has_duplication)

> +                               goto err;
> +               }
> +       }
> +
> +       return 0;
> +
> +err:
> +       NL_SET_ERR_MSG(extack,
> +                      "netem: cannot mix duplicating netems with other netems in tree");
> +       return -EINVAL;
> +}
> +
>  /* Parse netlink message to set options */
>  static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>                         struct netlink_ext_ack *extack)
> @@ -1031,6 +1071,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>         q->gap = qopt->gap;
>         q->counter = 0;
>         q->loss = qopt->loss;
> +
> +       ret = check_netem_in_tree(sch, qopt->duplicate == 0, extack);

check_netem_in_tree(sch, qopt->duplicate, extack) ?


cheers,
jamal

> +       if (ret)
> +               goto unlock;
> +
>         q->duplicate = qopt->duplicate;
>
>         /* for compatibility with earlier versions.
> --
> 2.43.0
>
>

Re: [PATCH net 2/2] selftests/tc-testing: Add tests for restrictions on netem duplication

[Victor Nogueira]

> Ensure that a duplicating netem cannot exist in a tree with other netems
> in both qdisc addition and change. This is meant to prevent the soft
> lockup and OOM loop scenario discussed in [1].

Can you add a new test case that checks for netem duplicate in 2 different
branches? Something similar to what test case "bf1d" is doing.

>  [...]
> 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..e46a97e75f4b 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,46 @@
>           "teardown": [
>               "$TC qdisc del dev $DUMMY handle 1: root"
>           ]
> +    },
> +    {
> +        "id": "d34d",
> +        "name": "NETEM test qdisc duplication restriction in qdisc tree",
> +        "category": ["qdisc", "netem"],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "$IP link set lo up || true",
> +            "$TC qdisc add dev lo root handle 1: netem limit 1 duplicate 100%"

It would be better to use dummy instead of lo like the other test cases in
this file.

cheers,
Victor

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

[Cong Wang]

On Sun, Jun 22, 2025 at 07:05:18PM +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 the patch. But singling out this specific case in netem does
not look like an elegant solution to me, it looks hacky.

I know you probably discussed this with Jamal before posting this, could
you please summarize why you decided to pick up this solution in the
changelog? It is very important for code review.

One additional comment below.

> 
> [1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/
> 
> 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: William Liu <will@willsroot.io>
> Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>
> ---
>  net/sched/sch_netem.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index fdd79d3ccd8c..308ce6629d7e 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -973,6 +973,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
>  	return 0;
>  }
>  
> +static const struct Qdisc_class_ops netem_class_ops;
> +
> +static inline bool has_duplication(struct Qdisc *sch)
> +{
> +	struct netem_sched_data *q = qdisc_priv(sch);
> +
> +	return q->duplicate != 0;
> +}

This is not worth a helper, because it only has one line and it is
actually more readable without using a helper, IMHO.

Regards,
Cong

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

[William Liu]

On Monday, June 23rd, 2025 at 2:33 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> 
> 
> BTW, you did fail to test tdc like i asked you to do. It was a trap
> question - if you did run it you would have caught the issue Jakub
> just pointed out. Maybe i shouldnt have been so coy/evil..
> Please run tdc fully..
> 

tdc has been fully run for v2 - I was under the assumption that only the netem category mattered. Is there any reason tc-tests/qdiscs/teql.json does not run under a new namepsace?

> On Sun, Jun 22, 2025 at 3:05 PM William Liu will@willsroot.io 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.
> > 
> > [1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/
> > 
> > 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: William Liu will@willsroot.io
> > Signed-off-by: Savino Dicanosa savy@syst3mfailure.io
> > ---
> > net/sched/sch_netem.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> > 
> > diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> > index fdd79d3ccd8c..308ce6629d7e 100644
> > --- a/net/sched/sch_netem.c
> > +++ b/net/sched/sch_netem.c
> > @@ -973,6 +973,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
> > return 0;
> > }
> > 
> > +static const struct Qdisc_class_ops netem_class_ops;
> > +
> > +static inline bool has_duplication(struct Qdisc *sch)
> > +{
> > + struct netem_sched_data *q = qdisc_priv(sch);
> > +
> > + return q->duplicate != 0;
> 
> 
> return q->duplicate not good enough?
> 
> > +}
> > +
> > +static int check_netem_in_tree(struct Qdisc *sch, bool only_duplicating,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct Qdisc *root, *q;
> > + unsigned int i;
> > +
> 
> 
> "only_duplicating" is very confusing. Why not "duplicates"?
> 
> > + root = qdisc_root_sleeping(sch);
> > +
> > + if (sch != root && root->ops->cl_ops == &netem_class_ops) {
> > + if (!only_duplicating || has_duplication(root))
> > + goto err;
> > + }
> > +
> > + if (!qdisc_dev(root))
> > + return 0;
> > +
> > + hash_for_each(qdisc_dev(root)->qdisc_hash, i, q, hash) {
> > + if (sch != q && q->ops->cl_ops == &netem_class_ops) {
> > + if (!only_duplicating || has_duplication(q))
> 
> 
> if (duplicates || has_duplication)
> 
> > + goto err;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + NL_SET_ERR_MSG(extack,
> > + "netem: cannot mix duplicating netems with other netems in tree");
> > + return -EINVAL;
> > +}
> > +
> > /* Parse netlink message to set options */
> > static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> > struct netlink_ext_ack *extack)
> > @@ -1031,6 +1071,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> > q->gap = qopt->gap;
> > q->counter = 0;
> > q->loss = qopt->loss;
> > +
> > + ret = check_netem_in_tree(sch, qopt->duplicate == 0, extack);
> 
> 
> check_netem_in_tree(sch, qopt->duplicate, extack) ?
> 
> 
> 
> cheers,
> jamal
> 
> > + if (ret)
> > + goto unlock;
> > +
> > q->duplicate = qopt->duplicate;
> > 
> > /* for compatibility with earlier versions.
> > --
> > 2.43.0

Ok, thank you to everyone for the helpful feedback. I have addressed everything and just posted v2.

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

[William Liu]

On Tuesday, June 24th, 2025 at 4:27 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:

> 
> 
> On Sun, Jun 22, 2025 at 07:05:18PM +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 the patch. But singling out this specific case in netem does
> not look like an elegant solution to me, it looks hacky.
> 
> I know you probably discussed this with Jamal before posting this, could
> you please summarize why you decided to pick up this solution in the
> changelog? It is very important for code review.
> 

Oh oops, I saw this email right after my reply and posting v2. I will update this in v3.

I originally suggested adding a single bit to the skb to track duplication for netem - Jamal mentioned a similar issue with a loopy DOS due to the removal of some TTL bits in the skb structure. I assumed it would be ok since it didn't change the struct size (and even if it did, it's backed by a slab allocator), but was informed that such a change would be very hard to push through.

The alternative approach is to ensure that multiple netems do not remain on the same qdisc tree path when duplication is involved. But Jamal pointed out that the issue will come up once again when filters and actions redirect packets from one path of the qdisc tree to another. 

> One additional comment below.
> 
> > [1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/
> > 
> > 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: William Liu will@willsroot.io
> > Signed-off-by: Savino Dicanosa savy@syst3mfailure.io
> > ---
> > net/sched/sch_netem.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> > 
> > diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> > index fdd79d3ccd8c..308ce6629d7e 100644
> > --- a/net/sched/sch_netem.c
> > +++ b/net/sched/sch_netem.c
> > @@ -973,6 +973,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
> > return 0;
> > }
> > 
> > +static const struct Qdisc_class_ops netem_class_ops;
> > +
> > +static inline bool has_duplication(struct Qdisc *sch)
> > +{
> > + struct netem_sched_data *q = qdisc_priv(sch);
> > +
> > + return q->duplicate != 0;
> > +}
> 
> 
> This is not worth a helper, because it only has one line and it is
> actually more readable without using a helper, IMHO.
> 
> Regards,
> Cong