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

[Michal Hocko]

On Mon 27-04-26 13:09:57, 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/69e798fe.050a0220.24bfd3.0032.GAE@xxxxxxxxxx
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>
Thanks!

> ---
>  include/linux/rhashtable-types.h | 22 ++++++++++++++++++----
>  lib/rhashtable.c                 | 17 ++++++++++-------
>  2 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/rhashtable-types.h b/include/linux/rhashtable-types.h
> index 015c8298bebc..841021c67d3d 100644
> --- a/include/linux/rhashtable-types.h
> +++ b/include/linux/rhashtable-types.h
> @@ -131,12 +131,26 @@ struct rhashtable_iter {
>  	bool end_of_table;
>  };
>  
> -int rhashtable_init_noprof(struct rhashtable *ht,
> -		    const struct rhashtable_params *params);
> +int __rhashtable_init_noprof(struct rhashtable *ht,
> +		    const struct rhashtable_params *params,
> +		    struct lock_class_key *key);
> +#define rhashtable_init_noprof(ht, params)				\
> +({									\
> +	static struct lock_class_key __key;				\
> +									\
> +	__rhashtable_init_noprof(ht, params, &__key);			\
> +})
>  #define rhashtable_init(...)	alloc_hooks(rhashtable_init_noprof(__VA_ARGS__))
>  
> -int rhltable_init_noprof(struct rhltable *hlt,
> -		  const struct rhashtable_params *params);
> +int __rhltable_init_noprof(struct rhltable *hlt,
> +		  const struct rhashtable_params *params,
> +		  struct lock_class_key *key);
> +#define rhltable_init_noprof(hlt, params)				\
> +({									\
> +	static struct lock_class_key __key;				\
> +									\
> +	__rhltable_init_noprof(hlt, params, &__key);			\
> +})
>  #define rhltable_init(...)	alloc_hooks(rhltable_init_noprof(__VA_ARGS__))
>  
>  #endif /* _LINUX_RHASHTABLE_TYPES_H */
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index 6074ed5f66f3..fb13749d824a 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -1025,8 +1025,9 @@ static u32 rhashtable_jhash2(const void *key, u32 length, u32 seed)
>   *	.obj_hashfn = my_hash_fn,
>   * };
>   */
> -int rhashtable_init_noprof(struct rhashtable *ht,
> -		    const struct rhashtable_params *params)
> +int __rhashtable_init_noprof(struct rhashtable *ht,
> +		    const struct rhashtable_params *params,
> +		    struct lock_class_key *key)
>  {
>  	struct bucket_table *tbl;
>  	size_t size;
> @@ -1036,7 +1037,7 @@ int rhashtable_init_noprof(struct rhashtable *ht,
>  		return -EINVAL;
>  
>  	memset(ht, 0, sizeof(*ht));
> -	mutex_init(&ht->mutex);
> +	mutex_init_with_key(&ht->mutex, key);
>  	spin_lock_init(&ht->lock);
>  	memcpy(&ht->p, params, sizeof(*params));
>  
> @@ -1087,7 +1088,7 @@ int rhashtable_init_noprof(struct rhashtable *ht,
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(rhashtable_init_noprof);
> +EXPORT_SYMBOL_GPL(__rhashtable_init_noprof);
>  
>  /**
>   * rhltable_init - initialize a new hash list table
> @@ -1098,15 +1099,17 @@ EXPORT_SYMBOL_GPL(rhashtable_init_noprof);
>   *
>   * See documentation for rhashtable_init.
>   */
> -int rhltable_init_noprof(struct rhltable *hlt, const struct rhashtable_params *params)
> +int __rhltable_init_noprof(struct rhltable *hlt,
> +			   const struct rhashtable_params *params,
> +			   struct lock_class_key *key)
>  {
>  	int err;
>  
> -	err = rhashtable_init_noprof(&hlt->ht, params);
> +	err = __rhashtable_init_noprof(&hlt->ht, params, key);
>  	hlt->ht.rhlist = true;
>  	return err;
>  }
> -EXPORT_SYMBOL_GPL(rhltable_init_noprof);
> +EXPORT_SYMBOL_GPL(__rhltable_init_noprof);
>  
>  static void rhashtable_free_one(struct rhashtable *ht, struct rhash_head *obj,
>  				void (*free_fn)(void *ptr, void *arg),
> 
> ---
> base-commit: 6596a02b207886e9e00bb0161c7fd59fea53c081
> change-id: 20260427-work-rhashtable-lockdep-cb0356367073

-- 
Michal Hocko
SUSE Labs