On Fri, Nov 8, 2024 at 6:46 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi Alexei,
>
> On 11/9/2024 4:22 AM, Alexei Starovoitov wrote:
> > On Thu, Nov 7, 2024 at 10:32 PM Kunwu Chan <kunwu.chan@xxxxxxxxx> wrote:
> >> From: Kunwu Chan <chentao@xxxxxxxxxx>
> >>
> >> When PREEMPT_RT is enabled, 'spinlock_t' becomes preemptible
> >> and bpf program has owned a raw_spinlock under a interrupt handler,
> >> which results in invalid lock acquire context.
> >>
> >> [ BUG: Invalid wait context ]
> >> 6.12.0-rc5-next-20241031-syzkaller #0 Not tainted
> >> -----------------------------
> >> swapper/0/0 is trying to lock:
> >> ffff8880261e7a00 (&trie->lock){....}-{3:3},
> >> at: trie_delete_elem+0x96/0x6a0 kernel/bpf/lpm_trie.c:462
> >> other info that might help us debug this:
> >> context-{3:3}
> >> 5 locks held by swapper/0/0:
> >> #0: ffff888020bb75c8 (&vp_dev->lock){-...}-{3:3},
> >> at: vp_vring_interrupt drivers/virtio/virtio_pci_common.c:80 [inline]
> >> #0: ffff888020bb75c8 (&vp_dev->lock){-...}-{3:3},
> >> at: vp_interrupt+0x142/0x200 drivers/virtio/virtio_pci_common.c:113
> >> #1: ffff88814174a120 (&vb->stop_update_lock){-...}-{3:3},
> >> at: spin_lock include/linux/spinlock.h:351 [inline]
> >> #1: ffff88814174a120 (&vb->stop_update_lock){-...}-{3:3},
> >> at: stats_request+0x6f/0x230 drivers/virtio/virtio_balloon.c:438
> >> #2: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: rcu_lock_acquire include/linux/rcupdate.h:337 [inline]
> >> #2: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: rcu_read_lock include/linux/rcupdate.h:849 [inline]
> >> #2: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: __queue_work+0x199/0xf50 kernel/workqueue.c:2259
> >> #3: ffff8880b863dd18 (&pool->lock){-.-.}-{2:2},
> >> at: __queue_work+0x759/0xf50
> >> #4: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: rcu_lock_acquire include/linux/rcupdate.h:337 [inline]
> >> #4: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: rcu_read_lock include/linux/rcupdate.h:849 [inline]
> >> #4: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: __bpf_trace_run kernel/trace/bpf_trace.c:2339 [inline]
> >> #4: ffffffff8e939f20 (rcu_read_lock){....}-{1:3},
> >> at: bpf_trace_run1+0x1d6/0x520 kernel/trace/bpf_trace.c:2380
> >> stack backtrace:
> >> CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
> >> 6.12.0-rc5-next-20241031-syzkaller #0
> >> Hardware name: Google Compute Engine/Google Compute Engine,
> >> BIOS Google 09/13/2024
> >> Call Trace:
> >> <IRQ>
> >> __dump_stack lib/dump_stack.c:94 [inline]
> >> dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
> >> print_lock_invalid_wait_context kernel/locking/lockdep.c:4826 [inline]
> >> check_wait_context kernel/locking/lockdep.c:4898 [inline]
> >> __lock_acquire+0x15a8/0x2100 kernel/locking/lockdep.c:5176
> >> lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5849
> >> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> >> _raw_spin_lock_irqsave+0xd5/0x120 kernel/locking/spinlock.c:162
> >> trie_delete_elem+0x96/0x6a0 kernel/bpf/lpm_trie.c:462
> > This trace is from non-RT kernel where spin_lock == raw_spin_lock.
>
> Yes. However, I think the reason for the warning is that lockdep
> considers the case is possible under PREEMPT_RT and it violates the rule
> of lock [1].
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=560af5dc839eef08a273908f390cfefefb82aa04
> >
> > I don't think Hou's explanation earlier is correct.
> > https://lore.kernel.org/bpf/e14d8882-4760-7c9c-0cfc-db04eda494ee@xxxxxxxxxxxxxxx/
>
> OK. Is the bpf mem allocator part OK for you ?
> >
> >> bpf_prog_2c29ac5cdc6b1842+0x43/0x47
> >> bpf_dispatcher_nop_func include/linux/bpf.h:1290 [inline]
> >> __bpf_prog_run include/linux/filter.h:701 [inline]
> >> bpf_prog_run include/linux/filter.h:708 [inline]
> >> __bpf_trace_run kernel/trace/bpf_trace.c:2340 [inline]
> >> bpf_trace_run1+0x2ca/0x520 kernel/trace/bpf_trace.c:2380
> >> trace_workqueue_activate_work+0x186/0x1f0 include/trace/events/workqueue.h:59
> >> __queue_work+0xc7b/0xf50 kernel/workqueue.c:2338
> >> queue_work_on+0x1c2/0x380 kernel/workqueue.c:2390
> > here irqs are disabled, but raw_spin_lock in lpm should be fine.
> >
> >> queue_work include/linux/workqueue.h:662 [inline]
> >> stats_request+0x1a3/0x230 drivers/virtio/virtio_balloon.c:441
> >> vring_interrupt+0x21d/0x380 drivers/virtio/virtio_ring.c:2595
> >> vp_vring_interrupt drivers/virtio/virtio_pci_common.c:82 [inline]
> >> vp_interrupt+0x192/0x200 drivers/virtio/virtio_pci_common.c:113
> >> __handle_irq_event_percpu+0x29a/0xa80 kernel/irq/handle.c:158
> >> handle_irq_event_percpu kernel/irq/handle.c:193 [inline]
> >> handle_irq_event+0x89/0x1f0 kernel/irq/handle.c:210
> >> handle_fasteoi_irq+0x48a/0xae0 kernel/irq/chip.c:720
> >> generic_handle_irq_desc include/linux/irqdesc.h:173 [inline]
> >> handle_irq arch/x86/kernel/irq.c:247 [inline]
> >> call_irq_handler arch/x86/kernel/irq.c:259 [inline]
> >> __common_interrupt+0x136/0x230 arch/x86/kernel/irq.c:285
> >> common_interrupt+0xb4/0xd0 arch/x86/kernel/irq.c:278
> >> </IRQ>
> >>
> >> Reported-by: syzbot+b506de56cbbb63148c33@xxxxxxxxxxxxxxxxxxxxxxxxx
> >> Closes: https://lore.kernel.org/bpf/6723db4a.050a0220.35b515.0168.GAE@xxxxxxxxxx/
> >> Fixes: 66150d0dde03 ("bpf, lpm: Make locking RT friendly")
> >> Signed-off-by: Kunwu Chan <chentao@xxxxxxxxxx>
> >> ---
> >> kernel/bpf/lpm_trie.c | 12 ++++++------
> >> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
> >> index 9b60eda0f727..373cdcfa0505 100644
> >> --- a/kernel/bpf/lpm_trie.c
> >> +++ b/kernel/bpf/lpm_trie.c
> >> @@ -35,7 +35,7 @@ struct lpm_trie {
> >> size_t n_entries;
> >> size_t max_prefixlen;
> >> size_t data_size;
> >> - spinlock_t lock;
> >> + raw_spinlock_t lock;
> >> };
> > We're certainly not going back.
>
> Only switching from spinlock_t to raw_spinlock_t is not enough, running
> it under PREEMPT_RT after the change will still trigger the similar
> lockdep warning. That is because kmalloc() may acquire a spinlock_t as
> well. However, after changing the kmalloc and its variants to bpf memory
> allocator, I think the switch to raw_spinlock_t will be safe. I have
> already written a draft patch set. Will post after after polishing and
> testing it. WDYT ?
Switching lpm to bpf_mem_alloc would address the issue.
Why do you want a switch to raw_spin_lock as well?
kfree_rcu() is already done outside of the lock.
