From: quanyeyang <[email protected]>
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.
Mark lock_release with TRACE_EVENT_FL_BPF_NO_LOCKDEP and temporarily
disable lockdep around bpf_prog_run_array() for that event.
Fixes: 149212f07856 ("rhashtable: add lockdep tracking to bucket
bit-spin-locks.")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=ef8d17bae14efb960935
Assisted-by: Cursor:GPT-5.6 Sol
Signed-off-by: quanyeyang <[email protected]>
---
Hi,
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].
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".
syzbot hits this via pidfs + a BPF hash map, because all rhashtable
bucket locks share one lock_class:
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]
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.
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
meaningfully -- here it only produces a false positive.
Scope of this patch (deliberately minimal):
- only lock_release is tagged;
- only the perf-event attach path (trace_call_bpf) is covered.
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.
This fixes the reported syzbot path (perf-event attach to lock_release).
[1]
https://lore.kernel.org/all/20260801-fix-rhashtable-bucket-lockdep-v1-1-15a0f8ae0...@gmail.com/
[2]
https://lore.kernel.org/r/[email protected]
---
include/linux/trace_events.h | 3 +++
include/trace/events/lock.h | 2 ++
kernel/trace/bpf_trace.c | 6 ++++++
3 files changed, 11 insertions(+)
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,
};
/*
@@ -347,6 +348,7 @@ enum {
* This is set when the custom event has not been attached
* 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 = (1 << TRACE_EVENT_FL_CAP_ANY_BIT),
@@ -360,6 +362,7 @@ enum {
TRACE_EVENT_FL_FPROBE = (1 << TRACE_EVENT_FL_FPROBE_BIT),
TRACE_EVENT_FL_CUSTOM = (1 << TRACE_EVENT_FL_CUSTOM_BIT),
TRACE_EVENT_FL_TEST_STR = (1 << TRACE_EVENT_FL_TEST_STR_BIT),
+ TRACE_EVENT_FL_BPF_NO_LOCKDEP = (1 <<
TRACE_EVENT_FL_BPF_NO_LOCKDEP_BIT),
};
#define TRACE_EVENT_FL_UKPROBE (TRACE_EVENT_FL_KPROBE | TRACE_EVENT_FL_UPROBE)
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)
);
+TRACE_EVENT_FLAGS(lock_release, TRACE_EVENT_FL_BPF_NO_LOCKDEP);
+
#ifdef CONFIG_LOCK_STAT
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>
#include <net/bpf_sk_storage.h>
@@ -110,6 +111,7 @@ static u64 bpf_uprobe_multi_entry_ip(struct bpf_run_ctx
*ctx);
*/
unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
{
+ bool no_lockdep = call->flags & TRACE_EVENT_FL_BPF_NO_LOCKDEP;
unsigned int ret;
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 = bpf_prog_run_array(rcu_dereference(call->prog_array),
ctx, bpf_prog_run);
+ if (no_lockdep)
+ lockdep_on();
rcu_read_unlock();
out:
---
base-commit: 075b74841bd0065a3bda3440873c747938e69b68
change-id: 20260803-fix-lock-tracepoint-bpf-lockdep-f93e32ea6346
Best regards,
--
quanyeyang <[email protected]>
