Re: slab: clean up kmem_cache_create_memcg() error handling

[Dave Jones <davej@redhat.com>]

On Fri, Jan 24, 2014 at 03:33:41AM +0000, Linux Kernel wrote:
 > Gitweb:     http://git.kernel.org/linus/;a=commit;h=3965fc3652244651006ebb31c8c45318ce84818f
 > Commit:     3965fc3652244651006ebb31c8c45318ce84818f
 > Parent:     309381feaee564281c3d9e90fbca8963bb7428ad
 > Author:     Vladimir Davydov <vdavydov@parallels.com>
 > AuthorDate: Thu Jan 23 15:52:55 2014 -0800
 > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
 > CommitDate: Thu Jan 23 16:36:50 2014 -0800
 > 
 >     slab: clean up kmem_cache_create_memcg() error handling
 >     
 >     Currently kmem_cache_create_memcg() backoffs on failure inside
 >     conditionals, without using gotos.  This results in the rollback code
 >     duplication, which makes the function look cumbersome even though on
 >     error we should only free the allocated cache.  Since in the next patch
 >     I am going to add yet another rollback function call on error path
 >     there, let's employ labels instead of conditionals for undoing any
 >     changes on failure to keep things clean.

...

 > +out_unlock:
 >  	mutex_unlock(&slab_mutex);
 >  	put_online_cpus();
 >  
 >  	if (err) {
 > -
 >  		if (flags & SLAB_PANIC)
 >  			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
 >  				name, err);
 > @@ -236,11 +230,14 @@ out_locked:
 >  				name, err);
 >  			dump_stack();
 >  		}
 > -
 >  		return NULL;
 >  	}
 > -
 >  	return s;
 > +
 > +out_free_cache:
 > +	kfree(s->name);
 > +	kmem_cache_free(kmem_cache, s);
 > +	goto out_unlock;
 >  }

This is now returning a freed pointer as 's' if an error occurs.
Perhaps the patch below ?

	Dave


diff --git a/mm/slab_common.c b/mm/slab_common.c
index 8e40321da091..2c62294cee23 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -257,6 +257,7 @@ out_free_cache:
 	memcg_free_cache_params(s);
 	kfree(s->name);
 	kmem_cache_free(kmem_cache, s);
+	s = NULL;
 	goto out_unlock;
 }