make kobject dynamic allocation check use kallsyms_lookup()

PATCH In review Dave Hansen 4 participants 4 messages Aug 22, 2007 – Aug 23, 2007

Subsystems (2)

Library code Driver core

lore

Share & tools

lore

Dave Hansen

author

2007-08-22 22:04 UTC #0 lore raw

PATCH 1/1 make kobject dynamic allocation check use kallsyms_lookup()

One of the top ten sysfs problems is that users use statically allocated kobjects. This patch reminds them that this is a naughty thing.

One _really_ nice thing this patch does, is us the kallsyms mechanism to print out exactly which symbol is being complained about:

	The kobject at, or inside 'statickobj.2'@(0xc040d020) is not dynamically allocated.

This patch replaces the previous implementation's use of a _sdata symbol in favor of using kallsyms_lookup(). If a kobject's address is a resolvable symbol, then it isn't dynamically allocated.

The one exception to this is init symbols. The patch also checks to see whether __init memory has been freed and if it has will allow kobjects in those sections.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/arch/i386/kernel/vmlinux.lds.S |    2 --
 lxc-dave/include/linux/init.h           |    1 +
 lxc-dave/init/main.c                    |    9 +++++++++
 lxc-dave/lib/kobject.c                  |   31 ++++++++++++++++++++++---------
 4 files changed, 32 insertions(+), 11 deletions(-)

lib/kobject.c renamed from lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup +22 -9

diff -puN lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup lib/kobject.c
@@ -139,23 +139,36 @@ static int ptr_in_range(void *ptr, void 
 	return 0;
 }
 
-static void verify_dynamic_kobject_allocation(struct kobject *kobj)
+void verify_dynamic_kobject_allocation(struct kobject *kobj)
 {
-	if (ptr_in_range(kobj, &_sdata[0], &_edata[0]))
-		goto warn;
-	if (ptr_in_range(kobj, &__bss_start[0], &__bss_stop[0]))
-		goto warn;
-	return;
-warn:
+	char *namebuf;
+	const char *ret;
+
+	namebuf = kzalloc(KSYM_NAME_LEN, GFP_KERNEL);
+	ret = kallsyms_lookup((unsigned long)kobj, NULL, NULL, NULL,
+			namebuf);
+	/*
+	 * This is the X86_32-only part of this function.
+	 * This is here because it is valid to have a kobject
+	 * in an __init section, but only after those
+	 * sections have been freed back to the dynamic pool.
+	 */
+	if (!initmem_now_dynamic &&
+	    ptr_in_range(kobj, __init_begin, __init_end))
+		goto out;
+	if (!ret || !strlen(ret))
+		goto out;
 	pr_debug("---- begin silly warning ----\n");
 	pr_debug("This is a janitorial warning, not a kernel bug.\n");
 #ifdef CONFIG_DEBUG_KOBJECT
-	print_symbol("The kobject at, or inside %s is not dynamically allocated.\n",
-			(unsigned long)kobj);
+	pr_debug("The kobject at, or inside '%s'@(0x%p) is not dynamically allocated.\n",
+			namebuf, kobj);
 #endif
 	pr_debug("kobjects must be dynamically allocated, not static\n");
 	/* dump_stack(); */
 	pr_debug("---- end silly warning ----\n");
+out:
+	kfree(namebuf);
 }
 #else
 static void verify_dynamic_kobject_allocation(struct kobject *kobj)

diff -L sre -puN /dev/null /dev/null

arch/i386/kernel/vmlinux.lds.S renamed from arch/i386/kernel/vmlinux.lds.S~make-kobject-allocation-debugging-check-use-kallsyms_lookup +0 -2

diff -puN arch/i386/kernel/vmlinux.lds.S~make-kobject-allocation-debugging-check-use-kallsyms_lookup arch/i386/kernel/vmlinux.lds.S
@@ -71,8 +71,6 @@ SECTIONS
   	__tracedata_end = .;
   }
 
-  _sdata = .;			/* End of text section */
-
   RODATA
 
   /* writeable */

init/main.c renamed from init/main.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup +9 -0

diff -puN init/main.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup init/main.c
@@ -771,12 +771,21 @@ static void run_init_process(char *init_
 	kernel_execve(init_filename, argv_init, envp_init);
 }
 
+/*
+ * __init/__init_data sections are turned into normal
+ * dynamically allocated memory later in boot.  When
+ * this is 0, the memory is for the __init purposes,
+ * when it it some other value, the memory is dynamic.
+ */
+int initmem_now_dynamic;
+
 /* This is a non __init function. Force it to be noinline otherwise gcc
  * makes it inline to init() and it becomes part of init.text section
  */
 static int noinline init_post(void)
 {
 	free_initmem();
+	initmem_now_dynamic = 1;
 	unlock_kernel();
 	mark_rodata_ro();
 	system_state = SYSTEM_RUNNING;

diff -puN lib/Makefile~make-kobject-allocation-debugging-check-use-kallsyms_lookup lib/Makefile diff -puN include/linux/kernel.h~make-kobject-allocation-debugging-check-use-kallsyms_lookup include/linux/kernel.h diff -puN arch/i386/mm/init.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup arch/i386/mm/init.c

include/linux/init.h renamed from include/linux/init.h~make-kobject-allocation-debugging-check-use-kallsyms_lookup +1 -0

diff -puN include/linux/init.h~make-kobject-allocation-debugging-check-use-kallsyms_lookup include/linux/init.h
@@ -83,6 +83,7 @@ extern initcall_t __security_initcall_st
 extern char __initdata boot_command_line[];
 extern char *saved_command_line;
 extern unsigned int reset_devices;
+extern int initmem_now_dynamic;
 
 /* used by init/main.c */
 void setup_arch(char **);

Satyam Sharma

2007-08-23 05:05 UTC #1 lore raw

Hi Dave,

Quoted 4 lines · Dave Hansen

One of the top ten sysfs problems is that users use statically allocated kobjects. This patch reminds them that this is a naughty thing.

Hmm, I might've missed previous discussion regarding this, but I'm curious to know why using statically allocated kobjects is "naughty". The code / warnings / printk messages below indicate this is only a "silly/janitorial" issue?

The reason I ask is that if it is serious, there is no reason why this check cannot be done at build-time itself, instead of this runtime kludge in the kernel -- catching potential bugs at build-time is always desirable. Modpost or some such tool can be taught to detect kobjects allocated statically in vmlinux/modules -- or there can be other build- time solutions, possibly. Have you considered such an approach?

Thanks,

Satyam

Greg KH

2007-08-23 08:34 UTC #2 lore raw

Quoted 14 lines · Satyam Sharma

Hi Dave,
Quoted 4 lines · Dave Hansen
One of the top ten sysfs problems is that users use statically allocated kobjects. This patch reminds them that this is a naughty thing.
Hmm, I might've missed previous discussion regarding this, but I'm curious to know why using statically allocated kobjects is "naughty". The code / warnings / printk messages below indicate this is only a "silly/janitorial" issue?

Quoted 6 lines

The reason I ask is that if it is serious, there is no reason why this check cannot be done at build-time itself, instead of this runtime kludge in the kernel -- catching potential bugs at build-time is always desirable. Modpost or some such tool can be taught to detect kobjects allocated statically in vmlinux/modules -- or there can be other build- time solutions, possibly. Have you considered such an approach?

If you have a patch to do so, I would appreciate it.

But the main reason I don't push such a change into Linus's tree is that there are still a lot of statically allocated kobjects today, like all driver definitions :(

Converting them to be dynamic is on my list of things to do, it's just a ways down there.

thanks,

greg k-h

Paulo Marques

2007-08-23 13:51 UTC #3 lore raw

In reply to Dave Hansen

Quoted 8 lines · Dave Hansen

One of the top ten sysfs problems is that users use statically allocated kobjects. This patch reminds them that this is a naughty thing.
One _really_ nice thing this patch does, is us the kallsyms mechanism to print out exactly which symbol is being complained about:

Quoted 6 lines

	The kobject at, or inside 'statickobj.2'@(0xc040d020) is not dynamically allocated.
This patch replaces the previous implementation's use of a _sdata symbol in favor of using kallsyms_lookup(). If a kobject's address is a resolvable symbol, then it isn't dynamically allocated.

Just a few concerns that I'm not sure of having been addressed:

- doing a kallsyms_lookup() is more expensive then just a simple range
test. This might be a concern if this is called very often. In this case you could keep the range check and only do the lookup for symbols that fail that check

- kallsyms_lookup() never finds a symbol if CONFIG_KALLSYMS is not
selected

- more comments below

Quoted 23 lines

The one exception to this is init symbols. The patch also checks to see whether __init memory has been freed and if it has will allow kobjects in those sections.
Signed-off-by Dave Hansen <[email protected]>
---
 lxc-dave/arch/i386/kernel/vmlinux.lds.S |    2 --
 lxc-dave/include/linux/init.h           |    1 +
 lxc-dave/init/main.c                    |    9 +++++++++
 lxc-dave/lib/kobject.c                  |   31 ++++++++++++++++++++++---------
 4 files changed, 32 insertions(+), 11 deletions(-)
diff -puN lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup lib/kobject.c
lib/kobject.c renamed from lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup +1 -1
@@ -139,23 +139,36 @@ static int ptr_in_range(void *ptr, void 
 	return 0;
 }
 
-static void verify_dynamic_kobject_allocation(struct kobject *kobj)
+void verify_dynamic_kobject_allocation(struct kobject *kobj)
 {

lib/kobject.c static int ptr_in_range(void *ptr, void · line 147

Quoted 10 lines

lib/kobject.c renamed from lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup +4 -6

-	if (ptr_in_range(kobj, &_sdata[0], &_edata[0]))
-		goto warn;
-	if (ptr_in_range(kobj, &__bss_start[0], &__bss_stop[0]))
-		goto warn;
-	return;
-warn:
+	char *namebuf;
+	const char *ret;
+
+	namebuf = kzalloc(KSYM_NAME_LEN, GFP_KERNEL);

You don't need kzalloc here. kmalloc will do just fine.

lib/kobject.c static int ptr_in_range(void *ptr, void · line 159

Quoted 12 lines

lib/kobject.c renamed from lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup +12 -0

+	ret = kallsyms_lookup((unsigned long)kobj, NULL, NULL, NULL,
+			namebuf);
+	/*
+	 * This is the X86_32-only part of this function.
+	 * This is here because it is valid to have a kobject
+	 * in an __init section, but only after those
+	 * sections have been freed back to the dynamic pool.
+	 */
+	if (!initmem_now_dynamic &&
+	    ptr_in_range(kobj, __init_begin, __init_end))
+		goto out;
+	if (!ret || !strlen(ret))

The "!strlen(ret)" is not only weird (why not write as "!ret[0] or !*ret) but is also unnecessary. When kallsyms_lookup fails to find a symbol it should always return NULL.

Quoted 17 lines

lib/kobject.c renamed from lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup +5 -2

+		goto out;
 	pr_debug("---- begin silly warning ----\n");
 	pr_debug("This is a janitorial warning, not a kernel bug.\n");
 #ifdef CONFIG_DEBUG_KOBJECT
-	print_symbol("The kobject at, or inside %s is not dynamically allocated.\n",
-			(unsigned long)kobj);
+	pr_debug("The kobject at, or inside '%s'@(0x%p) is not dynamically allocated.\n",
+			namebuf, kobj);
 #endif
 	pr_debug("kobjects must be dynamically allocated, not static\n");
 	/* dump_stack(); */
 	pr_debug("---- end silly warning ----\n");
+out:
+	kfree(namebuf);
 }
 #else
[...]

Signature · 3 lines

Paulo Marques - www.grupopie.com

"You're just jealous because the voices only talk to me."