Messages in this thread | | | Date | Tue, 29 Nov 2022 11:06:51 -0500 | Subject | Re: [net-next] bpf: avoid hashtab deadlock with try_lock | From | Waiman Long <> |
| |
On 11/29/22 07:45, Hou Tao wrote: > Hi, > > On 11/29/2022 2:06 PM, Tonghao Zhang wrote: >> On Tue, Nov 29, 2022 at 12:32 PM Hou Tao <houtao1@huawei.com> wrote: >>> Hi, >>> >>> On 11/29/2022 5:55 AM, Hao Luo wrote: >>>> On Sun, Nov 27, 2022 at 7:15 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: >>>> Hi Tonghao, >>>> >>>> With a quick look at the htab_lock_bucket() and your problem >>>> statement, I agree with Hou Tao that using hash & >>>> min(HASHTAB_MAP_LOCK_MASK, n_bucket - 1) to index in map_locked seems >>>> to fix the potential deadlock. Can you actually send your changes as >>>> v2 so we can take a look and better help you? Also, can you explain >>>> your solution in your commit message? Right now, your commit message >>>> has only a problem statement and is not very clear. Please include >>>> more details on what you do to fix the issue. >>>> >>>> Hao >>> It would be better if the test case below can be rewritten as a bpf selftests. >>> Please see comments below on how to improve it and reproduce the deadlock. >>>>> Hi >>>>> only a warning from lockdep. >>> Thanks for your details instruction. I can reproduce the warning by using your >>> setup. I am not a lockdep expert, it seems that fixing such warning needs to set >>> different lockdep class to the different bucket. Because we use map_locked to >>> protect the acquisition of bucket lock, so I think we can define lock_class_key >>> array in bpf_htab (e.g., lockdep_key[HASHTAB_MAP_LOCK_COUNT]) and initialize the >>> bucket lock accordingly. > The proposed lockdep solution doesn't work. Still got lockdep warning after > that, so cc +locking expert +lkml.org for lockdep help. > > Hi lockdep experts, > > We are trying to fix the following lockdep warning from bpf subsystem: > > [ 36.092222] ================================ > [ 36.092230] WARNING: inconsistent lock state > [ 36.092234] 6.1.0-rc5+ #81 Tainted: G E > [ 36.092236] -------------------------------- > [ 36.092237] inconsistent {INITIAL USE} -> {IN-NMI} usage. > [ 36.092238] perf/1515 [HC1[1]:SC0[0]:HE0:SE1] takes: > [ 36.092242] ffff888341acd1a0 (&htab->lockdep_key){....}-{2:2}, at: > htab_lock_bucket+0x4d/0x58 > [ 36.092253] {INITIAL USE} state was registered at: > [ 36.092255] mark_usage+0x1d/0x11d > [ 36.092262] __lock_acquire+0x3c9/0x6ed > [ 36.092266] lock_acquire+0x23d/0x29a > [ 36.092270] _raw_spin_lock_irqsave+0x43/0x7f > [ 36.092274] htab_lock_bucket+0x4d/0x58 > [ 36.092276] htab_map_delete_elem+0x82/0xfb > [ 36.092278] map_delete_elem+0x156/0x1ac > [ 36.092282] __sys_bpf+0x138/0xb71 > [ 36.092285] __do_sys_bpf+0xd/0x15 > [ 36.092288] do_syscall_64+0x6d/0x84 > [ 36.092291] entry_SYSCALL_64_after_hwframe+0x63/0xcd > [ 36.092295] irq event stamp: 120346 > [ 36.092296] hardirqs last enabled at (120345): [<ffffffff8180b97f>] > _raw_spin_unlock_irq+0x24/0x39 > [ 36.092299] hardirqs last disabled at (120346): [<ffffffff81169e85>] > generic_exec_single+0x40/0xb9 > [ 36.092303] softirqs last enabled at (120268): [<ffffffff81c00347>] > __do_softirq+0x347/0x387 > [ 36.092307] softirqs last disabled at (120133): [<ffffffff810ba4f0>] > __irq_exit_rcu+0x67/0xc6 > [ 36.092311] > [ 36.092311] other info that might help us debug this: > [ 36.092312] Possible unsafe locking scenario: > [ 36.092312] > [ 36.092313] CPU0 > [ 36.092313] ---- > [ 36.092314] lock(&htab->lockdep_key); > [ 36.092315] <Interrupt> > [ 36.092316] lock(&htab->lockdep_key); > [ 36.092318] > [ 36.092318] *** DEADLOCK *** > [ 36.092318] > [ 36.092318] 3 locks held by perf/1515: > [ 36.092320] #0: ffff8881b9805cc0 (&cpuctx_mutex){+.+.}-{4:4}, at: > perf_event_ctx_lock_nested+0x8e/0xba > [ 36.092327] #1: ffff8881075ecc20 (&event->child_mutex){+.+.}-{4:4}, at: > perf_event_for_each_child+0x35/0x76 > [ 36.092332] #2: ffff8881b9805c20 (&cpuctx_lock){-.-.}-{2:2}, at: > perf_ctx_lock+0x12/0x27 > [ 36.092339] > [ 36.092339] stack backtrace: > [ 36.092341] CPU: 0 PID: 1515 Comm: perf Tainted: G E > 6.1.0-rc5+ #81 > [ 36.092344] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014 > [ 36.092349] Call Trace: > [ 36.092351] <NMI> > [ 36.092354] dump_stack_lvl+0x57/0x81 > [ 36.092359] lock_acquire+0x1f4/0x29a > [ 36.092363] ? handle_pmi_common+0x13f/0x1f0 > [ 36.092366] ? htab_lock_bucket+0x4d/0x58 > [ 36.092371] _raw_spin_lock_irqsave+0x43/0x7f > [ 36.092374] ? htab_lock_bucket+0x4d/0x58 > [ 36.092377] htab_lock_bucket+0x4d/0x58 > [ 36.092379] htab_map_update_elem+0x11e/0x220 > [ 36.092386] bpf_prog_f3a535ca81a8128a_bpf_prog2+0x3e/0x42 > [ 36.092392] trace_call_bpf+0x177/0x215 > [ 36.092398] perf_trace_run_bpf_submit+0x52/0xaa > [ 36.092403] ? x86_pmu_stop+0x97/0x97 > [ 36.092407] perf_trace_nmi_handler+0xb7/0xe0 > [ 36.092415] nmi_handle+0x116/0x254 > [ 36.092418] ? x86_pmu_stop+0x97/0x97 > [ 36.092423] default_do_nmi+0x3d/0xf6 > [ 36.092428] exc_nmi+0xa1/0x109 > [ 36.092432] end_repeat_nmi+0x16/0x67 > [ 36.092436] RIP: 0010:wrmsrl+0xd/0x1b
So the lock is really taken in a NMI context. In general, we advise again using lock in a NMI context unless it is a lock that is used only in that context. Otherwise, deadlock is certainly a possibility as there is no way to mask off again NMI.
Cheers, Longman
| |