Commit 39d9ab89 authored May 27, 2024 by Eric Dumazet Committed by Zhengchao Shao May 27, 2024

net/sched: Fix mirred deadlock on device recursion

mainline inclusion
from mainline-v6.9-rc5
commit 0f022d32c3eca477fbf79a205243a6123ed0fe11
category: bugfix
bugzilla: https://gitee.com/src-openeuler/kernel/issues/I9L5IO
CVE: CVE-2024-27010

Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0f022d32c3eca477fbf79a205243a6123ed0fe11



--------------------------------

When the mirred action is used on a classful egress qdisc and a packet is
mirrored or redirected to self we hit a qdisc lock deadlock.
See trace below.

[..... other info removed for brevity....]
[   82.890906]
[   82.890906] ============================================
[   82.890906] WARNING: possible recursive locking detected
[   82.890906] 6.8.0-05205-g77fadd89fe2d-dirty #213 Tainted: G        W
[   82.890906] --------------------------------------------
[   82.890906] ping/418 is trying to acquire lock:
[   82.890906] ffff888006994110 (&sch->q.lock){+.-.}-{3:3}, at:
__dev_queue_xmit+0x1778/0x3550
[   82.890906]
[   82.890906] but task is already holding lock:
[   82.890906] ffff888006994110 (&sch->q.lock){+.-.}-{3:3}, at:
__dev_queue_xmit+0x1778/0x3550
[   82.890906]
[   82.890906] other info that might help us debug this:
[   82.890906]  Possible unsafe locking scenario:
[   82.890906]
[   82.890906]        CPU0
[   82.890906]        ----
[   82.890906]   lock(&sch->q.lock);
[   82.890906]   lock(&sch->q.lock);
[   82.890906]
[   82.890906]  *** DEADLOCK ***
[   82.890906]
[..... other info removed for brevity....]

Example setup (eth0->eth0) to recreate
tc qdisc add dev eth0 root handle 1: htb default 30
tc filter add dev eth0 handle 1: protocol ip prio 2 matchall \
     action mirred egress redirect dev eth0

Another example(eth0->eth1->eth0) to recreate
tc qdisc add dev eth0 root handle 1: htb default 30
tc filter add dev eth0 handle 1: protocol ip prio 2 matchall \
     action mirred egress redirect dev eth1

tc qdisc add dev eth1 root handle 1: htb default 30
tc filter add dev eth1 handle 1: protocol ip prio 2 matchall \
     action mirred egress redirect dev eth0

We fix this by adding an owner field (CPU id) to struct Qdisc set after
root qdisc is entered. When the softirq enters it a second time, if the
qdisc owner is the same CPU, the packet is dropped to break the loop.

Reported-by: Mingshuai Ren <renmingshuai@huawei.com>
Closes: https://lore.kernel.org/netdev/20240314111713.5979-1-renmingshuai@huawei.com/


Fixes: 3bcb846c ("net: get rid of spin_trylock() in net_tx_action()")
Fixes: e578d9c0 ("net: sched: use counter to break reclassify loops")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Link: https://lore.kernel.org/r/20240415210728.36949-1-victor@mojatatu.com


Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Conflicts:

[The conflict occurs because OLK-6.6 does not have the
SKB_DROP_REASON_TC_RECLASSIFY_LOOP macro. Use SKB_DROP_REASON_TC_EGRESS
macro for instead]
Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>

parent 5a6a4ae9

include/net/sch_generic.h

+1 −0

Original line number	Diff line number	Diff line
		@@ -117,6 +117,7 @@ struct Qdisc {
		struct qdisc_skb_head q;
		struct gnet_stats_basic_sync bstats;
		struct gnet_stats_queue qstats;
		int owner;
		unsigned long state;
		unsigned long state2; /* must be written under qdisc spinlock */
		struct Qdisc *next_sched;

net/core/dev.c

+6 −0

Original line number	Diff line number	Diff line
		@@ -3818,6 +3818,10 @@ static inline int __dev_xmit_skb(struct sk_buff skb, struct Qdisc q,
		return rc;
		}

		if (unlikely(READ_ONCE(q->owner) == smp_processor_id())) {
		kfree_skb_reason(skb, SKB_DROP_REASON_TC_EGRESS);
		return NET_XMIT_DROP;
		}
		/*
		* Heuristic to force contended enqueues to serialize on a
		* separate lock before trying to get qdisc main lock.
		@@ -3857,7 +3861,9 @@ static inline int __dev_xmit_skb(struct sk_buff skb, struct Qdisc q,
		qdisc_run_end(q);
		rc = NET_XMIT_SUCCESS;
		} else {
		WRITE_ONCE(q->owner, smp_processor_id());
		rc = dev_qdisc_enqueue(skb, q, &to_free, txq);
		WRITE_ONCE(q->owner, -1);
		if (qdisc_run_begin(q)) {
		if (unlikely(contended)) {
		spin_unlock(&q->busylock);

net/sched/sch_generic.c

+1 −0

Original line number	Diff line number	Diff line
		@@ -971,6 +971,7 @@ struct Qdisc qdisc_alloc(struct netdev_queue dev_queue,
		sch->enqueue = ops->enqueue;
		sch->dequeue = ops->dequeue;
		sch->dev_queue = dev_queue;
		sch->owner = -1;
		netdev_hold(dev, &sch->dev_tracker, GFP_KERNEL);
		refcount_set(&sch->refcnt, 1);