linux-kernel - Re: [PATCH] mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <48b66d90-34ae-8666-d9ee-2d36c82a6653@suse.cz>
Date:   Wed, 10 Aug 2022 11:34:58 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Waiman Long <longman@...hat.com>, Christoph Lameter <cl@...ux.com>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Xin Long <lucien.xin@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/slab_common: Deleting kobject in kmem_cache_destroy()
 without holding slab_mutex/cpu_hotplug_lock

On 8/9/22 22:59, Waiman Long wrote:
> A circular locking problem is reported by lockdep due to the following
> circular locking dependency.
> 
>    +--> cpu_hotplug_lock --> slab_mutex --> kn->active#126 --+
>    |                                                         |
>    +---------------------------------------------------------+

This sounded familiar and I've found a thread from January:

https://lore.kernel.org/all/388098b2c03fbf0a732834fc01b2d875c335bc49.1642170196.git.lucien.xin@gmail.com/

But that seemed to be specific to RHEL-8 RT kernel and not reproduced 
with mainline. Is it different this time? Can you share the splats?

> One way to break this circular locking chain is to avoid holding
> cpu_hotplug_lock and slab_mutex while deleting the kobject in
> sysfs_slab_unlink() which should be equivalent to doing a write_lock
> and write_unlock pair of the kn->active virtual lock.
> 
> Since the kobject structures are not protected by slab_mutex or the
> cpu_hotplug_lock, we can certainly release those locks before doing
> the delete operation.
> 
> Move sysfs_slab_unlink() and sysfs_slab_release() to the newly
> created kmem_cache_release() and call it outside the slab_mutex &
> cpu_hotplug_lock critical sections.
> 
> Signed-off-by: Waiman Long <longman@...hat.com>
> ---
>   mm/slab_common.c | 48 +++++++++++++++++++++++++++++++-----------------
>   1 file changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 17996649cfe3..9274fb03563e 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -392,6 +392,30 @@ kmem_cache_create(const char *name, unsigned int size, unsigned int align,
>   }
>   EXPORT_SYMBOL(kmem_cache_create);
>   
> +#ifdef SLAB_SUPPORTS_SYSFS
> +/*
> + * For a given kmem_cache, kmem_cache_destroy() should only be called
> + * once or there will be a use-after-free problem. The actual deletion
> + * and release of the kobject does not need slab_mutex or cpu_hotplug_lock
> + * protection. So they are now done without holding those locks.
> + */
> +static void kmem_cache_release(struct kmem_cache *s, bool workfn)
> +{
> +	if (!workfn)
> +		sysfs_slab_unlink(s);
> +
> +	if (workfn || !(s->flags & SLAB_TYPESAFE_BY_RCU))
> +		sysfs_slab_release(s);
> +	else
> +		schedule_work(&slab_caches_to_rcu_destroy_work);
> +}
> +#else
> +static inline void kmem_cache_release(struct kmem_cache *s, bool workfn)
> +{
> +	slab_kmem_cache_release(s);
> +}
> +#endif
> +
>   static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
>   {
>   	LIST_HEAD(to_destroy);
> @@ -418,11 +442,7 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
>   	list_for_each_entry_safe(s, s2, &to_destroy, list) {
>   		debugfs_slab_release(s);
>   		kfence_shutdown_cache(s);
> -#ifdef SLAB_SUPPORTS_SYSFS
> -		sysfs_slab_release(s);
> -#else
> -		slab_kmem_cache_release(s);
> -#endif
> +		kmem_cache_release(s, true);
>   	}
>   }
>   
> @@ -437,20 +457,10 @@ static int shutdown_cache(struct kmem_cache *s)
>   	list_del(&s->list);
>   
>   	if (s->flags & SLAB_TYPESAFE_BY_RCU) {
> -#ifdef SLAB_SUPPORTS_SYSFS
> -		sysfs_slab_unlink(s);
> -#endif
>   		list_add_tail(&s->list, &slab_caches_to_rcu_destroy);
> -		schedule_work(&slab_caches_to_rcu_destroy_work);
>   	} else {
>   		kfence_shutdown_cache(s);
>   		debugfs_slab_release(s);
> -#ifdef SLAB_SUPPORTS_SYSFS
> -		sysfs_slab_unlink(s);
> -		sysfs_slab_release(s);
> -#else
> -		slab_kmem_cache_release(s);
> -#endif
>   	}
>   
>   	return 0;
> @@ -465,14 +475,16 @@ void slab_kmem_cache_release(struct kmem_cache *s)
>   
>   void kmem_cache_destroy(struct kmem_cache *s)
>   {
> +	int refcnt;
> +
>   	if (unlikely(!s) || !kasan_check_byte(s))
>   		return;
>   
>   	cpus_read_lock();
>   	mutex_lock(&slab_mutex);
>   
> -	s->refcount--;
> -	if (s->refcount)
> +	refcnt = --s->refcount;
> +	if (refcnt)
>   		goto out_unlock;
>   
>   	WARN(shutdown_cache(s),
> @@ -481,6 +493,8 @@ void kmem_cache_destroy(struct kmem_cache *s)
>   out_unlock:
>   	mutex_unlock(&slab_mutex);
>   	cpus_read_unlock();
> +	if (!refcnt)
> +		kmem_cache_release(s, false);
>   }
>   EXPORT_SYMBOL(kmem_cache_destroy);
>