net/sched: Fix mirred deadlock on device recursion
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:3bcb846ca4
("net: get rid of spin_trylock() in net_tx_action()") Fixes:e578d9c025
("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>
This commit is contained in:
parent
83781384a9
commit
0f022d32c3
|
@ -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;
|
||||
|
|
|
@ -3775,6 +3775,10 @@ no_lock_out:
|
|||
return rc;
|
||||
}
|
||||
|
||||
if (unlikely(READ_ONCE(q->owner) == smp_processor_id())) {
|
||||
kfree_skb_reason(skb, SKB_DROP_REASON_TC_RECLASSIFY_LOOP);
|
||||
return NET_XMIT_DROP;
|
||||
}
|
||||
/*
|
||||
* Heuristic to force contended enqueues to serialize on a
|
||||
* separate lock before trying to get qdisc main lock.
|
||||
|
@ -3814,7 +3818,9 @@ no_lock_out:
|
|||
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);
|
||||
|
|
|
@ -974,6 +974,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);
|
||||
|
||||
|
|
Loading…
Reference in New Issue