Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

net/sched: fix false lockdep warning on qdisc root lock

Xiumei and Christoph reported the following lockdep splat, complaining of
the qdisc root lock being taken twice:

============================================
WARNING: possible recursive locking detected
6.7.0-rc3+ #598 Not tainted
--------------------------------------------
swapper/2/0 is trying to acquire lock:
ffff888177190110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70

but task is already holding lock:
ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70

other info that might help us debug this:
Possible unsafe locking scenario:

CPU0
----
lock(&sch->q.lock);
lock(&sch->q.lock);

*** DEADLOCK ***

May be due to missing lock nesting notation

5 locks held by swapper/2/0:
#0: ffff888135a09d98 ((&in_dev->mr_ifc_timer)){+.-.}-{0:0}, at: call_timer_fn+0x11a/0x510
#1: ffffffffaaee5260 (rcu_read_lock){....}-{1:2}, at: ip_finish_output2+0x2c0/0x1ed0
#2: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70
#3: ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70
#4: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70

stack backtrace:
CPU: 2 PID: 0 Comm: swapper/2 Not tainted 6.7.0-rc3+ #598
Hardware name: Red Hat KVM, BIOS 1.13.0-2.module+el8.3.0+7353+9de0a3cc 04/01/2014
Call Trace:
<IRQ>
dump_stack_lvl+0x4a/0x80
__lock_acquire+0xfdd/0x3150
lock_acquire+0x1ca/0x540
_raw_spin_lock+0x34/0x80
__dev_queue_xmit+0x1560/0x2e70
tcf_mirred_act+0x82e/0x1260 [act_mirred]
tcf_action_exec+0x161/0x480
tcf_classify+0x689/0x1170
prio_enqueue+0x316/0x660 [sch_prio]
dev_qdisc_enqueue+0x46/0x220
__dev_queue_xmit+0x1615/0x2e70
ip_finish_output2+0x1218/0x1ed0
__ip_finish_output+0x8b3/0x1350
ip_output+0x163/0x4e0
igmp_ifc_timer_expire+0x44b/0x930
call_timer_fn+0x1a2/0x510
run_timer_softirq+0x54d/0x11a0
__do_softirq+0x1b3/0x88f
irq_exit_rcu+0x18f/0x1e0
sysvec_apic_timer_interrupt+0x6f/0x90
</IRQ>

This happens when TC does a mirred egress redirect from the root qdisc of
device A to the root qdisc of device B. As long as these two locks aren't
protecting the same qdisc, they can be acquired in chain: add a per-qdisc
lockdep key to silence false warnings.
This dynamic key should safely replace the static key we have in sch_htb:
it was added to allow enqueueing to the device "direct qdisc" while still
holding the qdisc root lock.

v2: don't use static keys anymore in HTB direct qdiscs (thanks Eric Dumazet)

CC: Maxim Mikityanskiy <maxim@isovalent.com>
CC: Xiumei Mu <xmu@redhat.com>
Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Link: https://lore.kernel.org/r/7dc06d6158f72053cf877a82e2a7a5bd23692faa.1713448007.git.dcaratti@redhat.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

authored by

Davide Caratti and committed by
Paolo Abeni
af0cb3fa 1cedb16b

+7 -19
+1
include/net/sch_generic.h
··· 128 128 129 129 struct rcu_head rcu; 130 130 netdevice_tracker dev_tracker; 131 + struct lock_class_key root_lock_key; 131 132 /* private data */ 132 133 long privdata[] ____cacheline_aligned; 133 134 };
+3
net/sched/sch_generic.c
··· 945 945 __skb_queue_head_init(&sch->gso_skb); 946 946 __skb_queue_head_init(&sch->skb_bad_txq); 947 947 gnet_stats_basic_sync_init(&sch->bstats); 948 + lockdep_register_key(&sch->root_lock_key); 948 949 spin_lock_init(&sch->q.lock); 950 + lockdep_set_class(&sch->q.lock, &sch->root_lock_key); 949 951 950 952 if (ops->static_flags & TCQ_F_CPUSTATS) { 951 953 sch->cpu_bstats = ··· 1070 1068 if (ops->destroy) 1071 1069 ops->destroy(qdisc); 1072 1070 1071 + lockdep_unregister_key(&qdisc->root_lock_key); 1073 1072 module_put(ops->owner); 1074 1073 netdev_put(dev, &qdisc->dev_tracker); 1075 1074
+3 -19
net/sched/sch_htb.c
··· 1039 1039 rcu_read_unlock(); 1040 1040 } 1041 1041 1042 - static void htb_set_lockdep_class_child(struct Qdisc *q) 1043 - { 1044 - static struct lock_class_key child_key; 1045 - 1046 - lockdep_set_class(qdisc_lock(q), &child_key); 1047 - } 1048 - 1049 1042 static int htb_offload(struct net_device *dev, struct tc_htb_qopt_offload *opt) 1050 1043 { 1051 1044 return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_HTB, opt); ··· 1125 1132 return -ENOMEM; 1126 1133 } 1127 1134 1128 - htb_set_lockdep_class_child(qdisc); 1129 1135 q->direct_qdiscs[ntx] = qdisc; 1130 1136 qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT; 1131 1137 } ··· 1460 1468 } 1461 1469 1462 1470 if (q->offload) { 1463 - htb_set_lockdep_class_child(new); 1464 1471 /* One ref for cl->leaf.q, the other for dev_queue->qdisc. */ 1465 1472 qdisc_refcount_inc(new); 1466 1473 old_q = htb_graft_helper(dev_queue, new); ··· 1724 1733 new_q = qdisc_create_dflt(dev_queue, &pfifo_qdisc_ops, 1725 1734 cl->parent->common.classid, 1726 1735 NULL); 1727 - if (q->offload) { 1728 - if (new_q) 1729 - htb_set_lockdep_class_child(new_q); 1736 + if (q->offload) 1730 1737 htb_parent_to_leaf_offload(sch, dev_queue, new_q); 1731 - } 1732 1738 } 1733 1739 1734 1740 sch_tree_lock(sch); ··· 1935 1947 new_q = qdisc_create_dflt(dev_queue, &pfifo_qdisc_ops, 1936 1948 classid, NULL); 1937 1949 if (q->offload) { 1938 - if (new_q) { 1939 - htb_set_lockdep_class_child(new_q); 1940 - /* One ref for cl->leaf.q, the other for 1941 - * dev_queue->qdisc. 1942 - */ 1950 + /* One ref for cl->leaf.q, the other for dev_queue->qdisc. */ 1951 + if (new_q) 1943 1952 qdisc_refcount_inc(new_q); 1944 - } 1945 1953 old_q = htb_graft_helper(dev_queue, new_q); 1946 1954 /* No qdisc_put needed. */ 1947 1955 WARN_ON(!(old_q->flags & TCQ_F_BUILTIN));