[PATCH SLAB 1/2 v3] duplicate the cache name in SLUB's saved_alias list, SLAB, and SLOB

SLUB duplicates the cache name string passed into kmem_cache_create().
However if the cache could be merged to others during early boot, the
name pointer is saved in saved_alias list, and the string needs to be
kept valid before slab_sysfs_init() is finished. With this patch, the
name string (if kmalloced) could be kfreed after calling
kmem_cache_create().

Some more details:

kmem_cache_create() checks whether it is mergeable before creating one.
If not mergeable, the name is duplicated: n = kstrdup(name, GFP_KERNEL);

If it is mergeable, it calls sysfs_slab_alias(). If the sysfs is ready
(slab_state == SYSFS), then the name is duplicated (or dropped if no
SYSFS support) in sysfs_create_link() for use.

For the above cases, we could safely kfree the name string after calling
cache create. 

However, during early boot, before sysfs is ready (slab_state < SYSFS),
the sysfs_slab_alias() saves the pointer of name in the alias_list.
Those entries in the list are added to sysfs later in slab_sysfs_init()
to set up the sysfs stuff, and we need keep the name string passed in
valid until it finishes. By duplicating the name string here also, we
are able to safely kfree the name string after calling cache create.

v2: removed an unnecessary assignment in v1; some changes in change log,
added more details

v3: changed slab/slot to let them also duplicate the name string, so the
code is not slub-dependent, and in patch 2/2, we could call kfree()
after cache create without #ifdef slub.
    for slab, the name of the sizes caches created before
slab_is_available() is not duplicated, and it is not checked in
kmem_cache_destroy(), as I think these caches won't be destroyed.

Signed-off-by: Li Zhong <redacted>
---
 mm/slab.c |   15 ++++++++++++++-
 mm/slob.c |   17 ++++++++++++++---
 mm/slub.c |    7 ++++++-
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index e901a36..87df7d1 100644
--- a/mm/slab.c
+++ b/mm/slab.c

@@ -2280,6 +2280,7 @@ kmem_cache_create (const char *name, size_t size,

size_t align,
 	size_t left_over, slab_size, ralign;
 	struct kmem_cache *cachep = NULL, *pc;
 	gfp_t gfp;
+	const char *lname;
 
 	/*
 	 * Sanity checks... these are all serious usage bugs.

@@ -2291,6 +2292,13 @@ kmem_cache_create (const char *name, size_t size,

size_t align,
 		BUG();
 	}
 
+	if (slab_is_available()) {
+		lname = kstrdup(name, GFP_KERNEL);
+		if (!lname)
+			goto oops;
+	} else
+		lname = name;
+
 	/*
 	 * We use cache_chain_mutex to ensure a consistent view of
 	 * cpu_online_mask as well.  Please see cpuup_callback

@@ -2526,7 +2534,7 @@ kmem_cache_create (const char *name, size_t size,

size_t align,
 		BUG_ON(ZERO_OR_NULL_PTR(cachep->slabp_cache));
 	}
 	cachep->ctor = ctor;
-	cachep->name = name;
+	cachep->name = lname;
 
 	if (setup_cpu_cache(cachep, gfp)) {
 		__kmem_cache_destroy(cachep);

@@ -2550,6 +2558,9 @@ oops:
 	if (!cachep && (flags & SLAB_PANIC))
 		panic("kmem_cache_create(): failed to create slab `%s'\n",
 		      name);
+	if (!cachep && lname)
+		kfree(lname);
+
 	if (slab_is_available()) {
 		mutex_unlock(&cache_chain_mutex);
 		put_online_cpus();

@@ -2752,6 +2763,8 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
 	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
 		rcu_barrier();
 
+	/* sizes caches will not be destroyed? */
+	kfree(cachep->name);
 	__kmem_cache_destroy(cachep);
 	mutex_unlock(&cache_chain_mutex);
 	put_online_cpus();

diff --git a/mm/slob.c b/mm/slob.c
index 8105be4..7bea3a3 100644
--- a/mm/slob.c
+++ b/mm/slob.c

@@ -569,13 +569,18 @@ struct kmem_cache {
 struct kmem_cache *kmem_cache_create(const char *name, size_t size,
 	size_t align, unsigned long flags, void (*ctor)(void *))
 {
-	struct kmem_cache *c;
+	struct kmem_cache *c = NULL;
+	const char *lname;
+
+	lname = kstrdup(name, GFP_KERNEL);
+	if (!lname)
+		goto oops;
 
 	c = slob_alloc(sizeof(struct kmem_cache),
 		GFP_KERNEL, ARCH_KMALLOC_MINALIGN, -1);
 
 	if (c) {
-		c->name = name;
+		c->name = lname;
 		c->size = size;
 		if (flags & SLAB_DESTROY_BY_RCU) {
 			/* leave room for rcu footer at the end of object */

@@ -589,9 +594,14 @@ struct kmem_cache *kmem_cache_create(const char

*name, size_t size,
 			c->align = ARCH_SLAB_MINALIGN;
 		if (c->align < align)
 			c->align = align;
-	} else if (flags & SLAB_PANIC)
+	}
+oops:
+	if (!c && (flags & SLAB_PANIC))
 		panic("Cannot create slab cache %s\n", name);
 
+	if (!c && lname)
+		kfree(lname);
+
 	kmemleak_alloc(c, sizeof(struct kmem_cache), 1, GFP_KERNEL);
 	return c;
 }

@@ -602,6 +612,7 @@ void kmem_cache_destroy(struct kmem_cache *c)
 	kmemleak_free(c);
 	if (c->flags & SLAB_DESTROY_BY_RCU)
 		rcu_barrier();
+	kfree(c->name);
 	slob_free(c, sizeof(struct kmem_cache));
 }
 EXPORT_SYMBOL(kmem_cache_destroy);

diff --git a/mm/slub.c b/mm/slub.c
index 8c691fa..ed9f3c5 100644
--- a/mm/slub.c
+++ b/mm/slub.c

@@ -5372,7 +5372,11 @@ static int sysfs_slab_alias(struct kmem_cache *s,

const char *name)
 		return -ENOMEM;
 
 	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 +5413,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);
 	}
 

-- 
1.7.1

This patch tries to kfree the cache name of pgtables cache. It depends
on patch 1/2 -- ([PATCH SLAB 1/2 v3] duplicate the cache name in SLUB's
saved_alias list, SLAB, and SLOB) in this mail thread. 

For SLUB, as the pgtables cache might be mergeable to other caches.
During early boot, the name string is saved in the save_alias list. In
this case, the name could be safely kfreed after calling
kmem_cache_create() with patch 1.

For SLAB/SLOB, we need the changes in patch 1, which duplicates the name
strings in cache create.

v3: with patch 1/2 updated to make slab/slob consistent, #ifdef
CONFIG_SLUB is no longer needed. 

Signed-off-by: Li Zhong <redacted>
---
 arch/powerpc/mm/init_64.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 620b7ac..bc7f462 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c

@@ -130,6 +130,7 @@ void pgtable_cache_add(unsigned shift, void

(*ctor)(void *))
 	align = max_t(unsigned long, align, minalign);
 	name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
 	new = kmem_cache_create(name, table_size, align, 0, ctor);
+	kfree(name);
 	PGT_CACHE(shift) = new;
 
 	pr_debug("Allocated pgtable cache for order %d\n", shift);
-- 
1.7.1

On 07/06/2012 11:54 AM, Li Zhong wrote:

+	if (!c && lname)
+		kfree(lname);
+

kfree can still be validly called with a NULL argument. No need for the
lname in the conditional.

I thought I posted this a couple of days ago. Would this not fix things
without having to change all the allocators?


Subject: slub: Dup name earlier in kmem_cache_create

Dup the name earlier in kmem_cache_create so that alias
processing is done using the copy of the string and not
the string itself.

Signed-off-by: Christoph Lameter <redacted>

---
 mm/slub.c |   29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================

--- linux-2.6.orig/mm/slub.c	2012-06-11 08:49:56.000000000 -0500
+++ linux-2.6/mm/slub.c	2012-07-03 15:17:37.000000000 -0500

@@ -3933,8 +3933,12 @@ struct kmem_cache *kmem_cache_create(con
 	if (WARN_ON(!name))
 		return NULL;

+	n = kstrdup(name, GFP_KERNEL);
+	if (!n)
+		goto out;
+
 	down_write(&slub_lock);
-	s = find_mergeable(size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, n, ctor);
 	if (s) {
 		s->refcount++;
 		/*

@@ -3944,7 +3948,7 @@ struct kmem_cache *kmem_cache_create(con
 		s->objsize = max(s->objsize, (int)size);
 		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));

-		if (sysfs_slab_alias(s, name)) {
+		if (sysfs_slab_alias(s, n)) {
 			s->refcount--;
 			goto err;
 		}

@@ -3952,31 +3956,26 @@ struct kmem_cache *kmem_cache_create(con
 		return s;
 	}

-	n = kstrdup(name, GFP_KERNEL);
-	if (!n)
-		goto err;
-
 	s = kmalloc(kmem_size, GFP_KERNEL);
 	if (s) {
 		if (kmem_cache_open(s, n,
 				size, align, flags, ctor)) {
 			list_add(&s->list, &slab_caches);
 			up_write(&slub_lock);
-			if (sysfs_slab_add(s)) {
-				down_write(&slub_lock);
-				list_del(&s->list);
-				kfree(n);
-				kfree(s);
-				goto err;
-			}
-			return s;
+			if (!sysfs_slab_add(s))
+				return s;
+
+			down_write(&slub_lock);
+			list_del(&s->list);
 		}
 		kfree(s);
 	}
-	kfree(n);
+
 err:
+	kfree(n);
 	up_write(&slub_lock);

+out:
 	if (flags & SLAB_PANIC)
 		panic("Cannot create slabcache %s\n", name);
 	else

On Fri, 2012-07-06 at 08:56 -0500, Christoph Lameter wrote:

I thought I posted this a couple of days ago. Would this not fix things
without having to change all the allocators?

I was pointed by Glauber to the slab common code patches. I need some
more time to read the patches. Now I think the slab/slot changes in this
v3 are not needed, and can be ignored.

But for the SLUB's saved_alias list issue, I don't think the following
patch helps. Details below: (Maybe I am wrong, as I'm reading the patch
based on the 3.5-rc6 code ...)

		......
		up_write(&slub_lock);
		return s; 
	}

Here, the function returns without name string n be kfreed. 

But we couldn't kfree n here, because in sysfs_slab_alias(), if
(slab_state < SYS_FS), the name need to be kept valid until
slab_sysfs_init() is finished adding the entry into sysfs. 
		

I was pointed by Glauber to the slab common code patches. I need some
more time to read the patches. Now I think the slab/slot changes in this
v3 are not needed, and can be ignored.

That may take some kernel cycles. You have a current issue here that needs
to be fixed.

quoted

 	down_write(&slub_lock);
-	s = find_mergeable(size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, n, ctor);
 	if (s) {
 		s->refcount++;
 		/*

		......
		up_write(&slub_lock);
		return s;
	}

Here, the function returns without name string n be kfreed.

That is intentional since the string n is still referenced by the entry
that sysfs_slab_alias has created.

But we couldn't kfree n here, because in sysfs_slab_alias(), if
(slab_state < SYS_FS), the name need to be kept valid until
slab_sysfs_init() is finished adding the entry into sysfs.

Right that is why it is not freed and that is what fixes the issue you
see.

On Mon, 2012-07-09 at 09:01 -0500, Christoph Lameter wrote:

quoted

I was pointed by Glauber to the slab common code patches. I need some
more time to read the patches. Now I think the slab/slot changes in this
v3 are not needed, and can be ignored.

That may take some kernel cycles. You have a current issue here that needs
to be fixed.

I'm a little confused ... and what need I do for the next step? 

quoted

quoted

 	down_write(&slub_lock);
-	s = find_mergeable(size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, n, ctor);
 	if (s) {
 		s->refcount++;
 		/*

		......
		up_write(&slub_lock);
		return s;
	}

Here, the function returns without name string n be kfreed.

That is intentional since the string n is still referenced by the entry
that sysfs_slab_alias has created.

I'm not sure whether the "referenced by ..." you mentioned is what I
understood. From my understanding:

if slab_state == SYS_FS, after 
	return sysfs_create_link(&slab_kset->kobj, &s->kobj, name); 
is called, the name string passed in sysfs_slab_alias is no longer
referenced (sysfs_new_dirent duplicates the string for sysfs to use).

else, the name sting is referenced by 
	al->name = name;
temporarily. After slab_sysfs_init is finished, the name is not
referenced any more.

So in my patch (slub part), the string is duplicated here, and kfreed in
slab_sysfs_init.

quoted

But we couldn't kfree n here, because in sysfs_slab_alias(), if
(slab_state < SYS_FS), the name need to be kept valid until
slab_sysfs_init() is finished adding the entry into sysfs.

Right that is why it is not freed and that is what fixes the issue you
see.