diff mbox series

[net-next] net/sched: fix false lockdep warning on qdisc root lock

Message ID 73065927a49619fcd60e5b765c929f899a66cd1a.1701853200.git.dcaratti@redhat.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net/sched: fix false lockdep warning on qdisc root lock | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2581 this patch: 2581
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1265 this patch: 1265
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2675 this patch: 2675
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 23 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Davide Caratti Dec. 6, 2023, 9:03 a.m. UTC
Xiumei and Cristoph reported the following lockdep splat, it complains of
the qdisc root 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 class to silence false warnings.

CC: Xiumei Mu <xmu@redhat.com>
Reported-by: Cristoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 include/net/sch_generic.h | 1 +
 net/sched/sch_generic.c   | 3 +++
 2 files changed, 4 insertions(+)

Comments

Eric Dumazet Dec. 6, 2023, 10:16 a.m. UTC | #1
On Wed, Dec 6, 2023 at 10:04 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> Xiumei and Cristoph reported the following lockdep splat, it complains of
> the qdisc root 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
>
>

Can you add a Fixes: tag ?

Also, what is the interaction with htb_set_lockdep_class_child(), have
you tried to use HTB after your patch ?

Could htb_set_lockdep_class_child() be removed ?


> CC: Xiumei Mu <xmu@redhat.com>
> Reported-by: Cristoph Paasch <cpaasch@apple.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>  include/net/sch_generic.h | 1 +
>  net/sched/sch_generic.c   | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index dcb9160e6467..a395ca76066c 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -126,6 +126,7 @@ struct Qdisc {
>
>         struct rcu_head         rcu;
>         netdevice_tracker       dev_tracker;
> +       struct lock_class_key   root_lock_key;
>         /* private data */
>         long privdata[] ____cacheline_aligned;
>  };
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 8dd0e5925342..da3e1ea42852 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -944,7 +944,9 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
>         __skb_queue_head_init(&sch->gso_skb);
>         __skb_queue_head_init(&sch->skb_bad_txq);
>         gnet_stats_basic_sync_init(&sch->bstats);
> +       lockdep_register_key(&sch->root_lock_key);
>         spin_lock_init(&sch->q.lock);
> +       lockdep_set_class(&sch->q.lock, &sch->root_lock_key);
>
>         if (ops->static_flags & TCQ_F_CPUSTATS) {
>                 sch->cpu_bstats =
> @@ -1064,6 +1066,7 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
>         if (ops->destroy)
>                 ops->destroy(qdisc);
>
> +       lockdep_unregister_key(&qdisc->root_lock_key);
>         module_put(ops->owner);
>         netdev_put(qdisc_dev(qdisc), &qdisc->dev_tracker);
>
> --
> 2.41.0
>
Eric Dumazet Dec. 6, 2023, 10:25 a.m. UTC | #2
On Wed, Dec 6, 2023 at 11:16 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Dec 6, 2023 at 10:04 AM Davide Caratti <dcaratti@redhat.com> wrote:
> >
> > Xiumei and Cristoph reported the following lockdep splat, it complains of
> > the qdisc root 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
> >
> >
>
> Can you add a Fixes: tag ?
>
> Also, what is the interaction with htb_set_lockdep_class_child(), have
> you tried to use HTB after your patch ?
>
> Could htb_set_lockdep_class_child() be removed ?
>
>
> > CC: Xiumei Mu <xmu@redhat.com>
> > Reported-by: Cristoph Paasch <cpaasch@apple.com>
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451
> > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> > ---
> >  include/net/sch_generic.h | 1 +
> >  net/sched/sch_generic.c   | 3 +++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index dcb9160e6467..a395ca76066c 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -126,6 +126,7 @@ struct Qdisc {
> >
> >         struct rcu_head         rcu;
> >         netdevice_tracker       dev_tracker;
> > +       struct lock_class_key   root_lock_key;
> >         /* private data */
> >         long privdata[] ____cacheline_aligned;
> >  };
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 8dd0e5925342..da3e1ea42852 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -944,7 +944,9 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
> >         __skb_queue_head_init(&sch->gso_skb);
> >         __skb_queue_head_init(&sch->skb_bad_txq);
> >         gnet_stats_basic_sync_init(&sch->bstats);
> > +       lockdep_register_key(&sch->root_lock_key);
> >         spin_lock_init(&sch->q.lock);
> > +       lockdep_set_class(&sch->q.lock, &sch->root_lock_key);
> >
> >         if (ops->static_flags & TCQ_F_CPUSTATS) {
> >                 sch->cpu_bstats =
> > @@ -1064,6 +1066,7 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
> >         if (ops->destroy)
> >                 ops->destroy(qdisc);
> >
> > +       lockdep_unregister_key(&qdisc->root_lock_key);

lockdep_unregister_key() has a synchronize_rcu() call.

This would slow down qdisc dismantle too much.

I think we need to find another solution to this problem.
diff mbox series

Patch

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index dcb9160e6467..a395ca76066c 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -126,6 +126,7 @@  struct Qdisc {
 
 	struct rcu_head		rcu;
 	netdevice_tracker	dev_tracker;
+	struct lock_class_key	root_lock_key;
 	/* private data */
 	long privdata[] ____cacheline_aligned;
 };
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 8dd0e5925342..da3e1ea42852 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -944,7 +944,9 @@  struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
 	__skb_queue_head_init(&sch->gso_skb);
 	__skb_queue_head_init(&sch->skb_bad_txq);
 	gnet_stats_basic_sync_init(&sch->bstats);
+	lockdep_register_key(&sch->root_lock_key);
 	spin_lock_init(&sch->q.lock);
+	lockdep_set_class(&sch->q.lock, &sch->root_lock_key);
 
 	if (ops->static_flags & TCQ_F_CPUSTATS) {
 		sch->cpu_bstats =
@@ -1064,6 +1066,7 @@  static void __qdisc_destroy(struct Qdisc *qdisc)
 	if (ops->destroy)
 		ops->destroy(qdisc);
 
+	lockdep_unregister_key(&qdisc->root_lock_key);
 	module_put(ops->owner);
 	netdev_put(qdisc_dev(qdisc), &qdisc->dev_tracker);