linux-kernel - Re: [PATCH SLUB 1/2] duplicate the cache name in saved

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

Message-ID: <4FE84741.9000703@parallels.com>
Date:	Mon, 25 Jun 2012 15:10:57 +0400
From:	Glauber Costa <glommer@...allels.com>
To:	Li Zhong <zhong@...ux.vnet.ibm.com>
CC:	LKML <linux-kernel@...r.kernel.org>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Pekka Enberg <penberg@...nel.org>,
	Matt Mackall <mpm@...enic.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	"Paul Mackerras" <paulus@...ba.org>, linux-mm <linux-mm@...ck.org>,
	"PowerPC email list" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH SLUB 1/2] duplicate the cache name in saved_alias list

On 06/25/2012 01:53 PM, Li Zhong wrote:
> SLUB duplicates the cache name in kmem_cache_create(). However if the
> cache could be merged to others during early booting, the name pointer
> is saved in saved_alias list, and the string needs to be kept valid
> before slab_sysfs_init() is called.
>
> This patch tries to duplicate the cache name in saved_alias list, so
> that the cache name could be safely kfreed after calling
> kmem_cache_create(), if that name is kmalloced.
>
> Signed-off-by: Li Zhong <zhong@...ux.vnet.ibm.com>
> ---
>   mm/slub.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 8c691fa..3dc8ed5 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5373,6 +5373,11 @@ static int sysfs_slab_alias(struct kmem_cache *s,
> const char *name)
>
>   	al->s = s;
>   	al->name = name;
> +	al->name = kstrdup(name, GFP_KERNEL);
> +	if (!al->name) {
> +		kfree(al);
> +		return -ENOMEM;
> +	}
>   	al->next = alias_list;
>   	alias_list = al;
>   	return 0;
> @@ -5409,6 +5414,7 @@ static int __init slab_sysfs_init(void)
>   		if (err)
>   			printk(KERN_ERR "SLUB: Unable to add boot slab alias"
>   					" %s to sysfs\n", s->name);
> +		kfree(al->name);
>   		kfree(al);
>   	}
>
>

What's unsafe about the current state of affairs ?
Whenever we alias, we'll increase the reference counter.
kmem_cache_destroy will only actually destroy the structure whenever 
that refcnt reaches zero.

This means that kfree shouldn't happen until then. So what is exactly 
that you are seeing?

Now, if you ask me, keeping the name around in user-visible files like 
/proc/slabinfo for caches that are removed already can be a bit 
confusing (that is because we don't add aliases to the slab_cache list)

If you want to touch this, one thing you can do is to keep a list of 
names bundled in an alias. If an alias is removed, you free that name. 
If that name is the representative name of the bundle, you move to the 
next one.
--
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/