Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit ad340a4b authored by William Liu's avatar William Liu Committed by Greg Kroah-Hartman
Browse files

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

[ Upstream commit ec8e0e3d7adef940cdf9475e2352c0680189d14e ]

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: 0afb51e7 ("[PKT_SCHED]: netem: reinsert for duplication")
Reported-by: default avatarWilliam Liu <will@willsroot.io>
Reported-by: default avatarSavino Dicanosa <savy@syst3mfailure.io>
Signed-off-by: default avatarWilliam Liu <will@willsroot.io>
Signed-off-by: default avatarSavino Dicanosa <savy@syst3mfailure.io>
Acked-by: default avatarJamal Hadi Salim <jhs@mojatatu.com>
Link: https://patch.msgid.link/20250708164141.875402-1-will@willsroot.io


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
Signed-off-by: default avatarSasha Levin <sashal@kernel.org>
parent 39dd4db6
Loading
Loading
Loading
Loading
+40 −0
Original line number Diff line number Diff line
@@ -962,6 +962,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)
@@ -1023,6 +1058,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.