Re: [PATCH] rhashtable: give each instance its own lockdep class

Re: [PATCH] rhashtable: give each instance its own lockdep class

[Herbert Xu]

On Mon, Apr 27, 2026 at 01:09:57PM +0200, Christian Brauner wrote:
> syzbot reported a possible circular locking dependency between
> &ht->mutex and fs_reclaim:
> 
>   CPU0 (kswapd0)                    CPU1 (kworker)
>   --------------                    --------------
>   fs_reclaim                        ht->mutex
>     shmem_evict_inode                 rhashtable_rehash_alloc
>       simple_xattrs_free                bucket_table_alloc(GFP_KERNEL)
>         rhashtable_free_and_destroy       __kvmalloc_node
>           mutex_lock(&ht->mutex)            might_alloc -> fs_reclaim
> 
> The two halves of the splat refer to two different events on
> &ht->mutex.
> 
> The kswapd0 path is unambiguous: shmem_evict_inode at mm/shmem.c:1429
> calls simple_xattrs_free(), which calls rhashtable_free_and_destroy()
> on the per-inode simple_xattrs rhashtable being torn down with the
> inode.
> 
> The previously-recorded ht->mutex -> fs_reclaim edge comes from
> rht_deferred_worker -> rhashtable_rehash_alloc ->
> bucket_table_alloc(GFP_KERNEL) -> __kvmalloc_node ->
> might_alloc -> fs_reclaim. That stack stops at generic library code:
> there is no subsystem-specific frame above rht_deferred_worker, so
> the splat does not identify which rhashtable's worker recorded the
> edge -- only that some rhashtable in the system did.
> 
> Whether or not that recording happened on the same simple_xattrs ht
> that is now being destroyed, the predicted deadlock cannot occur:
> rhashtable_free_and_destroy() does cancel_work_sync(&ht->run_work)
> before taking ht->mutex, so the deferred worker cannot be running on
> the instance being torn down. If the recording was on a different
> rhashtable instance, the two ht->mutex acquisitions are on distinct
> mutex objects and cannot deadlock either.
> 
> Lockdep flags a cycle regardless because mutex_init(&ht->mutex) lives
> on a single source line in rhashtable_init_noprof(), so every
> ht->mutex in the kernel shares one static lockdep class. Lockdep
> matches by class, not by instance, and collapses all of these into
> one node.
> 
> Lift the lockdep key out of rhashtable_init_noprof() and into the
> caller. The user-visible rhashtable_init_noprof() /
> rhltable_init_noprof() identifiers become macros that declare a
> per-call-site static lock_class_key.
> 
> Reported-by: syzbot+5af806780f38a5fe691f@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/69e798fe.050a0220.24bfd3.0032.GAE@google.com
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  include/linux/rhashtable-types.h | 22 ++++++++++++++++++----
>  lib/rhashtable.c                 | 17 ++++++++++-------
>  2 files changed, 28 insertions(+), 11 deletions(-)

Thanks for the patch.

But could you please try this patch and see if it also fixes
your problem?

https://patchwork.kernel.org/project/linux-crypto/patch/20260422213349.1345098-2-mikhail.v.gavrilov@gmail.com/

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] rhashtable: give each instance its own lockdep class

[Christian Brauner]

On Mon, Apr 27, 2026 at 07:29:58PM +0800, Herbert Xu wrote:
> On Mon, Apr 27, 2026 at 01:09:57PM +0200, Christian Brauner wrote:
> > syzbot reported a possible circular locking dependency between
> > &ht->mutex and fs_reclaim:
> > 
> >   CPU0 (kswapd0)                    CPU1 (kworker)
> >   --------------                    --------------
> >   fs_reclaim                        ht->mutex
> >     shmem_evict_inode                 rhashtable_rehash_alloc
> >       simple_xattrs_free                bucket_table_alloc(GFP_KERNEL)
> >         rhashtable_free_and_destroy       __kvmalloc_node
> >           mutex_lock(&ht->mutex)            might_alloc -> fs_reclaim
> > 
> > The two halves of the splat refer to two different events on
> > &ht->mutex.
> > 
> > The kswapd0 path is unambiguous: shmem_evict_inode at mm/shmem.c:1429
> > calls simple_xattrs_free(), which calls rhashtable_free_and_destroy()
> > on the per-inode simple_xattrs rhashtable being torn down with the
> > inode.
> > 
> > The previously-recorded ht->mutex -> fs_reclaim edge comes from
> > rht_deferred_worker -> rhashtable_rehash_alloc ->
> > bucket_table_alloc(GFP_KERNEL) -> __kvmalloc_node ->
> > might_alloc -> fs_reclaim. That stack stops at generic library code:
> > there is no subsystem-specific frame above rht_deferred_worker, so
> > the splat does not identify which rhashtable's worker recorded the
> > edge -- only that some rhashtable in the system did.
> > 
> > Whether or not that recording happened on the same simple_xattrs ht
> > that is now being destroyed, the predicted deadlock cannot occur:
> > rhashtable_free_and_destroy() does cancel_work_sync(&ht->run_work)
> > before taking ht->mutex, so the deferred worker cannot be running on
> > the instance being torn down. If the recording was on a different
> > rhashtable instance, the two ht->mutex acquisitions are on distinct
> > mutex objects and cannot deadlock either.
> > 
> > Lockdep flags a cycle regardless because mutex_init(&ht->mutex) lives
> > on a single source line in rhashtable_init_noprof(), so every
> > ht->mutex in the kernel shares one static lockdep class. Lockdep
> > matches by class, not by instance, and collapses all of these into
> > one node.
> > 
> > Lift the lockdep key out of rhashtable_init_noprof() and into the
> > caller. The user-visible rhashtable_init_noprof() /
> > rhltable_init_noprof() identifiers become macros that declare a
> > per-call-site static lock_class_key.
> > 
> > Reported-by: syzbot+5af806780f38a5fe691f@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/69e798fe.050a0220.24bfd3.0032.GAE@google.com
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  include/linux/rhashtable-types.h | 22 ++++++++++++++++++----
> >  lib/rhashtable.c                 | 17 ++++++++++-------
> >  2 files changed, 28 insertions(+), 11 deletions(-)
> 
> Thanks for the patch.
> 
> But could you please try this patch and see if it also fixes
> your problem?
> 
> https://patchwork.kernel.org/project/linux-crypto/patch/20260422213349.1345098-2-mikhail.v.gavrilov@gmail.com/

Possibly, I don't have a way to easily reproduce this though.
Imho, the right thing would be to have both: actual useful keyed lockdep
annotation and - if safe - dropping the mutex.

Re: [PATCH] rhashtable: give each instance its own lockdep class

[Mikhail Gavrilov]

On Mon, Apr 27, 2026 at 5:21 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > But could you please try this patch and see if it also fixes
> > your problem?
> >
> > https://patchwork.kernel.org/project/linux-crypto/patch/20260422213349.1345098-2-mikhail.v.gavrilov@gmail.com/
>
> Possibly, I don't have a way to easily reproduce this though.
> Imho, the right thing would be to have both: actual useful keyed lockdep
> annotation and - if safe - dropping the mutex.

Agreed -- the two changes are orthogonal: yours fixes the lockdep
class collapse, mine removes a mutex acquisition that has been
unnecessary since cancel_work_sync() was added in front of it.

On the safety of dropping the mutex: rhashtable_free_and_destroy()'s
documented contract already requires the caller to ensure no
concurrent operations, and cancel_work_sync(&ht->run_work) at the
top of the function quiesces the only in-library writer (the rehash
worker). After that, the tables are owned exclusively by this
function. The walks I switch from rht_dereference() to
rcu_dereference_raw() were already correct; the lockdep annotation
was just asking for a lock that no longer needs to be held.

I checked all in-tree callers of rhashtable_free_and_destroy() (and
of rhashtable_destroy(), which inlines the same teardown) for
correctness against the contract; none rely on the mutex for
serialization with anything other than the rehash worker.

I have no objection to either patch order. Happy to rebase mine on
top of yours, or for it to go in independently via Herbert's tree
once yours is merged.

-- 
Thanks,
Mikhail