Re: [PATCH] rhashtable: use per-init-site lockdep classes for bucket locks
NeilBrown <[email protected]> Mon, 03 Aug 2026 12:00:32 +1000
| Newsgroups | org.kernel.vger.linux-crypto,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On Sun, 02 Aug 2026, Quanye Yang wrote:
> On Sat, Aug 01, 2026 at 07:12:00PM +0000, NeilBrown wrote:
> > This seems like a band-aid rather than a proper fix.
> >
> > Surely attaching a BPF program to lock_release() has potential for
> > causing all sorts of lockdep related problems. Any lock that the BPF
> > program takes can trigger something, and I find it unlikely that
> > rhashtable is the only part of BPF code that takes a lock.
> >
> > Maybe the tracepoint needs to disable lockdep while the BPF handler is
> > running, or something like that.
> >
> > But I would need a much stronger argument before I could be happy with
> > this patch.
>=20
> Thanks for the review.
>=20
> I agree that attaching BPF to lock_release() is a sharp edge: the
> tracepoint runs before __lock_release(), so the lock is still on the
> lockdep held stack, and any lock taken by the BPF program becomes a
> dependency edge. Whether that instrumentation path should be isolated
> from lockdep is a broader question, and I don't claim this patch solves
> it.
>=20
> But the syzbot warning at hand is not BPF-specific. It is a false
> positive from modeling every rhashtable bucket lock with one global
> lock_class_key. Different rhashtable instances have different physical
> bitlocks; nesting them is not recursive locking of the same lock.
This is common practice in the kernel.
All dentries for all filesystems have a d_lock with the same lock_class.
All block devices have a bd_holder_lock mutex with the same lock_class.
All kobjects have a list_lock with the same lock_class.
>=20
> I can reproduce the same "possible recursive locking detected" without
> any BPF / lock tracepoint:
>=20
> 1. init two tables at distinct rhashtable_init() call sites (ht_a, ht_b)
> 2. prefill so the insert path walks a non-empty bucket chain
> 3. in ht_a's obj_cmpfn (called under ht_a's bucket bitlock), call
> rhashtable_lookup_insert_key() on ht_b
Yes you *can* do that, but why *would* you do that?
The obj_cmpfn is meant to be an idempotent compare function.
We make no particular promises about when it will be called.
So making a change to anything in that function is ill-advised at best.
I don't think we have any interest in making change which would allow
an obj_cmpfn() to make changes to a different rhashtable.
>=20
> That is enough to hit the warning on an unpatched kernel. So disabling
> lockdep around the BPF handler would only silence one trigger for this
> class-modeling bug; it would not fix the underlying incorrect lock class
> sharing, and it would also hide real lock-order problems on that path
> (including the callback-under-bucket-lock cases that 149212f07856 wanted
> lockdep to see).
I think the main reason we added the lockdep tracking was to justify
that switching from spinlocks to bitlock didn't lose anything important.
It wouldn't be completely inappropriate to take a lock in an obj_cmpfn,
but I wouldn't normally expect it.
>=20
> The per-init-site bucket key follows the same approach as 060d4e94b8d4
> did for ht->mutex: keep one class per init site (and across resize of
> that table via SINGLE_DEPTH_NESTING), rather than one class for the
> entire kernel.
Hmmm.. I think that patch might have been a poor choice.
Commit 09ae540e1d5c ("rhashtable: drop ht->mutex in rhashtable_free_and_destr=
oy()")
landed about the same time and removed the problem instead of hiding it.
NeilBrown
>=20
> Happy to share the small local reproducer if that helps.
>=20
> On Sat, Aug 1, 2026 at 7:12=E2=80=AFPM NeilBrown <[email protected]> wrote:
> >
> > On Sat, 01 Aug 2026, [email protected] wrote:
> > > From: quanyeyang <[email protected]>
> > >
> > > All bucket tables currently share a single lockdep class. This makes
> > > lockdep conflate bucket locks from unrelated rhashtable instances.
> > >
> > > A BPF program attached to lock_release can expose this when pidfs
> > > inserts a pid. The tracepoint runs before lockdep removes the pidfs
> > > bucket lock from the task's held-lock stack. Deleting an element from
> > > a BPF RHASH map then acquires a bucket lock belonging to a different
> > > rhashtable. Since both tables use the same class, lockdep reports
> > > possible recursive locking.
> >
> > This seems like a band-aid rather than a proper fix.
> >
> > Surely attaching a BPF program to lock_release() has potential for
> > causing all sorts of lockdep related problems. Any lock that the BPF
> > program takes can trigger something, and I find it unlikely that
> > rhashtable is the only part of BPF code that takes a lock.
> >
> > Maybe the tracepoint needs to disable lockdep while the BPF handler is
> > running, or something like that.
> >
> > But I would need a much stronger argument before I could be happy with
> > this patch.
> >
> > NeilBrown
> >
> >
> > >
> > > Declare a separate bucket lock class key at each rhashtable_init() and
> > > rhltable_init() call site, alongside the mutex class key. Store the
> > > bucket key in struct rhashtable so tables created during resize keep
> > > using the same class.
> > >
> > > A targeted reproducer triggers the warning reliably before this change.
> > > After the change, the nested BPF RHASH deletion still executes, but
> > > lockdep no longer reports recursive locking.
> > >
> > > Fixes: 149212f07856 ("rhashtable: add lockdep tracking to bucket bit-sp=
in-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]>
> > > ---
> > > include/linux/rhashtable-types.h | 20 ++++++++++++++------
> > > lib/rhashtable.c | 19 +++++++++++++------
> > > 2 files changed, 27 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/include/linux/rhashtable-types.h b/include/linux/rhashtabl=
e-types.h
> > > index 57c11ec9dc64..4dea91a49ec8 100644
> > > --- a/include/linux/rhashtable-types.h
> > > +++ b/include/linux/rhashtable-types.h
> > > @@ -82,6 +82,7 @@ struct rhashtable_params {
> > > * @mutex: Mutex to protect current/future table swapping
> > > * @lock: Spin lock to protect walker list
> > > * @nelems: Number of elements in table
> > > + * @bucket_lock_key: Per-init-site lockdep class for bucket bit-locks
> > > */
> > > struct rhashtable {
> > > struct bucket_table __rcu *tbl;
> > > @@ -94,6 +95,7 @@ struct rhashtable {
> > > struct mutex mutex;
> > > spinlock_t lock;
> > > atomic_t nelems;
> > > + struct lock_class_key *bucket_lock_key;
> > > #ifdef CONFIG_MEM_ALLOC_PROFILING
> > > struct alloc_tag *alloc_tag;
> > > #endif
> > > @@ -138,23 +140,29 @@ struct rhashtable_iter {
> > >
> > > int __rhashtable_init_noprof(struct rhashtable *ht,
> > > const struct rhashtable_params *params,
> > > - struct lock_class_key *key);
> > > + struct lock_class_key *mutex_key,
> > > + struct lock_class_key *bucket_key);
> > > #define rhashtable_init_noprof(ht, params) \
> > > ({ \
> > > - static struct lock_class_key __key; \
> > > + static struct lock_class_key __mutex_key; \
> > > + static struct lock_class_key __bucket_key; \
> > > \
> > > - __rhashtable_init_noprof(ht, params, &__key); \
> > > + __rhashtable_init_noprof(ht, params, &__mutex_key, \
> > > + &__bucket_key); \
> > > })
> > > #define rhashtable_init(...) alloc_hooks(rhashtable_init_noprof(__VA_A=
RGS__))
> > >
> > > int __rhltable_init_noprof(struct rhltable *hlt,
> > > const struct rhashtable_params *params,
> > > - struct lock_class_key *key);
> > > + struct lock_class_key *mutex_key,
> > > + struct lock_class_key *bucket_key);
> > > #define rhltable_init_noprof(hlt, params) \
> > > ({ \
> > > - static struct lock_class_key __key; \
> > > + static struct lock_class_key __mutex_key; \
> > > + static struct lock_class_key __bucket_key; \
> > > \
> > > - __rhltable_init_noprof(hlt, params, &__key); \
> > > + __rhltable_init_noprof(hlt, params, &__mutex_key, \
> > > + &__bucket_key); \
> > > })
> > > #define rhltable_init(...) alloc_hooks(rhltable_init_noprof(__VA_ARG=
S__))
> > >
> > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> > > index d459bef245f4..e047ad912f0e 100644
> > > --- a/lib/rhashtable.c
> > > +++ b/lib/rhashtable.c
> > > @@ -189,7 +189,6 @@ static struct bucket_table *bucket_table_alloc(stru=
ct rhashtable *ht,
> > > struct bucket_table *tbl =3D NULL;
> > > size_t size;
> > > int i;
> > > - static struct lock_class_key __key;
> > >
> > > tbl =3D alloc_hooks_tag(ht->alloc_tag,
> > > kvmalloc_node_align_noprof(struct_size(tbl, bucke=
ts, nbuckets),
> > > @@ -205,7 +204,12 @@ static struct bucket_table *bucket_table_alloc(str=
uct rhashtable *ht,
> > > if (tbl =3D=3D NULL)
> > > return NULL;
> > >
> > > - lockdep_init_map(&tbl->dep_map, "rhashtable_bucket", &__key, 0);
> > > + /*
> > > + * Keep all bucket tables belonging to the same rhashtable in the
> > > + * per-init-site lock class, including tables created during resi=
ze.
> > > + */
> > > + lockdep_init_map(&tbl->dep_map, "rhashtable_bucket",
> > > + ht->bucket_lock_key, 0);
> > >
> > > tbl->size =3D size;
> > >
> > > @@ -1162,7 +1166,8 @@ static u32 rhashtable_jhash2(const void *key, u32=
length, u32 seed)
> > > */
> > > int __rhashtable_init_noprof(struct rhashtable *ht,
> > > const struct rhashtable_params *params,
> > > - struct lock_class_key *key)
> > > + struct lock_class_key *mutex_key,
> > > + struct lock_class_key *bucket_key)
> > > {
> > > struct bucket_table *tbl;
> > > size_t size;
> > > @@ -1172,7 +1177,8 @@ int __rhashtable_init_noprof(struct rhashtable *h=
t,
> > > return -EINVAL;
> > >
> > > memset(ht, 0, sizeof(*ht));
> > > - mutex_init_with_key(&ht->mutex, key);
> > > + mutex_init_with_key(&ht->mutex, mutex_key);
> > > + ht->bucket_lock_key =3D bucket_key;
> > > spin_lock_init(&ht->lock);
> > > memcpy(&ht->p, params, sizeof(*params));
> > >
> > > @@ -1237,11 +1243,12 @@ EXPORT_SYMBOL_GPL(__rhashtable_init_noprof);
> > > */
> > > int __rhltable_init_noprof(struct rhltable *hlt,
> > > const struct rhashtable_params *params,
> > > - struct lock_class_key *key)
> > > + struct lock_class_key *mutex_key,
> > > + struct lock_class_key *bucket_key)
> > > {
> > > int err;
> > >
> > > - err =3D __rhashtable_init_noprof(&hlt->ht, params, key);
> > > + err =3D __rhashtable_init_noprof(&hlt->ht, params, mutex_key, buc=
ket_key);
> > > hlt->ht.rhlist =3D true;
> > > return err;
> > > }
> > >
> > > ---
> > > base-commit: 0131b508c0e2489eac6e121135988f6eeb716f19
> > > change-id: 20260801-fix-rhashtable-bucket-lockdep-95e25abebeea
> > >
> > > Best regards,
> > > --
> > > quanyeyang <[email protected]>
> > >
> > >
> > >
> > >
> >
>=20