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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox