Re: [patch v2] slub: avoid leaking caches or refcounts on sysfs error

[Pekka Enberg <penberg@cs.helsinki.fi>]

Hi David,

On Wed, 2008-12-17 at 22:09 -0800, David Rientjes wrote:
> Pekka,
> 
> Here's an updated version of the patch I sent to LKML earlier this 
> evening.  It includes a kfree() for the kmalloc'd kmem_cache when the 
> sysfs link couldn't added.

Looks good to me. I queued it up for 2.6.29. Thanks!

		Pekka

> slub: avoid leaking caches or refcounts on sysfs error
> 
> If a slab cache is mergeable and the sysfs alias cannot be added, the
> target cache shall have its refcount decremented.  kmem_cache_create()
> will return NULL, so if kmem_cache_destroy() is ever called on the target
> cache, it will never be freed if the refcount has been leaked.
> 
> Likewise, if a slab cache is not mergeable and the sysfs link cannot be
> added, the new cache shall be removed from the slab_caches list.
> kmem_cache_create() will return NULL, so it will be impossible to call
> kmem_cache_destroy() on it.
> 
> Both of these operations require slub_lock since refcount of all slab
> caches and slab_caches are protected by the lock.
> 
> In the mergeable case, it would be better to restore objsize and offset
> back to their original values, but this could race with another merge
> since slub_lock was dropped.
> 
> Cc: Christoph Lameter <cl@linux-foundation.org>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/slub.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3123,8 +3123,12 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
>  		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
>  		up_write(&slub_lock);
>  
> -		if (sysfs_slab_alias(s, name))
> +		if (sysfs_slab_alias(s, name)) {
> +			down_write(&slub_lock);
> +			s->refcount--;
> +			up_write(&slub_lock);
>  			goto err;
> +		}
>  		return s;
>  	}
>  
> @@ -3134,8 +3138,13 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size,
>  				size, align, flags, ctor)) {
>  			list_add(&s->list, &slab_caches);
>  			up_write(&slub_lock);
> -			if (sysfs_slab_add(s))
> +			if (sysfs_slab_add(s)) {
> +				down_write(&slub_lock);
> +				list_del(&s->list);
> +				up_write(&slub_lock);
> +				kfree(s);
>  				goto err;
> +			}
>  			return s;
>  		}
>  		kfree(s);