linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees
@ 2008-05-28 17:40 Christoph Lameter
  2008-05-28 17:51 ` Matt Mackall
  2008-05-28 20:32 ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 6+ messages in thread
From: Christoph Lameter @ 2008-05-28 17:40 UTC (permalink / raw)
  To: Pekka J Enberg, mpm; +Cc: linux-mm, David Miller

kmem_cache_name() is used only by the networking subsystem in order to retrieve
a char * pointer that was passed to kmem_cache_create(). The name of the 
slab was created dynamically by the network subsystem and therefore there 
is a need to free the name when the slab is no longer in use.

This use creates a dependency on the internal workings of the slab 
allocator. It assumes that the slab allocator stores a pointer to the 
string passed in at kmem_cache_create and that the pointer can be 
retrieved later until the slab is destroyed.

SLUB does not follow that expectation for merged slabs. In that case the
slab name passed to kmem_cache_create() may only be used to create a 
symlink in /sys/kernel/slab. The "name" of the slab that will be returned 
on kmem_cache_name() is the name of the first kmem_cache_create() that 
caused a slab of a certain size to be created.

This can lead to double frees or the freeing of a string constant when
a slab is destroyed by the network subsystem by the following action in 
ccid_kmem_cache_destroy() (DCCP protocol) and in proto_unregister().

1. Retrieving the slab name via kmem_cache_name()
2. Destroying the slab cache by calling kmem_cache_destroy().
3. Freeing the slab name via kfree().

It seems that it is rare to trigger invalid kfrees because the slabs 
with the dynamic names are rarely created (at least on my systems) and 
then destroyed. In many cases it seems that the first name is the actual 
name of slab because of the uniqueness of the slab characteristics. I only 
found these while testing with cpu_alloc patches that influenced the 
sizes of these structures. But I am sure this can also be triggered under 
other conditions.

Fix:

Create special fields in the networking structs to store a pointer to
names of slab generated. The pointer is then used to free the name of
the slab after the slab was destroyed.

Drop the support for kmem_cache_name from all slab allocators.

Signed-off-by: Christoph Lameter <clameter@sgi.com>


---
 include/linux/slab.h        |    1 -
 include/net/request_sock.h  |    1 +
 include/net/timewait_sock.h |    1 +
 mm/slab.c                   |    6 ------
 mm/slob.c                   |    6 ------
 mm/slub.c                   |    6 ------
 net/core/sock.c             |   37 ++++++++++++++++++-------------------
 net/dccp/ccid.c             |   39 +++++++++++++++++++++++++--------------
 net/dccp/ccid.h             |    2 ++
 9 files changed, 47 insertions(+), 52 deletions(-)

Index: linux-2.6/include/linux/slab.h
===================================================================
--- linux-2.6.orig/include/linux/slab.h	2008-05-27 22:55:22.878988551 -0700
+++ linux-2.6/include/linux/slab.h	2008-05-27 22:55:35.167739564 -0700
@@ -63,7 +63,6 @@ void kmem_cache_destroy(struct kmem_cach
 int kmem_cache_shrink(struct kmem_cache *);
 void kmem_cache_free(struct kmem_cache *, void *);
 unsigned int kmem_cache_size(struct kmem_cache *);
-const char *kmem_cache_name(struct kmem_cache *);
 int kmem_ptr_validate(struct kmem_cache *cachep, const void *ptr);
 
 /*
Index: linux-2.6/mm/slab.c
===================================================================
--- linux-2.6.orig/mm/slab.c	2008-05-27 22:55:22.878988551 -0700
+++ linux-2.6/mm/slab.c	2008-05-27 22:55:35.167739564 -0700
@@ -3802,12 +3802,6 @@ unsigned int kmem_cache_size(struct kmem
 }
 EXPORT_SYMBOL(kmem_cache_size);
 
-const char *kmem_cache_name(struct kmem_cache *cachep)
-{
-	return cachep->name;
-}
-EXPORT_SYMBOL_GPL(kmem_cache_name);
-
 /*
  * This initializes kmem_list3 or resizes various caches for all nodes.
  */
Index: linux-2.6/mm/slob.c
===================================================================
--- linux-2.6.orig/mm/slob.c	2008-05-27 22:55:22.898987823 -0700
+++ linux-2.6/mm/slob.c	2008-05-27 22:55:35.187738215 -0700
@@ -617,12 +617,6 @@ unsigned int kmem_cache_size(struct kmem
 }
 EXPORT_SYMBOL(kmem_cache_size);
 
-const char *kmem_cache_name(struct kmem_cache *c)
-{
-	return c->name;
-}
-EXPORT_SYMBOL(kmem_cache_name);
-
 int kmem_cache_shrink(struct kmem_cache *d)
 {
 	return 0;
Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2008-05-27 22:55:22.888987704 -0700
+++ linux-2.6/mm/slub.c	2008-05-27 22:55:35.197736279 -0700
@@ -2390,12 +2390,6 @@ unsigned int kmem_cache_size(struct kmem
 }
 EXPORT_SYMBOL(kmem_cache_size);
 
-const char *kmem_cache_name(struct kmem_cache *s)
-{
-	return s->name;
-}
-EXPORT_SYMBOL(kmem_cache_name);
-
 static void list_slab_objects(struct kmem_cache *s, struct page *page,
 							const char *text)
 {
Index: linux-2.6/net/core/sock.c
===================================================================
--- linux-2.6.orig/net/core/sock.c	2008-05-27 22:55:22.928988094 -0700
+++ linux-2.6/net/core/sock.c	2008-05-27 23:25:14.124850559 -0700
@@ -2036,9 +2036,6 @@ static inline void release_proto_idx(str
 
 int proto_register(struct proto *prot, int alloc_slab)
 {
-	char *request_sock_slab_name = NULL;
-	char *timewait_sock_slab_name;
-
 	if (alloc_slab) {
 		prot->slab = kmem_cache_create(prot->name, prot->obj_size, 0,
 					       SLAB_HWCACHE_ALIGN, NULL);
@@ -2052,12 +2049,13 @@ int proto_register(struct proto *prot, i
 		if (prot->rsk_prot != NULL) {
 			static const char mask[] = "request_sock_%s";
 
-			request_sock_slab_name = kmalloc(strlen(prot->name) + sizeof(mask) - 1, GFP_KERNEL);
-			if (request_sock_slab_name == NULL)
+			prot->rsk_prot->name =
+				kmalloc(strlen(prot->name) + sizeof(mask) - 1, GFP_KERNEL);
+			if (prot->rsk_prot->name == NULL)
 				goto out_free_sock_slab;
 
-			sprintf(request_sock_slab_name, mask, prot->name);
-			prot->rsk_prot->slab = kmem_cache_create(request_sock_slab_name,
+			sprintf(prot->rsk_prot->name, mask, prot->name);
+			prot->rsk_prot->slab = kmem_cache_create(prot->rsk_prot->name,
 								 prot->rsk_prot->obj_size, 0,
 								 SLAB_HWCACHE_ALIGN, NULL);
 
@@ -2071,14 +2069,15 @@ int proto_register(struct proto *prot, i
 		if (prot->twsk_prot != NULL) {
 			static const char mask[] = "tw_sock_%s";
 
-			timewait_sock_slab_name = kmalloc(strlen(prot->name) + sizeof(mask) - 1, GFP_KERNEL);
+			prot->twsk_prot->twsk_name =
+				kmalloc(strlen(prot->name) + sizeof(mask) - 1, GFP_KERNEL);
 
-			if (timewait_sock_slab_name == NULL)
+			if (prot->twsk_prot->twsk_name == NULL)
 				goto out_free_request_sock_slab;
 
-			sprintf(timewait_sock_slab_name, mask, prot->name);
+			sprintf(prot->twsk_prot->twsk_name, mask, prot->name);
 			prot->twsk_prot->twsk_slab =
-				kmem_cache_create(timewait_sock_slab_name,
+				kmem_cache_create(prot->twsk_prot->twsk_name,
 						  prot->twsk_prot->twsk_obj_size,
 						  0, SLAB_HWCACHE_ALIGN,
 						  NULL);
@@ -2094,14 +2093,16 @@ int proto_register(struct proto *prot, i
 	return 0;
 
 out_free_timewait_sock_slab_name:
-	kfree(timewait_sock_slab_name);
+	kfree(prot->twsk_prot->twsk_name);
+	prot->twsk_prot->twsk_name = NULL;
 out_free_request_sock_slab:
 	if (prot->rsk_prot && prot->rsk_prot->slab) {
 		kmem_cache_destroy(prot->rsk_prot->slab);
 		prot->rsk_prot->slab = NULL;
 	}
 out_free_request_sock_slab_name:
-	kfree(request_sock_slab_name);
+	kfree(prot->rsk_prot->name);
+	prot->rsk_prot->name = NULL;
 out_free_sock_slab:
 	kmem_cache_destroy(prot->slab);
 	prot->slab = NULL;
@@ -2124,19 +2125,17 @@ void proto_unregister(struct proto *prot
 	}
 
 	if (prot->rsk_prot != NULL && prot->rsk_prot->slab != NULL) {
-		const char *name = kmem_cache_name(prot->rsk_prot->slab);
-
 		kmem_cache_destroy(prot->rsk_prot->slab);
-		kfree(name);
 		prot->rsk_prot->slab = NULL;
+		kfree(prot->rsk_prot->name);
+		prot->rsk_prot->name = NULL;
 	}
 
 	if (prot->twsk_prot != NULL && prot->twsk_prot->twsk_slab != NULL) {
-		const char *name = kmem_cache_name(prot->twsk_prot->twsk_slab);
-
 		kmem_cache_destroy(prot->twsk_prot->twsk_slab);
-		kfree(name);
 		prot->twsk_prot->twsk_slab = NULL;
+		kfree(prot->twsk_prot->twsk_name);
+		prot->twsk_prot->twsk_name = NULL;
 	}
 }
 
Index: linux-2.6/include/net/request_sock.h
===================================================================
--- linux-2.6.orig/include/net/request_sock.h	2008-05-27 22:55:33.516487474 -0700
+++ linux-2.6/include/net/request_sock.h	2008-05-27 22:55:36.738987441 -0700
@@ -30,6 +30,7 @@ struct request_sock_ops {
 	int		family;
 	int		obj_size;
 	struct kmem_cache	*slab;
+	char		*name;
 	int		(*rtx_syn_ack)(struct sock *sk,
 				       struct request_sock *req);
 	void		(*send_ack)(struct sk_buff *skb,
Index: linux-2.6/include/net/timewait_sock.h
===================================================================
--- linux-2.6.orig/include/net/timewait_sock.h	2008-05-27 22:55:33.516487474 -0700
+++ linux-2.6/include/net/timewait_sock.h	2008-05-27 22:55:36.738987441 -0700
@@ -17,6 +17,7 @@
 struct timewait_sock_ops {
 	struct kmem_cache	*twsk_slab;
 	unsigned int	twsk_obj_size;
+	char		*twsk_name;
 	int		(*twsk_unique)(struct sock *sk,
 				       struct sock *sktw, void *twp);
 	void		(*twsk_destructor)(struct sock *sk);
Index: linux-2.6/net/dccp/ccid.c
===================================================================
--- linux-2.6.orig/net/dccp/ccid.c	2008-05-27 23:00:39.466488223 -0700
+++ linux-2.6/net/dccp/ccid.c	2008-05-27 23:19:38.559419815 -0700
@@ -56,31 +56,32 @@ static inline void ccids_read_unlock(voi
 #define ccids_read_unlock() do { } while(0)
 #endif
 
-static struct kmem_cache *ccid_kmem_cache_create(int obj_size, const char *fmt,...)
+static struct kmem_cache *ccid_kmem_cache_create(int obj_size, char **name,
+							const char *fmt,...)
 {
 	struct kmem_cache *slab;
-	char slab_name_fmt[32], *slab_name;
+	char slab_name_fmt[32];
 	va_list args;
 
 	va_start(args, fmt);
 	vsnprintf(slab_name_fmt, sizeof(slab_name_fmt), fmt, args);
 	va_end(args);
 
-	slab_name = kstrdup(slab_name_fmt, GFP_KERNEL);
-	if (slab_name == NULL)
+	*name = kstrdup(slab_name_fmt, GFP_KERNEL);
+	if (*name == NULL)
 		return NULL;
-	slab = kmem_cache_create(slab_name, sizeof(struct ccid) + obj_size, 0,
+	slab = kmem_cache_create(*name, sizeof(struct ccid) + obj_size, 0,
 				 SLAB_HWCACHE_ALIGN, NULL);
-	if (slab == NULL)
-		kfree(slab_name);
+	if (slab == NULL) {
+		kfree(*name);
+		*name = NULL;
+	}
 	return slab;
 }
 
-static void ccid_kmem_cache_destroy(struct kmem_cache *slab)
+static void ccid_kmem_cache_destroy(struct kmem_cache *slab, char *name)
 {
 	if (slab != NULL) {
-		const char *name = kmem_cache_name(slab);
-
 		kmem_cache_destroy(slab);
 		kfree(name);
 	}
@@ -92,6 +93,7 @@ int ccid_register(struct ccid_operations
 
 	ccid_ops->ccid_hc_rx_slab =
 			ccid_kmem_cache_create(ccid_ops->ccid_hc_rx_obj_size,
+						&ccid_ops->ccid_hc_rx_name,
 					       "ccid%u_hc_rx_sock",
 					       ccid_ops->ccid_id);
 	if (ccid_ops->ccid_hc_rx_slab == NULL)
@@ -99,6 +101,7 @@ int ccid_register(struct ccid_operations
 
 	ccid_ops->ccid_hc_tx_slab =
 			ccid_kmem_cache_create(ccid_ops->ccid_hc_tx_obj_size,
+						&ccid_ops->ccid_hc_tx_name,
 					       "ccid%u_hc_tx_sock",
 					       ccid_ops->ccid_id);
 	if (ccid_ops->ccid_hc_tx_slab == NULL)
@@ -119,12 +122,16 @@ int ccid_register(struct ccid_operations
 out:
 	return err;
 out_free_tx_slab:
-	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab);
+	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab,
+					ccid_ops->ccid_hc_tx_name);
 	ccid_ops->ccid_hc_tx_slab = NULL;
+	ccid_ops->ccid_hc_tx_name = NULL;
 	goto out;
 out_free_rx_slab:
-	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab);
+	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab,
+					ccid_ops->ccid_hc_rx_name);
 	ccid_ops->ccid_hc_rx_slab = NULL;
+	ccid_ops->ccid_hc_rx_name = NULL;
 	goto out;
 }
 
@@ -136,10 +143,14 @@ int ccid_unregister(struct ccid_operatio
 	ccids[ccid_ops->ccid_id] = NULL;
 	ccids_write_unlock();
 
-	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab);
+	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab,
+						ccid_ops->ccid_hc_tx_name);
 	ccid_ops->ccid_hc_tx_slab = NULL;
-	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab);
+	ccid_ops->ccid_hc_tx_name = NULL;
+	ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab,
+						ccid_ops->ccid_hc_rx_name);
 	ccid_ops->ccid_hc_rx_slab = NULL;
+	ccid_ops->ccid_hc_rx_name = NULL;
 
 	pr_info("CCID: Unregistered CCID %d (%s)\n",
 		ccid_ops->ccid_id, ccid_ops->ccid_name);
Index: linux-2.6/net/dccp/ccid.h
===================================================================
--- linux-2.6.orig/net/dccp/ccid.h	2008-05-27 23:05:18.836487436 -0700
+++ linux-2.6/net/dccp/ccid.h	2008-05-27 23:11:02.787525807 -0700
@@ -51,6 +51,8 @@ struct ccid_operations {
 	struct module		*ccid_owner;
 	struct kmem_cache	*ccid_hc_rx_slab,
 				*ccid_hc_tx_slab;
+	char			*ccid_hc_rx_name,
+				*ccid_hc_tx_name;
 	__u32			ccid_hc_rx_obj_size,
 				ccid_hc_tx_obj_size;
 	/* Interface Routines */

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees
  2008-05-28 17:40 Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees Christoph Lameter
@ 2008-05-28 17:51 ` Matt Mackall
  2008-05-28 17:58   ` Christoph Lameter
  2008-05-28 18:23   ` Christoph Lameter
  2008-05-28 20:32 ` Arnaldo Carvalho de Melo
  1 sibling, 2 replies; 6+ messages in thread
From: Matt Mackall @ 2008-05-28 17:51 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka J Enberg, linux-mm, David Miller, acme

[Adding Arnaldo to cc:]

On Wed, 2008-05-28 at 10:40 -0700, Christoph Lameter wrote:
> Fix:
> 
> Create special fields in the networking structs to store a pointer to
> names of slab generated. The pointer is then used to free the name of
> the slab after the slab was destroyed.
> 
> Drop the support for kmem_cache_name from all slab allocators.

Seems it would be better to simply allow either unnamed caches or
duplicate cache names.

-- 
Mathematics is the supreme nostalgia of our time.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees
  2008-05-28 17:51 ` Matt Mackall
@ 2008-05-28 17:58   ` Christoph Lameter
  2008-05-28 18:23   ` Christoph Lameter
  1 sibling, 0 replies; 6+ messages in thread
From: Christoph Lameter @ 2008-05-28 17:58 UTC (permalink / raw)
  To: Matt Mackall; +Cc: Pekka J Enberg, linux-mm, David Miller, acme

On Wed, 28 May 2008, Matt Mackall wrote:

> Seems it would be better to simply allow either unnamed caches or
> duplicate cache names.

I am fine if we would allow NULL to be passed as a name. But the name is 
useful for debugging purposes.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees
  2008-05-28 17:51 ` Matt Mackall
  2008-05-28 17:58   ` Christoph Lameter
@ 2008-05-28 18:23   ` Christoph Lameter
  2008-05-28 19:48     ` Pekka Enberg
  1 sibling, 1 reply; 6+ messages in thread
From: Christoph Lameter @ 2008-05-28 18:23 UTC (permalink / raw)
  To: Matt Mackall; +Cc: Pekka J Enberg, linux-mm, David Miller, acme

A draft patch to allow anonymous caches follows. Allowing duplicate names
would also be possible. Just need to deal with sysfs. Maybe generate a
_x at the end?

Subject: slub: Support anonymous slabs

Slabs really do not need to have a name so one could pass NULL as a name. 
The name is only relevant for sysfs support. There we need a unique name.
Use the address of the kmem_cache structure as the name.

[Would output "" if debugging is one]


Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/slub.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2008-05-28 11:16:24.000000000 -0700
+++ linux-2.6/mm/slub.c	2008-05-28 11:17:33.000000000 -0700
@@ -4310,6 +4310,7 @@ static int sysfs_slab_add(struct kmem_ca
 	int err;
 	const char *name;
 	int unmergeable;
+	char buf[20];
 
 	if (slab_state < SYSFS)
 		/* Defer until later */
@@ -4322,8 +4323,13 @@ static int sysfs_slab_add(struct kmem_ca
 		 * This is typically the case for debug situations. In that
 		 * case we can catch duplicate names easily.
 		 */
-		sysfs_remove_link(&slab_kset->kobj, s->name);
+		if (s->name)
+			sysfs_remove_link(&slab_kset->kobj, s->name);
 		name = s->name;
+		if (!name) {
+			sprintf(buf, "%p", s);
+			name = buf;
+		}
 	} else {
 		/*
 		 * Create a unique name for the slab as a target
@@ -4343,7 +4349,7 @@ static int sysfs_slab_add(struct kmem_ca
 	if (err)
 		return err;
 	kobject_uevent(&s->kobj, KOBJ_ADD);
-	if (!unmergeable) {
+	if (!unmergeable && s->name) {
 		/* Setup first alias */
 		sysfs_slab_alias(s, s->name);
 		kfree(name);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees
  2008-05-28 18:23   ` Christoph Lameter
@ 2008-05-28 19:48     ` Pekka Enberg
  0 siblings, 0 replies; 6+ messages in thread
From: Pekka Enberg @ 2008-05-28 19:48 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Matt Mackall, linux-mm, David Miller, acme

Christoph Lameter wrote:
> A draft patch to allow anonymous caches follows. Allowing duplicate names
> would also be possible. Just need to deal with sysfs. Maybe generate a
> _x at the end?

Or a "cache-" prefix. I don't think we ought to allow duplicate names, 
though.

> Subject: slub: Support anonymous slabs
> 
> Slabs really do not need to have a name so one could pass NULL as a name. 
> The name is only relevant for sysfs support. There we need a unique name.
> Use the address of the kmem_cache structure as the name.
> 
> [Would output "" if debugging is one]
> 
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> ---
>  mm/slub.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2008-05-28 11:16:24.000000000 -0700
> +++ linux-2.6/mm/slub.c	2008-05-28 11:17:33.000000000 -0700
> @@ -4310,6 +4310,7 @@ static int sysfs_slab_add(struct kmem_ca
>  	int err;
>  	const char *name;
>  	int unmergeable;
> +	char buf[20];
>  
>  	if (slab_state < SYSFS)
>  		/* Defer until later */
> @@ -4322,8 +4323,13 @@ static int sysfs_slab_add(struct kmem_ca
>  		 * This is typically the case for debug situations. In that
>  		 * case we can catch duplicate names easily.
>  		 */
> -		sysfs_remove_link(&slab_kset->kobj, s->name);
> +		if (s->name)
> +			sysfs_remove_link(&slab_kset->kobj, s->name);
>  		name = s->name;
> +		if (!name) {
> +			sprintf(buf, "%p", s);
> +			name = buf;
> +		}

Perhaps add a comment here explaining why we're doing this?

Other than that, looks good to me. Please re-send with proper changelog 
from the original patch.

		Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees
  2008-05-28 17:40 Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees Christoph Lameter
  2008-05-28 17:51 ` Matt Mackall
@ 2008-05-28 20:32 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2008-05-28 20:32 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka J Enberg, mpm, linux-mm, David Miller

Em Wed, May 28, 2008 at 10:40:39AM -0700, Christoph Lameter escreveu:
> kmem_cache_name() is used only by the networking subsystem in order to retrieve
> a char * pointer that was passed to kmem_cache_create(). The name of the 
> slab was created dynamically by the network subsystem and therefore there 
> is a need to free the name when the slab is no longer in use.
> 
> This use creates a dependency on the internal workings of the slab 
> allocator. It assumes that the slab allocator stores a pointer to the 
> string passed in at kmem_cache_create and that the pointer can be 
> retrieved later until the slab is destroyed.
> 
> SLUB does not follow that expectation for merged slabs. In that case the
> slab name passed to kmem_cache_create() may only be used to create a 
> symlink in /sys/kernel/slab. The "name" of the slab that will be returned 
> on kmem_cache_name() is the name of the first kmem_cache_create() that 
> caused a slab of a certain size to be created.
> 
> This can lead to double frees or the freeing of a string constant when
> a slab is destroyed by the network subsystem by the following action in 
> ccid_kmem_cache_destroy() (DCCP protocol) and in proto_unregister().
> 
> 1. Retrieving the slab name via kmem_cache_name()
> 2. Destroying the slab cache by calling kmem_cache_destroy().
> 3. Freeing the slab name via kfree().
> 
> It seems that it is rare to trigger invalid kfrees because the slabs 
> with the dynamic names are rarely created (at least on my systems) and 
> then destroyed. In many cases it seems that the first name is the actual 
> name of slab because of the uniqueness of the slab characteristics. I only 
> found these while testing with cpu_alloc patches that influenced the 
> sizes of these structures. But I am sure this can also be triggered under 
> other conditions.
> 
> Fix:
> 
> Create special fields in the networking structs to store a pointer to
> names of slab generated. The pointer is then used to free the name of
> the slab after the slab was destroyed.
> 
> Drop the support for kmem_cache_name from all slab allocators.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>

I'm ok with this, thanks,

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-05-28 20:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-28 17:40 Subject: Slab allocators: Remove kmem_cache_name() to fix invalid frees Christoph Lameter
2008-05-28 17:51 ` Matt Mackall
2008-05-28 17:58   ` Christoph Lameter
2008-05-28 18:23   ` Christoph Lameter
2008-05-28 19:48     ` Pekka Enberg
2008-05-28 20:32 ` Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).