From: William Liu <will@willsroot.io>
To: netdev@vger.kernel.org
Cc: jhs@mojatatu.com, xiyou.wangcong@gmail.com, victor@mojatatu.com,
pctammela@mojatatu.com, pabeni@redhat.com, kuba@kernel.org,
stephen@networkplumber.org, dcaratti@redhat.com,
savy@syst3mfailure.io, jiri@resnulli.us, davem@davemloft.net,
edumazet@google.com, horms@kernel.org,
William Liu <will@willsroot.io>
Subject: [PATCH net v5 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree
Date: Tue, 08 Jul 2025 16:43:26 +0000 [thread overview]
Message-ID: <20250708164141.875402-1-will@willsroot.io> (raw)
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.
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.
4) Prevent the addition of a netem to a qdisc tree if its ancestral
path contains a netem. However, filters and actions can cause a
packet to change paths when re-enqueued to the root from netem
duplication, leading us to the current solution: prevent a
duplicating netem from inhabiting the same tree as other netems.
[1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/
[2] https://lwn.net/Articles/719297/
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>
---
v4 -> v5: no changes, reposting per Jakub's request
v3 -> v4:
- Clarify changelog with chronological order of attempted solutions
v2 -> v3:
- Clarify reasoning for approach in changelog
- Removed has_duplication
v1 -> v2:
- Renamed only_duplicating to duplicates and invert logic for clarity
---
net/sched/sch_netem.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fdd79d3ccd8c..eafc316ae319 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -973,6 +973,41 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
return 0;
}
+static const struct Qdisc_class_ops netem_class_ops;
+
+static int check_netem_in_tree(struct Qdisc *sch, bool duplicates,
+ 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 (duplicates ||
+ ((struct netem_sched_data *)qdisc_priv(root))->duplicate)
+ 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 (duplicates ||
+ ((struct netem_sched_data *)qdisc_priv(q))->duplicate)
+ 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 +1066,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, extack);
+ if (ret)
+ goto unlock;
+
q->duplicate = qopt->duplicate;
/* for compatibility with earlier versions.
--
2.43.0
next reply other threads:[~2025-07-08 16:43 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-08 16:43 William Liu [this message]
2025-07-08 16:44 ` [PATCH net v5 2/2] selftests/tc-testing: Add tests for restrictions on netem duplication William Liu
2025-07-08 18:50 ` Jamal Hadi Salim
2025-07-08 18:50 ` [PATCH net v5 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree Jamal Hadi Salim
2025-07-08 19:42 ` This breaks netem use cases Cong Wang
2025-07-08 20:35 ` Jamal Hadi Salim
2025-07-08 21:32 ` Cong Wang
2025-07-08 22:26 ` Jamal Hadi Salim
2025-07-08 22:45 ` Stephen Hemminger
2025-07-11 5:19 ` Cong Wang
2025-07-10 8:26 ` Paolo Abeni
2025-07-11 5:44 ` Cong Wang
2025-07-11 22:55 ` [PATCH net v5 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree Jakub Kicinski
2025-07-12 16:51 ` William Liu
2025-07-14 14:58 ` Jakub Kicinski
2025-07-11 23:00 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250708164141.875402-1-will@willsroot.io \
--to=will@willsroot.io \
--cc=davem@davemloft.net \
--cc=dcaratti@redhat.com \
--cc=edumazet@google.com \
--cc=horms@kernel.org \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pctammela@mojatatu.com \
--cc=savy@syst3mfailure.io \
--cc=stephen@networkplumber.org \
--cc=victor@mojatatu.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.