Re: [PATCH] bpf: disable lockdep while running BPF on lock_release
NeilBrown <[email protected]> Tue, 04 Aug 2026 18:34:26 +1000
| Newsgroups | org.kernel.vger.linux-trace-kernel,org.kernel.vger.bpf,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On Tue, 04 Aug 2026, Justin Suess wrote: > On Mon, Aug 03, 2026 at 06:37:45PM +0800, quanyeyang via B4 Relay wrote: > > From: quanyeyang <[email protected]> > >=20 > > trace_lock_release() runs before __lock_release(), so the lock is > > still on the held stack when attached BPF programs execute. If those > > programs take another lock of the same class, lockdep reports a false > > recursive locking warning. > >=20 > > Mark lock_release with TRACE_EVENT_FL_BPF_NO_LOCKDEP and temporarily > > disable lockdep around bpf_prog_run_array() for that event. > >=20 > > Fixes: 149212f07856 ("rhashtable: add lockdep tracking to bucket bit-spin= -locks.") > > Reported-by: [email protected] > > Closes: https://syzkaller.appspot.com/bug?extid=3Def8d17bae14efb960935 > > Assisted-by: Cursor:GPT-5.6 Sol > > Signed-off-by: quanyeyang <[email protected]> > > --- > > Hi, > >=20 > > Small RFC to align on the approach before widening scope. This is the > > alternative to the rhashtable per-init-site lock-class patch [1], taking > > the direction NeilBrown floated in that thread [2]. > >=20 > > The problem: trace_lock_release() runs before __lock_release(), so the > > lock is still on lockdep's held stack when an attached BPF program runs. > > If that program takes another lock whose class collides with a held > > lock, lockdep reports a false "possible recursive locking". > >=20 > > syzbot hits this via pidfs + a BPF hash map, because all rhashtable > > bucket locks share one lock_class: > >=20 > > copy_process -> alloc_pid -> pidfs_add_pid [pidfs bucket bitlock held] > > lock_release tracepoint > > trace_call_bpf -> bpf_prog_run_array > > rhtab_map_delete_elem -> rhashtable_remove_fast -> rht_lock > > [same "rhashtable_bucket" class -> false recursion] > >=20 > > Why I pivoted from the per-class rhashtable fix: NeilBrown argued (a) > > sharing one lock_class across instances is common practice (d_lock, > > bd_holder_lock, kobject list_lock), and (b) BPF on lock_release() can > > perturb lockdep for *any* lock the program takes, not only rhashtable > > [2]. Disabling lockdep around the BPF handler addresses that broader > > surface, not just rhashtable. > >=20 > > On the concern that this hides real lock-order bugs: BPF programs are > > user-supplied, sandboxed code; their internal lock ordering is not part > > of the kernel's lock contract, and lockdep cannot validate it >=20 > BPF programs are not sandboxed. If a BPF program is able to break the > kernels locking semantics and trigger a true, non-recoverable deadlock > like this, that's a bug in the kernel. I know close enough to nothing about BPF or trace points... Can BPF programs ever block waiting for a lock that is held across a tracepoint? If so, then attaching a BPF program to that trace point could trivially cause a deadlock. If not - then how is that ensured? Thanks, NeilBrown >=20 > > meaningfully -- here it only produces a false positive. > > > This doesn't seem like the correct fix. And I'd argue it's a true > positive. >=20 > What happens if the cpu gets interrupted while lockdep is disabled? >=20 > Then we become blind to any other locking issues happening in whatever > NMI context we got plopped into because we disabled lockdep here. >=20 > It seems more prudent to fix this in rhashtable. >=20 > Like what 20b6cc34ea74 ("bpf: Avoid hashtab deadlock with map_locked") did > for hashtab and the subsequent move to rqspinlock did. Basically make > the implementation tolerant to temporary recursive deadlocks like this > by detecting it and returning an error. >=20 > Which is going to be a bit more of an endevour than is done in this > patch. >=20 > Justin > > Scope of this patch (deliberately minimal): > > - only lock_release is tagged; > > - only the perf-event attach path (trace_call_bpf) is covered. > >=20 > > Open questions I'd like to align on before doing more: > > - lock_acquire can produce a (different, ABBA-shaped) false positive > > by the same mechanism -- tag it too? > > - raw_tracepoint attaches go through __bpf_trace_run and are not > > covered -- extend there too? > > - flag vs always-off: should trace_call_bpf disable lockdep for all > > BPF programs? The flag keeps blast radius small, but the rationale > > applies generally. > >=20 > > This fixes the reported syzbot path (perf-event attach to lock_release). > >=20 > > [1] https://lore.kernel.org/all/20260801-fix-rhashtable-bucket-lockdep-v= [email protected]/ > > [2] https://lore.kernel.org/r/178572243204.3252194.4367547703856027885@n= oble.neil.brown.name > > --- > > include/linux/trace_events.h | 3 +++ > > include/trace/events/lock.h | 2 ++ > > kernel/trace/bpf_trace.c | 6 ++++++ > > 3 files changed, 11 insertions(+) > >=20 > > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > > index 308c76b57d13..6f67b5e9e38d 100644 > > --- a/include/linux/trace_events.h > > +++ b/include/linux/trace_events.h > > @@ -330,6 +330,7 @@ enum { > > TRACE_EVENT_FL_FPROBE_BIT, > > TRACE_EVENT_FL_CUSTOM_BIT, > > TRACE_EVENT_FL_TEST_STR_BIT, > > + TRACE_EVENT_FL_BPF_NO_LOCKDEP_BIT, > > }; > > =20 > > /* > > @@ -347,6 +348,7 @@ enum { > > * This is set when the custom event has not been atta= ched > > * to a tracepoint yet, then it is cleared when it is. > > * TEST_STR - The event has a "%s" that points to a string outside= the event > > + * BPF_NO_LOCKDEP - Disable lockdep while running attached BPF programs > > */ > > enum { > > TRACE_EVENT_FL_CAP_ANY =3D (1 << TRACE_EVENT_FL_CAP_ANY_BIT), > > @@ -360,6 +362,7 @@ enum { > > TRACE_EVENT_FL_FPROBE =3D (1 << TRACE_EVENT_FL_FPROBE_BIT), > > TRACE_EVENT_FL_CUSTOM =3D (1 << TRACE_EVENT_FL_CUSTOM_BIT), > > TRACE_EVENT_FL_TEST_STR =3D (1 << TRACE_EVENT_FL_TEST_STR_BIT), > > + TRACE_EVENT_FL_BPF_NO_LOCKDEP =3D (1 << TRACE_EVENT_FL_BPF_NO_LOCKDEP_B= IT), > > }; > > =20 > > #define TRACE_EVENT_FL_UKPROBE (TRACE_EVENT_FL_KPROBE | TRACE_EVENT_FL_U= PROBE) > > diff --git a/include/trace/events/lock.h b/include/trace/events/lock.h > > index 1ded869cd619..5ccf5c54e3d2 100644 > > --- a/include/trace/events/lock.h > > +++ b/include/trace/events/lock.h > > @@ -72,6 +72,8 @@ DEFINE_EVENT(lock, lock_release, > > TP_ARGS(lock, ip) > > ); > > =20 > > +TRACE_EVENT_FLAGS(lock_release, TRACE_EVENT_FL_BPF_NO_LOCKDEP); > > + > > #ifdef CONFIG_LOCK_STAT > > =20 > > DEFINE_EVENT(lock, lock_contended, > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 75495a5c3507..f2460f3c860e 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -24,6 +24,7 @@ > > #include <linux/key.h> > > #include <linux/namei.h> > > #include <linux/file.h> > > +#include <linux/lockdep.h> > > =20 > > #include <net/bpf_sk_storage.h> > > =20 > > @@ -110,6 +111,7 @@ static u64 bpf_uprobe_multi_entry_ip(struct bpf_run_c= tx *ctx); > > */ > > unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx) > > { > > + bool no_lockdep =3D call->flags & TRACE_EVENT_FL_BPF_NO_LOCKDEP; > > unsigned int ret; > > =20 > > cant_sleep(); > > @@ -144,8 +146,12 @@ unsigned int trace_call_bpf(struct trace_event_call = *call, void *ctx) > > * rcu_dereference() which is accepted risk. > > */ > > rcu_read_lock(); > > + if (no_lockdep) > > + lockdep_off(); > > ret =3D bpf_prog_run_array(rcu_dereference(call->prog_array), > > ctx, bpf_prog_run); > > + if (no_lockdep) > > + lockdep_on(); > > rcu_read_unlock(); > > =20 > > out: > >=20 > > --- > > base-commit: 075b74841bd0065a3bda3440873c747938e69b68 > > change-id: 20260803-fix-lock-tracepoint-bpf-lockdep-f93e32ea6346 > >=20 > > Best regards, > > -- =20 > > quanyeyang <[email protected]> > >=20 > >=20 >=20 >=20