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(-)
diff -puN lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup lib/kobject.c
--- lxc/lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup 2007-08-22 14:51:50.000000000 -0700
+++ lxc-dave/lib/kobject.c 2007-08-22 14:51:50.000000000 -0700
@@ -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
diff -puN arch/i386/kernel/vmlinux.lds.S~make-kobject-allocation-debugging-check-use-kallsyms_lookup arch/i386/kernel/vmlinux.lds.S
--- lxc/arch/i386/kernel/vmlinux.lds.S~make-kobject-allocation-debugging-check-use-kallsyms_lookup 2007-08-22 14:51:50.000000000 -0700
+++ lxc-dave/arch/i386/kernel/vmlinux.lds.S 2007-08-22 14:51:50.000000000 -0700
@@ -71,8 +71,6 @@ SECTIONS
__tracedata_end = .;
}
- _sdata = .; /* End of text section */
-
RODATA
/* writeable */
diff -puN init/main.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup init/main.c
--- lxc/init/main.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup 2007-08-22 14:51:50.000000000 -0700
+++ lxc-dave/init/main.c 2007-08-22 14:51:50.000000000 -0700
@@ -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
diff -puN include/linux/init.h~make-kobject-allocation-debugging-check-use-kallsyms_lookup include/linux/init.h
--- lxc/include/linux/init.h~make-kobject-allocation-debugging-check-use-kallsyms_lookup 2007-08-22 14:51:50.000000000 -0700
+++ lxc-dave/include/linux/init.h 2007-08-22 14:51:50.000000000 -0700
@@ -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 **);
_
Hi Dave,
On Wed, 22 Aug 2007, Dave Hansen wrote:
>
> 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
On Thu, Aug 23, 2007 at 10:48:23AM +0530, Satyam Sharma wrote:
> Hi Dave,
>
>
> On Wed, 22 Aug 2007, Dave Hansen wrote:
> >
> > 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?
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
Dave Hansen wrote:
> 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.
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
> 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
> --- lxc/lib/kobject.c~make-kobject-allocation-debugging-check-use-kallsyms_lookup 2007-08-22 14:51:50.000000000 -0700
> +++ lxc-dave/lib/kobject.c 2007-08-22 14:51:50.000000000 -0700
> @@ -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);
You don't need kzalloc here. kmalloc will do just fine.
> + 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.
> + 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
> [...]
--
Paulo Marques - http://www.grupopie.com
"You're just jealous because the voices only talk to me."