slab.c use of __get_user and sparse

slab.c use of __get_user and sparse

[Sam Ravnborg]

Hi Andi, lkml.

In slab.c around line 1450 the following code is present:

list_for_each(p, &cache_chain) {
	kmem_cache_t *pc = list_entry(p, kmem_cache_t, next);
	char tmp;
	/* This happens when the module gets unloaded and doesn't
	   destroy its slab cache and noone else reuses the vmalloc
	   area of the module. Print a warning. */
	if (__get_user(tmp,(char __user *) pc->name)) { 
		printk("SLAB: cache with size %d has lost its name\n", 
			pc->objsize); 
		continue; 

sparse emit a warning for the line with __get_user because the pointer
is not marker __user. So the above cast inserted by me made sparse shut up.

Based on the comment it is understood that suddenly this pointer points
to userspace, because the module got unloaded.
I wonder why we can rely on the same address now the module got unloaded -
we may risk this virtual address is taken over by someone else?

Andi - sent to you since you made this change loong time ago.

[mm/ is sparse clean with defconfig when this is fixed].

	Sam

Re: slab.c use of __get_user and sparse

[Andi Kleen]

> Based on the comment it is understood that suddenly this pointer points
> to userspace, because the module got unloaded.
> I wonder why we can rely on the same address now the module got unloaded -
> we may risk this virtual address is taken over by someone else?

The address is not user space; you would be lying.

Perhaps it's best to get rid of the hack completely. Turn kmem_cache_t->name
into an array and copy the name instead of storing the pointer, then
it wouldn't be needed at all.

-Andi

Re: slab.c use of __get_user and sparse

[Andreas Gruenbacher]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Saturday 15 January 2005 23:01, Andi Kleen wrote:
> > Based on the comment it is understood that suddenly this pointer points
> > to userspace, because the module got unloaded.
> > I wonder why we can rely on the same address now the module got unloaded
> > - we may risk this virtual address is taken over by someone else?
>
> The address is not user space; you would be lying.
>
> Perhaps it's best to get rid of the hack completely. Turn
> kmem_cache_t->name into an array and copy the name instead of storing the
> pointer, then it wouldn't be needed at all.

Those are just bugs from the time before there was kmem_cache_destroy. I 
checked the 2.6.11-rc1-mm1 tree: every kmem_cache_create in modules seems to 
destroyed properly except in decnet, and decnet module unloading currently is 
disabled. The attached patch fixes the decnet case, puts the slab name in a 
static array, and removes the name accessibilty check.

Regards,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

[-- Attachment #2: slab-destroy.diff --]
[-- Type: text/plain, Size: 2396 bytes --]

From: Andreas Gruenbacher <agruen@suse.de>
Subject: Missing kmem_cache_destroy()s in decnet; remove dead-slab check

Decnet is not destroying two of the slabs it creates. Put slab names into
struct kmem_cache_s, and remove the name accessibility hack.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-rc1-mm1/net/decnet/dn_table.c
===================================================================
--- linux-2.6.11-rc1-mm1.orig/net/decnet/dn_table.c
+++ linux-2.6.11-rc1-mm1/net/decnet/dn_table.c
@@ -820,6 +820,7 @@ void __exit dn_fib_table_cleanup(void)
 
 	for (i = RT_TABLE_MIN; i <= RT_TABLE_MAX; ++i)
 		dn_fib_del_tree(i);
+	kmem_cache_destroy(dn_hash_kmem);
 
 	return;
 }
Index: linux-2.6.11-rc1-mm1/net/decnet/dn_route.c
===================================================================
--- linux-2.6.11-rc1-mm1.orig/net/decnet/dn_route.c
+++ linux-2.6.11-rc1-mm1/net/decnet/dn_route.c
@@ -1835,6 +1835,7 @@ void __exit dn_route_cleanup(void)
 {
 	del_timer(&dn_route_timer);
 	dn_run_flush(0);
+	kmem_cache_destroy(dn_dst_ops.kmem_cachep);
 
 	proc_net_remove("decnet_cache");
 }
Index: linux-2.6.11-rc1-mm1/mm/slab.c
===================================================================
--- linux-2.6.11-rc1-mm1.orig/mm/slab.c
+++ linux-2.6.11-rc1-mm1/mm/slab.c
@@ -334,7 +334,7 @@ struct kmem_cache_s {
 	void (*dtor)(void *, kmem_cache_t *, unsigned long);
 
 /* 4) cache creation/removal */
-	const char		*name;
+	char			name[32];
 	struct list_head	next;
 
 /* 5) statistics */
@@ -1419,7 +1419,7 @@ next:
 		cachep->slabp_cache = kmem_find_general_cachep(slab_size,0);
 	cachep->ctor = ctor;
 	cachep->dtor = dtor;
-	cachep->name = name;
+	strlcpy(cachep->name, name, sizeof(cachep->name));
 
 	/* Don't let CPUs to come and go */
 	lock_cpu_hotplug();
@@ -1465,15 +1465,6 @@ next:
 		set_fs(KERNEL_DS);
 		list_for_each(p, &cache_chain) {
 			kmem_cache_t *pc = list_entry(p, kmem_cache_t, next);
-			char tmp;
-			/* This happens when the module gets unloaded and doesn't
-			   destroy its slab cache and noone else reuses the vmalloc
-			   area of the module. Print a warning. */
-			if (__get_user(tmp,pc->name)) { 
-				printk("SLAB: cache with size %d has lost its name\n", 
-					pc->objsize); 
-				continue; 
-			} 	
 			if (!strcmp(pc->name,name)) { 
 				printk("kmem_cache_create: duplicate cache %s\n",name); 
 				up(&cache_chain_sem); 

Re: slab.c use of __get_user and sparse

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5966 bytes --]

On Jan 15, 2005  10:22 +0100, Andreas Gruenbacher wrote:
> On Saturday 15 January 2005 23:01, Andi Kleen wrote:
> Those are just bugs from the time before there was kmem_cache_destroy. I 
> checked the 2.6.11-rc1-mm1 tree: every kmem_cache_create in modules seems to 
> destroyed properly except in decnet, and decnet module unloading currently is 
> disabled. The attached patch fixes the decnet case, puts the slab name in a 
> static array, and removes the name accessibilty check.

Actually, it appears that a fix I made for 2.4 never made it into 2.5/2.6.
In 2.4 we define the maximum length and check for this in kmem_cache_create().
The decnet slab cleanup fix is of course valid, but we may as well make the
2.6 code match the fix in 2.4.

I've shortened the cache names in ntfs to be less than the 20-character
limit present in 2.4, there are no others that are that long.


===== mm/slab.c 1.153 vs edited =====
--- 1.153/mm/slab.c	2005-01-07 22:44:01 -07:00
+++ edited/mm/slab.c	2005-01-16 13:42:51 -07:00
@@ -298,7 +298,9 @@
  *
  * manages a cache.
  */
-	
+
+#define CACHE_NAMELEN	20	/* max name length for a slab cache */
+
 struct kmem_cache_s {
 /* 1) per-cpu data, touched during every alloc/free */
 	struct array_cache	*array[NR_CPUS];
@@ -334,7 +336,7 @@ struct kmem_cache_s {
 	void (*dtor)(void *, kmem_cache_t *, unsigned long);
 
 /* 4) cache creation/removal */
-	const char		*name;
+	char			name[CACHE_NAMELEN];
 	struct list_head	next;
 
 /* 5) statistics */
@@ -1198,6 +1200,7 @@ kmem_cache_create (const char *name,
 	 * Sanity checks... these are all serious usage bugs.
 	 */
 	if ((!name) ||
+		(strlen(name) >= CACHE_NAMELEN - 1) ||
 		in_interrupt() ||
 		(size < BYTES_PER_WORD) ||
 		(size > (1<<MAX_OBJ_ORDER)*PAGE_SIZE) ||
@@ -1417,7 +1420,8 @@ next:
 		cachep->slabp_cache = kmem_find_general_cachep(slab_size,0);
 	cachep->ctor = ctor;
 	cachep->dtor = dtor;
-	cachep->name = name;
+	/* Copy name over so we don't have problems with unloaded modules */
+	strcpy(cachep->name, name);
 
 	/* Don't let CPUs to come and go */
 	lock_cpu_hotplug();
@@ -1459,21 +1463,12 @@ next:
 		set_fs(KERNEL_DS);
 		list_for_each(p, &cache_chain) {
 			kmem_cache_t *pc = list_entry(p, kmem_cache_t, next);
-			char tmp;
-			/* This happens when the module gets unloaded and doesn't
-			   destroy its slab cache and noone else reuses the vmalloc
-			   area of the module. Print a warning. */
-			if (__get_user(tmp,pc->name)) { 
-				printk("SLAB: cache with size %d has lost its name\n", 
-					pc->objsize); 
-				continue; 
-			} 	
-			if (!strcmp(pc->name,name)) { 
-				printk("kmem_cache_create: duplicate cache %s\n",name); 
-				up(&cache_chain_sem); 
+			if (!strcmp(pc->name,name)) {
+				printk("kmem_cache_create: duplicate cache %s\n",name);
+				up(&cache_chain_sem)
 				unlock_cpu_hotplug();
-				BUG(); 
-			}	
+				BUG();
+			}
 		}
 		set_fs(old_fs);
 	}
===== fs/ntfs/super.c 1.184 vs edited =====
--- 1.184/fs/ntfs/super.c	2005-01-04 19:48:14 -07:00
+++ edited/fs/ntfs/super.c	2005-01-16 14:21:03 -07:00
@@ -2621,11 +2621,11 @@
 };
 
 /* Stable names for the slab caches. */
-static const char ntfs_index_ctx_cache_name[] = "ntfs_index_ctx_cache";
-static const char ntfs_attr_ctx_cache_name[] = "ntfs_attr_ctx_cache";
+static const char ntfs_index_ctx_cache_name[] = "ntfs_index_ctx";
+static const char ntfs_attr_ctx_cache_name[] = "ntfs_attr_ctx";
 static const char ntfs_name_cache_name[] = "ntfs_name_cache";
 static const char ntfs_inode_cache_name[] = "ntfs_inode_cache";
-static const char ntfs_big_inode_cache_name[] = "ntfs_big_inode_cache";
+static const char ntfs_big_inode_cache_name[] = "ntfs_big_inode";
 
 static int __init init_ntfs_fs(void)
 {
@@ -2652,7 +2652,7 @@
 			sizeof(ntfs_index_context), 0 /* offset */,
 			SLAB_HWCACHE_ALIGN, NULL /* ctor */, NULL /* dtor */);
 	if (!ntfs_index_ctx_cache) {
-		printk(KERN_CRIT "NTFS: Failed to create %s!\n",
+		printk(KERN_CRIT "NTFS: Failed to create %s cache!\n",
 				ntfs_index_ctx_cache_name);
 		goto ictx_err_out;
 	}
@@ -2660,7 +2660,7 @@
 			sizeof(ntfs_attr_search_ctx), 0 /* offset */,
 			SLAB_HWCACHE_ALIGN, NULL /* ctor */, NULL /* dtor */);
 	if (!ntfs_attr_ctx_cache) {
-		printk(KERN_CRIT "NTFS: Failed to create %s!\n",
+		printk(KERN_CRIT "NTFS: Failed to create %s cache!\n",
 				ntfs_attr_ctx_cache_name);
 		goto actx_err_out;
 	}
@@ -2688,7 +2688,7 @@
 			SLAB_HWCACHE_ALIGN|SLAB_RECLAIM_ACCOUNT,
 			ntfs_big_inode_init_once, NULL);
 	if (!ntfs_big_inode_cache) {
-		printk(KERN_CRIT "NTFS: Failed to create %s!\n",
+		printk(KERN_CRIT "NTFS: Failed to create %s cache!\n",
 				ntfs_big_inode_cache_name);
 		goto big_inode_err_out;
 	}
@@ -2735,7 +2735,7 @@
 	unregister_filesystem(&ntfs_fs_type);
 
 	if (kmem_cache_destroy(ntfs_big_inode_cache) && (err = 1))
-		printk(KERN_CRIT "NTFS: Failed to destory %s.\n",
+		printk(KERN_CRIT "NTFS: Failed to destory %s cache.\n",
 				ntfs_big_inode_cache_name);
 	if (kmem_cache_destroy(ntfs_inode_cache) && (err = 1))
 		printk(KERN_CRIT "NTFS: Failed to destory %s.\n",
@@ -2744,10 +2744,10 @@
 		printk(KERN_CRIT "NTFS: Failed to destory %s.\n",
 				ntfs_name_cache_name);
 	if (kmem_cache_destroy(ntfs_attr_ctx_cache) && (err = 1))
-		printk(KERN_CRIT "NTFS: Failed to destory %s.\n",
+		printk(KERN_CRIT "NTFS: Failed to destory %s cache.\n",
 				ntfs_attr_ctx_cache_name);
 	if (kmem_cache_destroy(ntfs_index_ctx_cache) && (err = 1))
-		printk(KERN_CRIT "NTFS: Failed to destory %s.\n",
+		printk(KERN_CRIT "NTFS: Failed to destory %s cache.\n",
 				ntfs_index_ctx_cache_name);
 	if (err)
 		printk(KERN_CRIT "NTFS: This causes memory to leak! There is "


Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: slab.c use of __get_user and sparse

[Al Viro]

On Sat, Jan 15, 2005 at 11:01:51PM +0100, Andi Kleen wrote:
> > Based on the comment it is understood that suddenly this pointer points
> > to userspace, because the module got unloaded.
> > I wonder why we can rely on the same address now the module got unloaded -
> > we may risk this virtual address is taken over by someone else?
> 
> The address is not user space; you would be lying.
> 
> Perhaps it's best to get rid of the hack completely. Turn kmem_cache_t->name
> into an array and copy the name instead of storing the pointer, then
> it wouldn't be needed at all.

Alternatively, we could provide a new primitive -

size_t safe_memcpy(void *to, void *from, size_t size);

Semantics: copy byte-by-byte until we either get 'size' bytes or trigger an
exception.  Return the number of bytes copied.

Obvious implementation via __get_user() would be default, overridable by
architecture...

Re: slab.c use of __get_user and sparse

[Sam Ravnborg]

On Sat, Jan 15, 2005 at 11:01:51PM +0100, Andi Kleen wrote:
> > Based on the comment it is understood that suddenly this pointer points
> > to userspace, because the module got unloaded.
> > I wonder why we can rely on the same address now the module got unloaded -
> > we may risk this virtual address is taken over by someone else?
> 
> The address is not user space; you would be lying.
Which is very bad - dropped slab.c for now.

	Sam