From: Davide Caratti <dcaratti@redhat.com>
To: Eric Dumazet <edumazet@google.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Jiri Pirko <jiri@resnulli.us>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Naresh Kamboju <naresh.kamboju@linaro.org>,
netdev@vger.kernel.org
Subject: Re: [PATCH net-next] net/sched: unregister lockdep keys in qdisc_create/qdisc_alloc error path
Date: Tue, 30 Apr 2024 20:35:31 +0200 [thread overview]
Message-ID: <ZjE587MsVBZA61fJ@dcaratti.users.ipa.redhat.com> (raw)
In-Reply-To: <CANn89iJJefUheeur5E=bziiqxjqmKXEk3NCO=8em4XVJThExMQ@mail.gmail.com>
hi Eric, thanks for looking at this!
On Tue, Apr 30, 2024 at 07:58:14PM +0200, Eric Dumazet wrote:
> On Tue, Apr 30, 2024 at 7:11 PM Davide Caratti <dcaratti@redhat.com> wrote:
> >
[...]
> > @@ -1389,6 +1389,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> > ops->destroy(sch);
> > qdisc_put_stab(rtnl_dereference(sch->stab));
> > err_out3:
> > + lockdep_unregister_key(&sch->root_lock_key);
> > netdev_put(dev, &sch->dev_tracker);
> > qdisc_free(sch);
> > err_out2:
>
> For consistency with the other path, what about this instead ?
>
> This would also allow a qdisc goten from an rcu lookup to allow its
> spinlock to be acquired.
> (I am not saying this can happen, but who knows...)
>
> Ie defer the lockdep_unregister_key() right before the kfree()
the problem is, qdisc_free() is called also in a RCU callback. So, if we move
lockdep_unregister_key() inside the function, the non-error path is
going to splat like this:
BUG: sleeping function called from invalid context at kernel/locking/lockdep.c:6450
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/6
preempt_count: 101, expected: 0
RCU nest depth: 0, expected: 0
1 lock held by swapper/6/0:
#0: ffffffff83e03000 (rcu_callback){....}-{0:0}, at: rcu_do_batch+0x1d6/0x630
Preemption disabled at:
[<0000000000000000>] 0x0
CPU: 6 PID: 0 Comm: swapper/6 Not tainted 6.9.0-rc2+ #655
Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0 07/26/2013
Call Trace:
<IRQ>
dump_stack_lvl+0xa9/0xc0
__might_resched+0x1a6/0x2b0
? rcu_do_batch+0x208/0x630
lockdep_unregister_key+0x28/0x290
qdisc_free+0x1b/0x40
rcu_do_batch+0x20d/0x630
? lockdep_hardirqs_on+0x78/0x100
rcu_core+0x305/0x570
__do_softirq+0xcd/0x484
irq_exit_rcu+0xc9/0x110
sysvec_apic_timer_interrupt+0x9e/0xc0
</IRQ>
<TASK>
asm_sysvec_apic_timer_interrupt+0x16/0x20
RIP: 0010:cpuidle_enter_state+0x104/0x5e0
Code: 00 89 c0 48 0f a3 05 3b 3e 1e 01 0f 82 4f 03 00 00 31 ff e8 5e 9e 33 ff 45 84 ff 0f 85 ff 02 00 00 e8 60 4b 47 ff fb 45 85 f6 <0f> 88 24 02 00 00 49 63 d6 48 2b 2c 24 48 8d 04 52 48 8d 04 82 49
RSP: 0018:ffffbe4ec22e7e78 EFLAGS: 00000202
RAX: 000000000002ec55 RBX: 0000000000000004 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff838d1436 RDI: ffffffff8385cace
RBP: 0000001486cedd9b R08: 0000000000000001 R09: 0000000000000001
R10: 00004d0a32102e4a R11: ffffffff83a0bb08 R12: ffffde4abd602710
R13: ffffffff83f66f20 R14: 0000000000000004 R15: 0000000000000000
6442 void lockdep_unregister_key(struct lock_class_key *key)
6443 {
6444 struct hlist_head *hash_head = keyhashentry(key);
6445 struct lock_class_key *k;
6446 struct pending_free *pf;
6447 unsigned long flags;
6448 bool found = false;
6449
6450 might_sleep();
... because of the above line. That's why in the normal path, the dynamic key is
unregistered before scheduling the RCU callback.
--
davide
next prev parent reply other threads:[~2024-04-30 18:35 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 17:11 [PATCH net-next] net/sched: unregister lockdep keys in qdisc_create/qdisc_alloc error path Davide Caratti
2024-04-30 17:58 ` Eric Dumazet
2024-04-30 18:35 ` Davide Caratti [this message]
2024-04-30 18:43 ` Eric Dumazet
2024-05-03 12:44 ` Davide Caratti
2024-05-03 12:48 ` Eric Dumazet
2024-05-01 7:39 ` Eric Dumazet
2024-05-01 10:26 ` Ido Schimmel
2024-05-01 10:38 ` Naresh Kamboju
2024-05-02 6:22 ` Ido Schimmel
2024-05-02 14:20 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjE587MsVBZA61fJ@dcaratti.users.ipa.redhat.com \
--to=dcaratti@redhat.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=naresh.kamboju@linaro.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.