Date:	Fri, 5 Feb 2016 17:47:55 +0300
From:	Dmitry Safonov <dsafonov@...tuozzo.com>
To:	Vladimir Davydov <vdavydov@...tuozzo.com>
CC:	<akpm@...ux-foundation.org>, <cl@...ux.com>, <penberg@...nel.org>,
	<rientjes@...gle.com>, <iamjoonsoo.kim@....com>,
	<linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
	<0x7f454c46@...il.com>
Subject: Re: [PATCHv4] mm: slab: shutdown caches only after releasing sysfs
 file

On 02/05/2016 04:22 PM, Vladimir Davydov wrote:
> On Thu, Feb 04, 2016 at 07:39:48PM +0300, Dmitry Safonov wrote:
> ...
>> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
>> index b7e57927..a6bf41a 100644
>> --- a/include/linux/slub_def.h
>> +++ b/include/linux/slub_def.h
>> @@ -103,9 +103,10 @@ struct kmem_cache {
>>   
>>   #ifdef CONFIG_SYSFS
>>   #define SLAB_SUPPORTS_SYSFS
>> -void sysfs_slab_remove(struct kmem_cache *);
>> +int sysfs_slab_remove(struct kmem_cache *);
>> +void sysfs_slab_remove_cancel(struct kmem_cache *s);
>>   #else
>> -static inline void sysfs_slab_remove(struct kmem_cache *s)
>> +void sysfs_slab_remove_cancel(struct kmem_cache *s)
>>   {
>>   }
>>   #endif
>> diff --git a/mm/slab.h b/mm/slab.h
>> index 834ad24..ec87600 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -141,7 +141,7 @@ static inline unsigned long kmem_cache_flags(unsigned long object_size,
>>   
>>   int __kmem_cache_shutdown(struct kmem_cache *);
>>   int __kmem_cache_shrink(struct kmem_cache *, bool);
>> -void slab_kmem_cache_release(struct kmem_cache *);
>> +int slab_kmem_cache_release(struct kmem_cache *);
>>   
>>   struct seq_file;
>>   struct file;
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index b50aef0..3ad3d22 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -448,33 +448,58 @@ out_unlock:
>>   }
>>   EXPORT_SYMBOL(kmem_cache_create);
>>   
>> -static int shutdown_cache(struct kmem_cache *s,
>> +static void prepare_caches_release(struct kmem_cache *s,
>>   		struct list_head *release, bool *need_rcu_barrier)
>>   {
>> -	if (__kmem_cache_shutdown(s) != 0)
>> -		return -EBUSY;
>> -
>>   	if (s->flags & SLAB_DESTROY_BY_RCU)
>>   		*need_rcu_barrier = true;
>>   
>>   	list_move(&s->list, release);
>> -	return 0;
>>   }
>>   
>> -static void release_caches(struct list_head *release, bool need_rcu_barrier)
>> +#ifdef SLAB_SUPPORTS_SYSFS
>> +#define release_one_cache sysfs_slab_remove
>> +#else
>> +#define release_one_cache slab_kmem_cache_release
>> +#endif
>> +
>> +static int release_caches_type(struct list_head *release, bool children)
>>   {
>>   	struct kmem_cache *s, *s2;
>> +	int ret = 0;
>>   
>> +	list_for_each_entry_safe(s, s2, release, list) {
>> +		if (is_root_cache(s) == children)
>> +			continue;
>> +
>> +		ret += release_one_cache(s);
>> +	}
>> +	return ret;
>> +}
>> +
>> +static void release_caches(struct list_head *release, bool need_rcu_barrier)
>> +{
>>   	if (need_rcu_barrier)
>>   		rcu_barrier();
> You must issue an rcu barrier before freeing kmem_cache structure, not
> before issuing __kmem_cache_shutdown. Otherwise a slab freed by
> __kmem_cache_shutdown might hit use-after-free bug.
Yeah, I wrongly interpreted fast reuse of deleted object.
>
>>   
>> -	list_for_each_entry_safe(s, s2, release, list) {
>> -#ifdef SLAB_SUPPORTS_SYSFS
>> -		sysfs_slab_remove(s);
>> -#else
>> -		slab_kmem_cache_release(s);
>> -#endif
>> -	}
>> +	/* remove children in the first place, remove root on success */
>> +	if (!release_caches_type(release, true))
>> +		release_caches_type(release, false);
>> +}
>> +
>> +static void release_cache_cancel(struct kmem_cache *s)
>> +{
>> +	sysfs_slab_remove_cancel(s);
>> +
>> +	get_online_cpus();
>> +	get_online_mems();
> What's point taking these locks when you just want to add a cache to the
> slab_list?
>
> Besides, if cpu/mem hotplug happens *between* prepare_cache_release and
> release_cache_cancel we'll get a cache on the list with not all per
> node/cpu structures allocated.
Oh, I see, you right.
>
>> +	mutex_lock(&slab_mutex);
>> +
>> +	list_move(&s->list, &slab_caches);
>> +
>> +	mutex_unlock(&slab_mutex);
>> +	put_online_mems();
>> +	put_online_cpus();
>>   }
>>   
>>   #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
>> @@ -589,16 +614,14 @@ void memcg_deactivate_kmem_caches(struct mem_cgroup *memcg)
>>   	put_online_cpus();
>>   }
>>   
>> -static int __shutdown_memcg_cache(struct kmem_cache *s,
>> +static void prepare_memcg_empty_caches(struct kmem_cache *s,
>>   		struct list_head *release, bool *need_rcu_barrier)
>>   {
>>   	BUG_ON(is_root_cache(s));
>>   
>> -	if (shutdown_cache(s, release, need_rcu_barrier))
>> -		return -EBUSY;
>> +	prepare_caches_release(s, release, need_rcu_barrier);
>>   
>> -	list_del(&s->memcg_params.list);
>> -	return 0;
>> +	list_del_init(&s->memcg_params.list);
> AFAICS if kmem_cache_destroy fails, you don't add per-memcg cache back
> to the list. Not good.
>
>>   }
>>   
>>   void memcg_destroy_kmem_caches(struct mem_cgroup *memcg)
>> @@ -614,11 +637,12 @@ void memcg_destroy_kmem_caches(struct mem_cgroup *memcg)
>>   	list_for_each_entry_safe(s, s2, &slab_caches, list) {
>>   		if (is_root_cache(s) || s->memcg_params.memcg != memcg)
>>   			continue;
>> +
>>   		/*
>>   		 * The cgroup is about to be freed and therefore has no charges
>>   		 * left. Hence, all its caches must be empty by now.
>>   		 */
>> -		BUG_ON(__shutdown_memcg_cache(s, &release, &need_rcu_barrier));
> It was a nice little check if everything works fine on memcg side. And
> you wiped it out. Sad.
It's still there but in form of WARN()
>
>> +		prepare_memcg_empty_caches(s, &release, &need_rcu_barrier);
>>   	}
>>   	mutex_unlock(&slab_mutex);
>>   
>> @@ -628,81 +652,53 @@ void memcg_destroy_kmem_caches(struct mem_cgroup *memcg)
>>   	release_caches(&release, need_rcu_barrier);
>>   }
>>   
>> -static int shutdown_memcg_caches(struct kmem_cache *s,
>> +static void prepare_memcg_filled_caches(struct kmem_cache *s,
>>   		struct list_head *release, bool *need_rcu_barrier)
>>   {
>>   	struct memcg_cache_array *arr;
>>   	struct kmem_cache *c, *c2;
>> -	LIST_HEAD(busy);
>> -	int i;
>>   
>>   	BUG_ON(!is_root_cache(s));
>>   
>> -	/*
>> -	 * First, shutdown active caches, i.e. caches that belong to online
>> -	 * memory cgroups.
>> -	 */
>>   	arr = rcu_dereference_protected(s->memcg_params.memcg_caches,
>>   					lockdep_is_held(&slab_mutex));
> And now what's that for?
>
>> -	for_each_memcg_cache_index(i) {
>> -		c = arr->entries[i];
>> -		if (!c)
>> -			continue;
>> -		if (__shutdown_memcg_cache(c, release, need_rcu_barrier))
>> -			/*
>> -			 * The cache still has objects. Move it to a temporary
>> -			 * list so as not to try to destroy it for a second
>> -			 * time while iterating over inactive caches below.
>> -			 */
>> -			list_move(&c->memcg_params.list, &busy);
>> -		else
>> -			/*
>> -			 * The cache is empty and will be destroyed soon. Clear
>> -			 * the pointer to it in the memcg_caches array so that
>> -			 * it will never be accessed even if the root cache
>> -			 * stays alive.
>> -			 */
>> -			arr->entries[i] = NULL;
> So you don't clean arr->entries on global cache destruction. If we fail
> to destroy a cache, this will result in use-after-free when the global
> cache gets reused.
Yes, should have clean
>
>> -	}
>>   
>> -	/*
>> -	 * Second, shutdown all caches left from memory cgroups that are now
>> -	 * offline.
>> -	 */
>> +	/* move children caches to release list */
>>   	list_for_each_entry_safe(c, c2, &s->memcg_params.list,
>>   				 memcg_params.list)
>> -		__shutdown_memcg_cache(c, release, need_rcu_barrier);
>> -
>> -	list_splice(&busy, &s->memcg_params.list);
>> +		prepare_caches_release(c, release, need_rcu_barrier);
>>   
>> -	/*
>> -	 * A cache being destroyed must be empty. In particular, this means
>> -	 * that all per memcg caches attached to it must be empty too.
>> -	 */
>> -	if (!list_empty(&s->memcg_params.list))
>> -		return -EBUSY;
>> -	return 0;
>> +	/* root cache to the same place */
>> +	prepare_caches_release(s, release, need_rcu_barrier);
>>   }
>> +
>>   #else
>> -static inline int shutdown_memcg_caches(struct kmem_cache *s,
>> -		struct list_head *release, bool *need_rcu_barrier)
>> -{
>> -	return 0;
>> -}
>> +#define prepare_memcg_filled_caches prepare_caches_release
>>   #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
>>   
>> -void slab_kmem_cache_release(struct kmem_cache *s)
>> +int slab_kmem_cache_release(struct kmem_cache *s)
>>   {
>> +	if (__kmem_cache_shutdown(s)) {
>> +		WARN(1, "release slub cache %s failed: it still has objects\n",
>> +			s->name);
>> +		release_cache_cancel(s);
>> +		return 1;
>> +	}
>> +
>> +#ifdef CONFIG_MEMCG
>> +	list_del(&s->memcg_params.list);
>> +#endif
>> +
>>   	destroy_memcg_params(s);
>>   	kfree_const(s->name);
>>   	kmem_cache_free(kmem_cache, s);
>> +	return 0;
>>   }
>>   
>>   void kmem_cache_destroy(struct kmem_cache *s)
>>   {
>>   	LIST_HEAD(release);
>>   	bool need_rcu_barrier = false;
>> -	int err;
>>   
>>   	if (unlikely(!s))
>>   		return;
>> @@ -716,15 +712,8 @@ void kmem_cache_destroy(struct kmem_cache *s)
>>   	if (s->refcount)
>>   		goto out_unlock;
>>   
>> -	err = shutdown_memcg_caches(s, &release, &need_rcu_barrier);
>> -	if (!err)
>> -		err = shutdown_cache(s, &release, &need_rcu_barrier);
>> +	prepare_memcg_filled_caches(s, &release, &need_rcu_barrier);
>>   
>> -	if (err) {
>> -		pr_err("kmem_cache_destroy %s: "
>> -		       "Slab cache still has objects\n", s->name);
>> -		dump_stack();
>> -	}
>>   out_unlock:
>>   	mutex_unlock(&slab_mutex);
>>   
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 2e1355a..373aa6d 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -5429,14 +5429,14 @@ out_del_kobj:
>>   	goto out;
>>   }
>>   
>> -void sysfs_slab_remove(struct kmem_cache *s)
>> +int sysfs_slab_remove(struct kmem_cache *s)
>>   {
>>   	if (slab_state < FULL)
>>   		/*
>>   		 * Sysfs has not been setup yet so no need to remove the
>>   		 * cache from sysfs.
>>   		 */
>> -		return;
>> +		return 0;
>>   
>>   #ifdef CONFIG_MEMCG
>>   	kset_unregister(s->memcg_kset);
>> @@ -5444,6 +5444,24 @@ void sysfs_slab_remove(struct kmem_cache *s)
>>   	kobject_uevent(&s->kobj, KOBJ_REMOVE);
>>   	kobject_del(&s->kobj);
>>   	kobject_put(&s->kobj);
>> +	return 0;
>> +}
>> +
>> +void sysfs_slab_remove_cancel(struct kmem_cache *s)
>> +{
>> +	int ret;
>> +
>> +	if (slab_state < FULL)
>> +		return;
>> +
>> +	/* tricky */
> What purpose is this comment supposed to serve for?
>
>> +	kobject_get(&s->kobj);
>> +	ret = kobject_add(&s->kobj, NULL, "%s", s->name);
> ret is not used.
>
>> +	kobject_uevent(&s->kobj, KOBJ_ADD);
>> +
>> +#ifdef CONFIG_MEMCG
>> +	s->memcg_kset = kset_create_and_add("cgroup", NULL, &s->kobj);
>> +#endif
> For per-memcg cache we must not create memcg_kset.
>
> And what if any of these functions fail? I don't think that silently
> ignoring failures is good.
I thought, it's fine after WARN try to revert everything as far as you can.
Without checking was reverting successful.
>
> Anyway, I really don't like how this patch approaches the problem. AFAIU
> we only want to postpone kmem_cache_node free until sysfs file is
> destroyed. Can't we just move the code freeing kmem_cache_node to a
> separate function which will be called from sysfs release instead of
> messing things up?
Yeah, I'll redo that way.
Thanks for a review.
>
> Thanks,
> Vladimir


-- 
Regards,
Dmitry Safonov

Powered by blists - more mailing lists