[PATCHv3] mm/slab: fix race with dereferencing NULL ptr in alloc_calls_show

[PATCHv3] mm/slab: fix race with dereferencing NULL ptr in alloc_calls_show

[Dmitry Safonov]

memcg_destroy_kmem_caches shutdowns in the first place kmem_caches under
slab_mutex which involves freeing NUMA node structures for kmem_cache
and only then under release_caches removes corresponding sysfs files for
these caches. Which may lead to dereferencing NULL ptr on read.
Lets remove sysfs files right there.

Fixes the following panic:
[43963.463055] BUG: unable to handle kernel
[43963.463090] NULL pointer dereference at 0000000000000020
[43963.463146] IP: [<ffffffff811c6959>] list_locations+0x169/0x4e0
[43963.463185] PGD 257304067 PUD 438456067 PMD 0
[43963.463220] Oops: 0000 [#1] SMP
[43963.463850] CPU: 3 PID: 973074 Comm: cat ve: 0 Not tainted 3.10.0-229.7.2.ovz.9.30-00007-japdoll-dirty #2 9.30
[43963.463913] Hardware name: DEPO Computers To Be Filled By O.E.M./H67DE3, BIOS L1.60c 07/14/2011
[43963.463976] task: ffff88042a5dc5b0 ti: ffff88037f8d8000 task.ti: ffff88037f8d8000
[43963.464036] RIP: 0010:[<ffffffff811c6959>]  [<ffffffff811c6959>] list_locations+0x169/0x4e0
[43963.464725] Call Trace:
[43963.464756]  [<ffffffff811c6d1d>] alloc_calls_show+0x1d/0x30
[43963.464793]  [<ffffffff811c15ab>] slab_attr_show+0x1b/0x30
[43963.464829]  [<ffffffff8125d27a>] sysfs_read_file+0x9a/0x1a0
[43963.464865]  [<ffffffff811e3c6c>] vfs_read+0x9c/0x170
[43963.464900]  [<ffffffff811e4798>] SyS_read+0x58/0xb0
[43963.464936]  [<ffffffff81612d49>] system_call_fastpath+0x16/0x1b
[43963.464970] Code: 5e 07 12 00 b9 00 04 00 00 3d 00 04 00 00 0f 4f c1 3d 00 04 00 00 89 45 b0 0f 84 c3 00 00 00 48 63 45 b0 49 8b 9c c4 f8 00 00 00 <48> 8b 43 20 48 85 c0 74 b6 48 89 df e8 46 37 44 00 48 8b 53 10
[43963.465119] RIP  [<ffffffff811c6959>] list_locations+0x169/0x4e0
[43963.465155]  RSP <ffff88037f8dbe28>
[43963.465185] CR2: 0000000000000020

Cc: Vladimir Davydov <vdavydov@virtuozzo.com>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
v2: Down with SLAB_SUPPORTS_SYSFS thing.
v3: Moved sysfs_slab_remove inside shutdown_cache

 include/linux/slub_def.h | 10 ----------
 mm/slab.h                |  8 ++++++++
 mm/slab_common.c         | 10 ++++------
 mm/slub.c                |  6 ------
 4 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index b7e57927..43634cd 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -101,16 +101,6 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 
-#ifdef CONFIG_SYSFS
-#define SLAB_SUPPORTS_SYSFS
-void sysfs_slab_remove(struct kmem_cache *);
-#else
-static inline void sysfs_slab_remove(struct kmem_cache *s)
-{
-}
-#endif
-
-
 /**
  * virt_to_obj - returns address of the beginning of object.
  * @s: object's kmem_cache
diff --git a/mm/slab.h b/mm/slab.h
index 834ad24..2983ab2 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -367,6 +367,14 @@ static inline struct kmem_cache_node *get_node(struct kmem_cache *s, int node)
 
 #endif
 
+#if defined(CONFIG_SLUB) && defined(CONFIG_SYSFS)
+void sysfs_slab_remove(struct kmem_cache *);
+#else
+static inline void sysfs_slab_remove(struct kmem_cache *s)
+{
+}
+#endif
+
 void *slab_start(struct seq_file *m, loff_t *pos);
 void *slab_next(struct seq_file *m, void *p, loff_t *pos);
 void slab_stop(struct seq_file *m, void *p);
diff --git a/mm/slab_common.c b/mm/slab_common.c
index b50aef0..2bfc0b1 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -451,6 +451,8 @@ EXPORT_SYMBOL(kmem_cache_create);
 static int shutdown_cache(struct kmem_cache *s,
 		struct list_head *release, bool *need_rcu_barrier)
 {
+	sysfs_slab_remove(s);
+
 	if (__kmem_cache_shutdown(s) != 0)
 		return -EBUSY;
 
@@ -468,13 +470,8 @@ static void release_caches(struct list_head *release, bool need_rcu_barrier)
 	if (need_rcu_barrier)
 		rcu_barrier();
 
-	list_for_each_entry_safe(s, s2, release, list) {
-#ifdef SLAB_SUPPORTS_SYSFS
-		sysfs_slab_remove(s);
-#else
+	list_for_each_entry_safe(s, s2, release, list)
 		slab_kmem_cache_release(s);
-#endif
-	}
 }
 
 #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
@@ -614,6 +611,7 @@ 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.
diff --git a/mm/slub.c b/mm/slub.c
index 2e1355a..b6a68b7 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5296,11 +5296,6 @@ static void memcg_propagate_slab_attrs(struct kmem_cache *s)
 #endif
 }
 
-static void kmem_cache_release(struct kobject *k)
-{
-	slab_kmem_cache_release(to_slab(k));
-}
-
 static const struct sysfs_ops slab_sysfs_ops = {
 	.show = slab_attr_show,
 	.store = slab_attr_store,
@@ -5308,7 +5303,6 @@ static const struct sysfs_ops slab_sysfs_ops = {
 
 static struct kobj_type slab_ktype = {
 	.sysfs_ops = &slab_sysfs_ops,
-	.release = kmem_cache_release,
 };
 
 static int uevent_filter(struct kset *kset, struct kobject *kobj)
-- 
2.7.0

Re: [PATCHv3] mm/slab: fix race with dereferencing NULL ptr in alloc_calls_show

[Vladimir Davydov]

On Wed, Feb 03, 2016 at 10:52:13AM +0300, Dmitry Safonov wrote:
...
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index b50aef0..2bfc0b1 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -451,6 +451,8 @@ EXPORT_SYMBOL(kmem_cache_create);
>  static int shutdown_cache(struct kmem_cache *s,
>  		struct list_head *release, bool *need_rcu_barrier)
>  {
> +	sysfs_slab_remove(s);
> +

shutdown_cache is called with slab_mutex held. slab_attr_store may take
the mutex. So placing sysfs_slab_remove here introduces a potential
deadlock.

>  	if (__kmem_cache_shutdown(s) != 0)
>  		return -EBUSY;
>  
> @@ -468,13 +470,8 @@ static void release_caches(struct list_head *release, bool need_rcu_barrier)
>  	if (need_rcu_barrier)
>  		rcu_barrier();
>  
> -	list_for_each_entry_safe(s, s2, release, list) {
> -#ifdef SLAB_SUPPORTS_SYSFS
> -		sysfs_slab_remove(s);
> -#else
> +	list_for_each_entry_safe(s, s2, release, list)
>  		slab_kmem_cache_release(s);
> -#endif
> -	}
>  }
>  
>  #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
> @@ -614,6 +611,7 @@ 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;
> +

Please remove this hunk.

>  		/*
>  		 * The cgroup is about to be freed and therefore has no charges
>  		 * left. Hence, all its caches must be empty by now.
> diff --git a/mm/slub.c b/mm/slub.c
> index 2e1355a..b6a68b7 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5296,11 +5296,6 @@ static void memcg_propagate_slab_attrs(struct kmem_cache *s)
>  #endif
>  }
>  
> -static void kmem_cache_release(struct kobject *k)
> -{
> -	slab_kmem_cache_release(to_slab(k));
> -}
> -
>  static const struct sysfs_ops slab_sysfs_ops = {
>  	.show = slab_attr_show,
>  	.store = slab_attr_store,
> @@ -5308,7 +5303,6 @@ static const struct sysfs_ops slab_sysfs_ops = {
>  
>  static struct kobj_type slab_ktype = {
>  	.sysfs_ops = &slab_sysfs_ops,
> -	.release = kmem_cache_release,

I surmise this will resurrect the bug that was fixed by 41a212859a4dd
("slub: use sysfs'es release mechanism for kmem_cache").

Thanks,
Vladimir

Re: [PATCHv3] mm/slab: fix race with dereferencing NULL ptr in alloc_calls_show

[Dmitry Safonov]

On 02/03/2016 12:44 PM, Vladimir Davydov wrote:
> On Wed, Feb 03, 2016 at 10:52:13AM +0300, Dmitry Safonov wrote:
> ...
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index b50aef0..2bfc0b1 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -451,6 +451,8 @@ EXPORT_SYMBOL(kmem_cache_create);
>>   static int shutdown_cache(struct kmem_cache *s,
>>   		struct list_head *release, bool *need_rcu_barrier)
>>   {
>> +	sysfs_slab_remove(s);
>> +
> shutdown_cache is called with slab_mutex held. slab_attr_store may take
> the mutex. So placing sysfs_slab_remove here introduces a potential
> deadlock.
>
>>   	if (__kmem_cache_shutdown(s) != 0)
>>   		return -EBUSY;
>>   
>> @@ -468,13 +470,8 @@ static void release_caches(struct list_head *release, bool need_rcu_barrier)
>>   	if (need_rcu_barrier)
>>   		rcu_barrier();
>>   
>> -	list_for_each_entry_safe(s, s2, release, list) {
>> -#ifdef SLAB_SUPPORTS_SYSFS
>> -		sysfs_slab_remove(s);
>> -#else
>> +	list_for_each_entry_safe(s, s2, release, list)
>>   		slab_kmem_cache_release(s);
>> -#endif
>> -	}
>>   }
>>   
>>   #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
>> @@ -614,6 +611,7 @@ 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;
>> +
> Please remove this hunk.
>
>>   		/*
>>   		 * The cgroup is about to be freed and therefore has no charges
>>   		 * left. Hence, all its caches must be empty by now.
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 2e1355a..b6a68b7 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -5296,11 +5296,6 @@ static void memcg_propagate_slab_attrs(struct kmem_cache *s)
>>   #endif
>>   }
>>   
>> -static void kmem_cache_release(struct kobject *k)
>> -{
>> -	slab_kmem_cache_release(to_slab(k));
>> -}
>> -
>>   static const struct sysfs_ops slab_sysfs_ops = {
>>   	.show = slab_attr_show,
>>   	.store = slab_attr_store,
>> @@ -5308,7 +5303,6 @@ static const struct sysfs_ops slab_sysfs_ops = {
>>   
>>   static struct kobj_type slab_ktype = {
>>   	.sysfs_ops = &slab_sysfs_ops,
>> -	.release = kmem_cache_release,
> I surmise this will resurrect the bug that was fixed by 41a212859a4dd
> ("slub: use sysfs'es release mechanism for kmem_cache").
So, I move __kmem_cache_shutdown into slab_kmem_cache_release,
release list will still be collected under slab_mutex.
Seems like no other way to do this.
Will resend mended version.
>
> Thanks,
> Vladimir


-- 
Regards,
Dmitry Safonov