[PATCH SLUB 1/2] duplicate the cache name in saved_alias list

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 <redacted>
---
 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);
 	}
 

-- 
1.7.1

This patch tries to kfree the cache name of pgtables cache if SLUB is
used, as SLUB duplicates the cache name, and the original one is leaked.

This patch depends on patch 1 -- (duplicate the cache name in
saved_alias list) in this mail thread. As the pgtables cache might be
merged to other caches. In this case, the name could be safely kfreed
after calling kmem_cache_create() with patch 1.

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

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

@@ -130,6 +130,9 @@ 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);
+#ifdef CONFIG_SLUB
+	kfree(name); /* SLUB duplicates the cache name */
+#endif
 	PGT_CACHE(shift) = new;
 
 	pr_debug("Allocated pgtable cache for order %d\n", shift);
-- 
1.7.1

On 06/25/2012 05:53 PM, Li Zhong wrote:

dup assigned the al->name ?


Thanks,
Wanlong Gao

On 06/25/2012 01:53 PM, Li Zhong wrote:

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.

On Mon, 2012-06-25 at 18:54 +0800, Wanlong Gao wrote:

On 06/25/2012 05:53 PM, Li Zhong wrote:

quoted

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 <redacted>
---
 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);

dup assigned the al->name ?

Ah, yes, there should be a '-' before the line al->name = name;

Thank you, I will update it. 

Thanks,
Wanlong Gao

quoted

+	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);
 	}
 

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

On Mon, 2012-06-25 at 15:10 +0400, Glauber Costa wrote:

On 06/25/2012 01:53 PM, Li Zhong wrote:

quoted

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 <redacted>
---
  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?

Maybe I didn't describe it clearly ... It is only about the name string
passed into kmem_cache_create() during early boot. 

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. And those entries in the list are added to sysfs later after
slab_sysfs_init() is called. So we need to keep the name string valid
until slab_sysfs_init() is called to set up the sysfs stuff. By
duplicating the name string here also, we are able to kfree the name
string after calling the cache create. 

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.

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

Signed-off-by: Li Zhong <redacted>
---
 mm/slub.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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.9.5

On Mon, 2012-06-25 at 17:54 +0800, Li Zhong wrote:

This is very gross ... and fragile. Also the subtle difference in
semantics between SLUB and SLAB is a VERY BAD IDEA.

I reckon you should make the other allocators all copy the name
instead.

Ben.

On 06/29/2012 08:45 AM, Benjamin Herrenschmidt wrote:

On Mon, 2012-06-25 at 17:54 +0800, Li Zhong wrote:

quoted

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

@@ -130,6 +130,9 @@ 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);
+#ifdef CONFIG_SLUB
+	kfree(name); /* SLUB duplicates the cache name */
+#endif
 	PGT_CACHE(shift) = new;
 
 	pr_debug("Allocated pgtable cache for order %d\n", shift);

This is very gross ... and fragile. Also the subtle difference in
semantics between SLUB and SLAB is a VERY BAD IDEA.

I agree.

I reckon you should make the other allocators all copy the name
instead.

Thank you for the suggestion. I will do it in the next version.

Thanks, Zhong

Ben.

On Mon, 25 Jun 2012, Li Zhong wrote:

This patch tries to kfree the cache name of pgtables cache if SLUB is
used, as SLUB duplicates the cache name, and the original one is leaked.

SLAB also does not free the name. Why would you have an #ifdef in there?

Looking through the emails it seems that there is an issue with alias
strings. That can be solved by duping the name of the slab earlier in kmem_cache_create().
Does this patch fix the issue?

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 Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

And from my understanding of the code, the saved_alias list, which is
used to keep track of the alias entries during early boot (slab_state <
SYSFS), is a blocker. It needs the name string to be valid until
slab_sysfs_init() is finished. 

That can be solved by duping the name of the slab earlier in kmem_cache_create().
Does this patch fix the issue?

I'm afraid not...

With the patch below, we still need to kfree the duplicated name in
slab_sysfs_init().

And I think it would be easier to understand if we duplicate the name
string when creating one entry for saved_alias list, and kfree it when
we remove one entry from saved_alias list. 

I'm not sure whether you got the patch #1 of the two I sent previously.
If not, would you kindly spend some time reviewing it to see if I missed
anything? Link below for your convenience:
  https://lkml.org/lkml/2012/6/27/83

Btw, as Ben suggested, I'm now working on duplicating the name string in
SLAB to make them consistent, so we don't need the #ifdef CONFIG_SLUB
any more. Will send it out for your review after it is finished.

On 07/04/2012 01:00 PM, Li Zhong wrote:

On Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

quoted

quoted

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

Out of curiosity: Why?
This is not (currently) possible with the other allocators (may change
with christoph's unification patches), so you would be making your code
slub-dependent.

On Wed, 2012-07-04 at 16:40 +0400, Glauber Costa wrote:

On 07/04/2012 01:00 PM, Li Zhong wrote:

quoted

On Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

quoted

quoted

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

Out of curiosity: Why?
This is not (currently) possible with the other allocators (may change
with christoph's unification patches), so you would be making your code
slub-dependent.

For slub itself, I think it's not good that: in some cases, the name
string could be kfreed ( if it was kmalloced ) immediately after calling
the cache create; in some other case, the name string needs to be kept
valid until some init calls finished. 

I agree with you that it would make the code slub-dependent, so I'm now
working on the consistency of the other allocators regarding this name
string duplicating thing. 

On 07/05/2012 05:41 AM, Li Zhong wrote:

On Wed, 2012-07-04 at 16:40 +0400, Glauber Costa wrote:

quoted

On 07/04/2012 01:00 PM, Li Zhong wrote:

quoted

On Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

quoted

quoted

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

Out of curiosity: Why?
This is not (currently) possible with the other allocators (may change
with christoph's unification patches), so you would be making your code
slub-dependent.

For slub itself, I think it's not good that: in some cases, the name
string could be kfreed ( if it was kmalloced ) immediately after calling
the cache create; in some other case, the name string needs to be kept
valid until some init calls finished. 

I agree with you that it would make the code slub-dependent, so I'm now
working on the consistency of the other allocators regarding this name
string duplicating thing. 

If you really need to kfree the string, or even if it is easier for you
this way, it can be done. As a matter of fact, this is the case for me.
Just that your patch is not enough. Christoph has a patch that makes
this behavior consistent over all allocators.

This just needs to be pushed again to the tree.

On Thu, 2012-07-05 at 12:23 +0400, Glauber Costa wrote:

On 07/05/2012 05:41 AM, Li Zhong wrote:

quoted

On Wed, 2012-07-04 at 16:40 +0400, Glauber Costa wrote:

quoted

On 07/04/2012 01:00 PM, Li Zhong wrote:

quoted

On Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

quoted

quoted

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

Out of curiosity: Why?
This is not (currently) possible with the other allocators (may change
with christoph's unification patches), so you would be making your code
slub-dependent.

For slub itself, I think it's not good that: in some cases, the name
string could be kfreed ( if it was kmalloced ) immediately after calling
the cache create; in some other case, the name string needs to be kept
valid until some init calls finished. 

I agree with you that it would make the code slub-dependent, so I'm now
working on the consistency of the other allocators regarding this name
string duplicating thing. 

If you really need to kfree the string, or even if it is easier for you
this way, it can be done. As a matter of fact, this is the case for me.
Just that your patch is not enough. Christoph has a patch that makes
this behavior consistent over all allocators.

Sorry, I didn't know that. Seems I don't need to continue the half-done
work in slab. If possible, would you please give me a link of the patch?
Thank you. 

This just needs to be pushed again to the tree.

On 07/05/2012 01:29 PM, Li Zhong wrote:

On Thu, 2012-07-05 at 12:23 +0400, Glauber Costa wrote:

quoted

On 07/05/2012 05:41 AM, Li Zhong wrote:

quoted

On Wed, 2012-07-04 at 16:40 +0400, Glauber Costa wrote:

quoted

On 07/04/2012 01:00 PM, Li Zhong wrote:

quoted

On Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

quoted

quoted

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

Out of curiosity: Why?
This is not (currently) possible with the other allocators (may change
with christoph's unification patches), so you would be making your code
slub-dependent.

For slub itself, I think it's not good that: in some cases, the name
string could be kfreed ( if it was kmalloced ) immediately after calling
the cache create; in some other case, the name string needs to be kept
valid until some init calls finished. 

I agree with you that it would make the code slub-dependent, so I'm now
working on the consistency of the other allocators regarding this name
string duplicating thing. 

If you really need to kfree the string, or even if it is easier for you
this way, it can be done. As a matter of fact, this is the case for me.
Just that your patch is not enough. Christoph has a patch that makes
this behavior consistent over all allocators.

Sorry, I didn't know that. Seems I don't need to continue the half-done
work in slab. If possible, would you please give me a link of the patch?
Thank you. 

Sorry for the delay. In case you haven't found it out yourself yet:

http://www.spinics.net/lists/linux-mm/msg36149.html

Please not this posted patch as is has a bug.

I do believe that your take on the aliasing code adds value to it. But
as I've already said once, might have to dig a bit deeper in that to get
to end of the rabbit hole.

On Fri, 2012-07-06 at 14:13 +0400, Glauber Costa wrote:

On 07/05/2012 01:29 PM, Li Zhong wrote:

quoted

On Thu, 2012-07-05 at 12:23 +0400, Glauber Costa wrote:

quoted

On 07/05/2012 05:41 AM, Li Zhong wrote:

quoted

On Wed, 2012-07-04 at 16:40 +0400, Glauber Costa wrote:

quoted

On 07/04/2012 01:00 PM, Li Zhong wrote:

quoted

On Tue, 2012-07-03 at 15:36 -0500, Christoph Lameter wrote:

quoted

quoted

Looking through the emails it seems that there is an issue with alias
strings. 

To be more precise, there seems no big issue currently. I just wanted to
make following usage of kmem_cache_create (SLUB) possible:

	name = some string kmalloced
	kmem_cache_create(name, ...)
	kfree(name);

Out of curiosity: Why?
This is not (currently) possible with the other allocators (may change
with christoph's unification patches), so you would be making your code
slub-dependent.

For slub itself, I think it's not good that: in some cases, the name
string could be kfreed ( if it was kmalloced ) immediately after calling
the cache create; in some other case, the name string needs to be kept
valid until some init calls finished. 

I agree with you that it would make the code slub-dependent, so I'm now
working on the consistency of the other allocators regarding this name
string duplicating thing. 

If you really need to kfree the string, or even if it is easier for you
this way, it can be done. As a matter of fact, this is the case for me.
Just that your patch is not enough. Christoph has a patch that makes
this behavior consistent over all allocators.

Sorry, I didn't know that. Seems I don't need to continue the half-done
work in slab. If possible, would you please give me a link of the patch?
Thank you. 

Sorry for the delay. In case you haven't found it out yourself yet:

http://www.spinics.net/lists/linux-mm/msg36149.html

Thank you. I think it is better to have these things in the
slab_common.c. 

Please not this posted patch as is has a bug.

I do believe that your take on the aliasing code adds value to it. But
as I've already said once, might have to dig a bit deeper in that to get
to end of the rabbit hole.

With slab_common, I think my slab/slob modifications are not needed any
more. After I understand the common patches, I will check whether the
aliasing problem in slub still exists, and if yes, try to send a patch
based on that.