* 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).