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

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>] [day] [month] [year] [list]

Message-Id: <1229594268.1047.3.camel@penberg-laptop>
Date:	Thu, 18 Dec 2008 11:57:48 +0200
From:	Pekka Enberg <penberg@...helsinki.fi>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Christoph Lameter <cl@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [patch v2] slub: avoid leaking caches or refcounts on sysfs
	error

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@...ux-foundation.org>
> Signed-off-by: David Rientjes <rientjes@...gle.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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/