net/sched: Restrict conditions for adding duplicating netems to qdisc tree

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: 0afb51e728 ("[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>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Link: https://patch.msgid.link/20250708164141.875402-1-will@willsroot.io
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
This commit is contained in:
William Liu 2025-07-08 16:43:26 +00:00 committed by Jakub Kicinski
parent 0cad34fb7c
commit ec8e0e3d7a

View File

@ -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.