PATCH] debug: add notifier chain debugging

PATCH] debug: add notifier chain debugging

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] debug: add notifier chain debugging

during some development we suspected a case where we left something
in a notifier chain that was from a module that was unloaded already...
and that sort of thing is rather hard to track down.

This patch adds a very simple sanity check (which isn't all that
expensive) to make sure the notifier we're about to call is
actually from either the kernel itself of from a still-loaded
module, avoiding a hard-to-chase-down crash.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 kernel/notifier.c |   16 ++++++++++++++++
 lib/Kconfig.debug |   10 ++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index 823be11..143fdd7 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -21,6 +21,10 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
 static int notifier_chain_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
+	if (!kernel_text_address((unsigned long)n->notifier_call)) {
+		WARN(1, "Invalid notifier registered!");
+		return 0;
+	}
 	while ((*nl) != NULL) {
 		if (n->priority > (*nl)->priority)
 			break;
@@ -34,6 +38,10 @@ static int notifier_chain_register(struct notifier_block **nl,
 static int notifier_chain_cond_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
+	if (!kernel_text_address((unsigned long)n->notifier_call)) {
+		WARN(1, "Invalid notifier registered!");
+		return 0;
+	}
 	while ((*nl) != NULL) {
 		if ((*nl) == n)
 			return 0;
@@ -82,6 +90,14 @@ static int __kprobes notifier_call_chain(struct notifier_block **nl,
 
 	while (nb && nr_to_call) {
 		next_nb = rcu_dereference(nb->next);
+
+#ifdef CONFIG_DEBUG_NOTIFIERS
+		if (!kernel_text_address((unsigned long)nb->notifier_call)) {
+			WARN(1, "Invalid notifier called!");
+			nb = next_nb;
+			continue;
+		}
+#endif
 		ret = nb->notifier_call(nb, val, v);
 
 		if (nr_calls)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 800ac84..f4bb36e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -536,6 +536,16 @@ config DEBUG_SG
 
 	  If unsure, say N.
 
+config DEBUG_NOTIFIERS
+	bool "Debug notifier call chains"
+	depends on DEBUG_KERNEL
+	help
+	  Enable this to turn on sanity checking for notifier call chains.
+	  This is most useful for kernel developers to make sure that
+	  modules properly unregister themselves from notifier chains.
+	  This is a relatively cheap check but if you care about maximum
+	  performance, say N.
+
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	depends on DEBUG_KERNEL && \
-- 
1.5.5.1

Re: PATCH] debug: add notifier chain debugging

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: [PATCH] debug: add notifier chain debugging
> 
> during some development we suspected a case where we left something in 
> a notifier chain that was from a module that was unloaded already... 
> and that sort of thing is rather hard to track down.
> 
> This patch adds a very simple sanity check (which isn't all that 
> expensive) to make sure the notifier we're about to call is actually 
> from either the kernel itself of from a still-loaded module, avoiding 
> a hard-to-chase-down crash.

applied for testing, thanks Arjan.

	Ingo

Re: PATCH] debug: add notifier chain debugging

[Andrew Morton]

On Fri, 15 Aug 2008 15:29:38 -0700 Arjan van de Ven <arjan@infradead.org> wrote:

> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: [PATCH] debug: add notifier chain debugging
> 
> during some development we suspected a case where we left something
> in a notifier chain that was from a module that was unloaded already...
> and that sort of thing is rather hard to track down.
> 
> This patch adds a very simple sanity check (which isn't all that
> expensive) to make sure the notifier we're about to call is
> actually from either the kernel itself of from a still-loaded
> module, avoiding a hard-to-chase-down crash.
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  kernel/notifier.c |   16 ++++++++++++++++
>  lib/Kconfig.debug |   10 ++++++++++
>  2 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index 823be11..143fdd7 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -21,6 +21,10 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
>  static int notifier_chain_register(struct notifier_block **nl,
>  		struct notifier_block *n)
>  {
> +	if (!kernel_text_address((unsigned long)n->notifier_call)) {
> +		WARN(1, "Invalid notifier registered!");
> +		return 0;
> +	}
>  	while ((*nl) != NULL) {
>  		if (n->priority > (*nl)->priority)
>  			break;
> @@ -34,6 +38,10 @@ static int notifier_chain_register(struct notifier_block **nl,
>  static int notifier_chain_cond_register(struct notifier_block **nl,
>  		struct notifier_block *n)
>  {
> +	if (!kernel_text_address((unsigned long)n->notifier_call)) {
> +		WARN(1, "Invalid notifier registered!");
> +		return 0;
> +	}

Seems strange to add checks to the registration functions.  What could
be that broken?

>  	while ((*nl) != NULL) {
>  		if ((*nl) == n)
>  			return 0;
> @@ -82,6 +90,14 @@ static int __kprobes notifier_call_chain(struct notifier_block **nl,
>  
>  	while (nb && nr_to_call) {
>  		next_nb = rcu_dereference(nb->next);
> +
> +#ifdef CONFIG_DEBUG_NOTIFIERS
> +		if (!kernel_text_address((unsigned long)nb->notifier_call)) {
> +			WARN(1, "Invalid notifier called!");
> +			nb = next_nb;
> +			continue;
> +		}
> +#endif
>  		ret = nb->notifier_call(nb, val, v);
>  
>  		if (nr_calls)
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 800ac84..f4bb36e 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -536,6 +536,16 @@ config DEBUG_SG
>  
>  	  If unsure, say N.
>  
> +config DEBUG_NOTIFIERS
> +	bool "Debug notifier call chains"
> +	depends on DEBUG_KERNEL
> +	help
> +	  Enable this to turn on sanity checking for notifier call chains.
> +	  This is most useful for kernel developers to make sure that
> +	  modules properly unregister themselves from notifier chains.
> +	  This is a relatively cheap check but if you care about maximum
> +	  performance, say N.
> +

If we remove the first two checks then surely we can afford to add the
remaining check unconditionally and lose the new config option and its
ifdef.

Re: PATCH] debug: add notifier chain debugging

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > +config DEBUG_NOTIFIERS
> > +	bool "Debug notifier call chains"
> > +	depends on DEBUG_KERNEL
> > +	help
> > +	  Enable this to turn on sanity checking for notifier call chains.
> > +	  This is most useful for kernel developers to make sure that
> > +	  modules properly unregister themselves from notifier chains.
> > +	  This is a relatively cheap check but if you care about maximum
> > +	  performance, say N.
> > +
> 
> If we remove the first two checks then surely we can afford to add the
> remaining check unconditionally and lose the new config option and its
> ifdef.

yeah, that's sensible. Arjan?

	Ingo

Re: PATCH] debug: add notifier chain debugging

[Arjan van de Ven]

On Tue, 19 Aug 2008 21:09:32 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> 
> If we remove the first two checks then surely we can afford to add the
> remaining check unconditionally and lose the new config option and its
> ifdef.
> 

Andi told me that this notifier is a hotpath for kprobes (systemtap)
use models; at which point making this a config option makes sort of
sense to get the last bit of performance out of that. Personally I would
always want this option on for anything I run, and I suspect
(enterprise) distros will just always turn it on as well since it's a
good sanity check at low cost.

I'd be happy to lose the config (I think we have way too many unneeded
ones of those already); I'll make a patch.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] debug: add notifier chain debugging

[Christoph Hellwig]

On Fri, Aug 22, 2008 at 07:00:54AM -0700, Arjan van de Ven wrote:
> On Tue, 19 Aug 2008 21:09:32 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > 
> > If we remove the first two checks then surely we can afford to add the
> > remaining check unconditionally and lose the new config option and its
> > ifdef.
> > 
> 
> Andi told me that this notifier is a hotpath for kprobes (systemtap)
> use models; at which point making this a config option makes sort of
> sense to get the last bit of performance out of that. Personally I would
> always want this option on for anything I run, and I suspect
> (enterprise) distros will just always turn it on as well since it's a
> good sanity check at low cost.
> 
> I'd be happy to lose the config (I think we have way too many unneeded
> ones of those already); I'll make a patch.

What about checking yourself? At least the braindead use of notiiers in
the pagefault path for kprobes (even unconditional on x86_64) is gone
now and replaced with proper kprobes hooks. The die notifier that's
also used by kprobes shouldn't be in a performance critical path.

Re: PATCH] debug: add notifier chain debugging

[Tony Luck]

On Fri, Aug 15, 2008 at 3:29 PM, Arjan van de Ven <arjan@infradead.org> wrote:
> +       if (!kernel_text_address((unsigned long)n->notifier_call)) {
> +               WARN(1, "Invalid notifier registered!");
> +               return 0;
> +       }

This breaks on ia64 (and pa-risc I think) where function pointers don't point
directly at the code, they point to a {code,data} structure which is itself
located in data space, not text space.

-Tony

Re: PATCH] debug: add notifier chain debugging

[Arjan van de Ven]

On Mon, 25 Aug 2008 15:39:13 -0700
"Tony Luck" <tony.luck@intel.com> wrote:

> On Fri, Aug 15, 2008 at 3:29 PM, Arjan van de Ven
> <arjan@infradead.org> wrote:
> > +       if (!kernel_text_address((unsigned long)n->notifier_call)) {
> > +               WARN(1, "Invalid notifier registered!");
> > +               return 0;
> > +       }
> 
> This breaks on ia64 (and pa-risc I think) where function pointers
> don't point directly at the code, they point to a {code,data}
> structure which is itself located in data space, not text space.

is there a way to go to the actual address? I'm sure this is a bit more
common.... (like kallsyms!)


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] debug: add notifier chain debugging

[Tony Luck]

>> This breaks on ia64 (and pa-risc I think) where function pointers
>> don't point directly at the code, they point to a {code,data}
>> structure which is itself located in data space, not text space.
>
> is there a way to go to the actual address? I'm sure this is a bit more
> common.... (like kallsyms!)

See dereference_function_descriptor() in lib/vsprintf.c (where it will
be clear that my memory was wrong and that PPC64 is the other
architecture that needs this).

Perhaps this needs to be moved to some place in asm-generic so
that it can be used by code like your sanity check?

-Tony

Re: PATCH] debug: add notifier chain debugging

[Arjan van de Ven]

On Mon, 25 Aug 2008 22:08:00 -0700
"Tony Luck" <tony.luck@intel.com> wrote:

> >> This breaks on ia64 (and pa-risc I think) where function pointers
> >> don't point directly at the code, they point to a {code,data}
> >> structure which is itself located in data space, not text space.
> >
> > is there a way to go to the actual address? I'm sure this is a bit
> > more common.... (like kallsyms!)
> 
> See dereference_function_descriptor() in lib/vsprintf.c (where it will
> be clear that my memory was wrong and that PPC64 is the other
> architecture that needs this).
> 
> Perhaps this needs to be moved to some place in asm-generic so
> that it can be used by code like your sanity check?

or we make a func_is_kernel_text() thast maps underneath...
after all that'll be a common use of this function ;)

> 
> -Tony


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] debug: add notifier chain debugging

[Arjan van de Ven]

On Mon, 25 Aug 2008 22:08:00 -0700
"Tony Luck" <tony.luck@intel.com> wrote:

> >> This breaks on ia64 (and pa-risc I think) where function pointers
> >> don't point directly at the code, they point to a {code,data}
> >> structure which is itself located in data space, not text space.
> >
> > is there a way to go to the actual address? I'm sure this is a bit
> > more common.... (like kallsyms!)
> 
> See dereference_function_descriptor() in lib/vsprintf.c (where it will
> be clear that my memory was wrong and that PPC64 is the other
> architecture that needs this).
> 

The patch below fixes this;
Ingo, please replace the patch with this one.


From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] debug: add notifier chain debugging

during some development we suspected a case where we left something
in a notifier chain that was from a module that was unloaded already...
and that sort of thing is rather hard to track down.

This patch adds a very simple sanity check (which isn't all that
expensive) to make sure the notifier we're about to call is
actually from either the kernel itself of from a still-loaded
module, avoiding a hard-to-chase-down crash.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Acked-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/kernel.h |    3 +++
 kernel/extable.c       |   16 ++++++++++++++++
 kernel/notifier.c      |    6 ++++++
 lib/vsprintf.c         |    2 +-
 4 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2651f80..4e1366b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -187,6 +187,9 @@ extern unsigned long long memparse(char *ptr, char **retptr);
 extern int core_kernel_text(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
+extern int func_ptr_is_kernel_text(void *ptr);
+extern void *dereference_function_descriptor(void *ptr);
+
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
diff --git a/kernel/extable.c b/kernel/extable.c
index a26cb2e..adf0cc9 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -66,3 +66,19 @@ int kernel_text_address(unsigned long addr)
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
+/*
+ * On some architectures (PPC64, IA64) function pointers
+ * are actually only tokens to some data that then holds the
+ * real function address. As a result, to find if a function
+ * pointer is part of the kernel text, we need to do some
+ * special dereferencing first.
+ */
+int func_ptr_is_kernel_text(void *ptr)
+{
+	unsigned long addr;
+	addr = (unsigned long) dereference_function_descriptor(ptr);
+	if (core_kernel_text(addr))
+		return 1;
+	return module_text_address(addr) != NULL;
+}
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 823be11..522277c 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -82,6 +82,12 @@ static int __kprobes notifier_call_chain(struct notifier_block **nl,
 
 	while (nb && nr_to_call) {
 		next_nb = rcu_dereference(nb->next);
+		if (!func_ptr_is_kernel_text(nb->notifier_call)) {
+			WARN(1, "Invalid notifier called!");
+			nb = next_nb;
+			continue;
+		}
+
 		ret = nb->notifier_call(nb, val, v);
 
 		if (nr_calls)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8d1d11..f5e5ffb 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -513,7 +513,7 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
 	return buf;
 }
 
-static inline void *dereference_function_descriptor(void *ptr)
+void *dereference_function_descriptor(void *ptr)
 {
 #if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
 	void *p;
-- 
1.5.5.1

Re: PATCH] debug: add notifier chain debugging

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> The patch below fixes this;
> Ingo, please replace the patch with this one.

well, this one is now less than cheap on x86:

> +int func_ptr_is_kernel_text(void *ptr)
> +{
> +	unsigned long addr;
> +	addr = (unsigned long) dereference_function_descriptor(ptr);
> +	if (core_kernel_text(addr))
> +		return 1;
> +	return module_text_address(addr) != NULL;
> +}

as it's rather large. So i kept the config option, and it defaults to 
off.

> +		if (!func_ptr_is_kernel_text(nb->notifier_call)) {
> +			WARN(1, "Invalid notifier called!");
> +			nb = next_nb;
> +			continue;
> +		}

and that should be an unlikely() too i guess.

i've applied v2 as a delta patch to -tip, with these changes - see the 
commit below.

	Ingo

-------------------->
>From e0f789bde8bda8ee6cf084197b6f3fc951ad241a Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <arjan@linux.intel.com>
Date: Fri, 15 Aug 2008 15:29:38 -0700
Subject: [PATCH] debug: add notifier chain debugging, v2

- unbreak ia64 (and powerpc) where function pointers dont
  point at code but at data (reported by Tony Luck)

[ mingo@elte.hu: various cleanups ]

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/kernel.h |    3 +++
 kernel/extable.c       |   16 ++++++++++++++++
 kernel/notifier.c      |   10 +---------
 lib/vsprintf.c         |    2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 1ceafa4..892529d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -187,6 +187,9 @@ extern unsigned long long memparse(char *ptr, char **retptr);
 extern int core_kernel_text(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
+extern int func_ptr_is_kernel_text(void *ptr);
+extern void *dereference_function_descriptor(void *ptr);
+
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
diff --git a/kernel/extable.c b/kernel/extable.c
index a26cb2e..adf0cc9 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -66,3 +66,19 @@ int kernel_text_address(unsigned long addr)
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
+/*
+ * On some architectures (PPC64, IA64) function pointers
+ * are actually only tokens to some data that then holds the
+ * real function address. As a result, to find if a function
+ * pointer is part of the kernel text, we need to do some
+ * special dereferencing first.
+ */
+int func_ptr_is_kernel_text(void *ptr)
+{
+	unsigned long addr;
+	addr = (unsigned long) dereference_function_descriptor(ptr);
+	if (core_kernel_text(addr))
+		return 1;
+	return module_text_address(addr) != NULL;
+}
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 143fdd7..0f39e39 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -21,10 +21,6 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
 static int notifier_chain_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
-	if (!kernel_text_address((unsigned long)n->notifier_call)) {
-		WARN(1, "Invalid notifier registered!");
-		return 0;
-	}
 	while ((*nl) != NULL) {
 		if (n->priority > (*nl)->priority)
 			break;
@@ -38,10 +34,6 @@ static int notifier_chain_register(struct notifier_block **nl,
 static int notifier_chain_cond_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
-	if (!kernel_text_address((unsigned long)n->notifier_call)) {
-		WARN(1, "Invalid notifier registered!");
-		return 0;
-	}
 	while ((*nl) != NULL) {
 		if ((*nl) == n)
 			return 0;
@@ -92,7 +84,7 @@ static int __kprobes notifier_call_chain(struct notifier_block **nl,
 		next_nb = rcu_dereference(nb->next);
 
 #ifdef CONFIG_DEBUG_NOTIFIERS
-		if (!kernel_text_address((unsigned long)nb->notifier_call)) {
+		if (unlikely(!func_ptr_is_kernel_text(nb->notifier_call))) {
 			WARN(1, "Invalid notifier called!");
 			nb = next_nb;
 			continue;
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8d1d11..f5e5ffb 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -513,7 +513,7 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
 	return buf;
 }
 
-static inline void *dereference_function_descriptor(void *ptr)
+void *dereference_function_descriptor(void *ptr)
 {
 #if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
 	void *p;

Re: PATCH] debug: add notifier chain debugging

[Arjan van de Ven]

On Wed, 27 Aug 2008 08:39:15 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > The patch below fixes this;
> > Ingo, please replace the patch with this one.
> 
> well, this one is now less than cheap on x86:

really it isn't; dereference_function_descriptor is a fall through and
basically free.

> 
> as it's rather large. So i kept the config option, and it defaults to 
> off.

huh? the consensus was that the config needed to go ... nothing changed
on that front. I agree absolutely with not introducing config options
for things that don't need it; we have too many already anyway.


> 
> > +		if (!func_ptr_is_kernel_text(nb->notifier_call)) {
> > +			WARN(1, "Invalid notifier called!");
> > +			nb = next_nb;
> > +			continue;
> > +		}
> 
> and that should be an unlikely() too i guess.

please do not add new unlikely()'s to the kernel.. not on non-hotpaths
at least