[RFC] [patch 1/2] add non_init_kernel_text_address

[RFC] [patch 1/2] add non_init_kernel_text_address

[Srinivasa Ds]

Since __init functions are discarded and its memory freed once
initialization completes, It would be better if we enable kprobes 
to refuse probing __init functions. The attached patchset will do 
that.

This patch creates non_init_kernel_text_address() to identify
non_init text area.

Iam open to suggestions for a better functionname. 

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com> 
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>


---
 include/linux/kernel.h |    2 ++
 kernel/extable.c       |   16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.24-rc5-mm1/include/linux/kernel.h
===================================================================
--- linux-2.6.24-rc5-mm1.orig/include/linux/kernel.h
+++ linux-2.6.24-rc5-mm1/include/linux/kernel.h
@@ -169,6 +169,8 @@ extern unsigned long long memparse(char 
 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 non_init_kernel_text_address(unsigned long addr);
+extern int non_init_core_kernel_text(unsigned long addr);
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
Index: linux-2.6.24-rc5-mm1/kernel/extable.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/extable.c
+++ linux-2.6.24-rc5-mm1/kernel/extable.c
@@ -40,11 +40,18 @@ const struct exception_table_entry *sear
 	return e;
 }
 
-int core_kernel_text(unsigned long addr)
+int non_init_core_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
 		return 1;
+	return 0;
+}
+
+int core_kernel_text(unsigned long addr)
+{
+	if (non_init_core_kernel_text(addr))
+		return 1;
 
 	if (addr >= (unsigned long)_sinittext &&
 	    addr <= (unsigned long)_einittext)
@@ -65,3 +72,10 @@ int kernel_text_address(unsigned long ad
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
+int non_init_kernel_text_address(unsigned long addr)
+{
+	if (non_init_core_kernel_text(addr))
+		return 1;
+	return module_text_address(addr) != NULL;
+}

[RFC] [patch 2/2] Refuse kprobe insertion on __init section code

[Srinivasa Ds]

This patch makes use of non_init_kernel_text_address() to avoid
probing __init functions.

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>


---
 kernel/kprobes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6.24-rc5-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/kprobes.c
+++ linux-2.6.24-rc5-mm1/kernel/kprobes.c
@@ -520,7 +520,7 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = (kprobe_opcode_t *)(((char *)p->addr)+ p->offset);
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
+	if (!non_init_kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr))
 		return -EINVAL;
 
@@ -662,7 +662,7 @@ int __kprobes register_jprobe(struct jpr
 {
 	unsigned long addr = arch_deref_entry_point(jp->entry);
 
-	if (!kernel_text_address(addr))
+	if (!non_init_kernel_text_address(addr))
 		return -EINVAL;
 
 	/* Todo: Verify probepoint is a function entry point */

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Andrew Morton]

On Fri, 14 Dec 2007 12:25:30 +0530 Srinivasa Ds <srinivasa@in.ibm.com> wrote:

> Since __init functions are discarded and its memory freed once
> initialization completes, It would be better if we enable kprobes 
> to refuse probing __init functions. The attached patchset will do 
> that.
> 
> This patch creates non_init_kernel_text_address() to identify
> non_init text area.
> 
> Iam open to suggestions for a better functionname. 
> 

It's not a great name.  One wonders how it handles __exit text, for example.

regular_kernel_text_address()?  Dunno.

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Ananth N Mavinakayanahalli]

On Thu, Dec 13, 2007 at 11:09:16PM -0800, Andrew Morton wrote:
> On Fri, 14 Dec 2007 12:25:30 +0530 Srinivasa Ds <srinivasa@in.ibm.com> wrote:
> 
> > Since __init functions are discarded and its memory freed once
> > initialization completes, It would be better if we enable kprobes 
> > to refuse probing __init functions. The attached patchset will do 
> > that.
> > 
> > This patch creates non_init_kernel_text_address() to identify
> > non_init text area.
> > 
> > Iam open to suggestions for a better functionname. 
> > 
> 
> It's not a great name.  One wonders how it handles __exit text, for example.

When registering kprobes on modules, we 'get' a module refcount so the
probed module doesn't disappear from under us. When the probe is
unregistered, we 'put' the refcount. That works great for __exit text.

> regular_kernel_text_address()?  Dunno.

Sounds better :-)

Ananth

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Rusty Russell]

On Friday 14 December 2007 18:51:06 Ananth N Mavinakayanahalli wrote:
> On Thu, Dec 13, 2007 at 11:09:16PM -0800, Andrew Morton wrote:
> > regular_kernel_text_address()?  Dunno.
>
> Sounds better :-)

The better answer was to invert it and use "discarded_kernel_text_address()",
which is what you actually care about (rather than the details of whether it
was init or not).

However, you have, in fact, located a potential bug.  If someone were to
kmalloc module text, then symbol_put() could fail.

How's this?
---
Don't report discarded init pages as kernel text.

In theory this could cause a bug in symbol_put() if an arch used for
a module: we might think the symbol belongs to the core kernel.

The downside is that this might make backtraces through (discarded)
init functions harder to read on some archs.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 0eabf082c13a kernel/extable.c
--- a/kernel/extable.c	Tue Dec 18 13:51:13 2007 +1100
+++ b/kernel/extable.c	Tue Dec 18 15:53:01 2007 +1100
@@ -46,7 +46,8 @@ int core_kernel_text(unsigned long addr)
 	    addr <= (unsigned long)_etext)
 		return 1;
 
-	if (addr >= (unsigned long)_sinittext &&
+	if (system_state == SYSTEM_BOOTING &&
+	    addr >= (unsigned long)_sinittext &&
 	    addr <= (unsigned long)_einittext)
 		return 1;
 	return 0;

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Srinivasa Ds]

Rusty Russell wrote:
> On Friday 14 December 2007 18:51:06 Ananth N Mavinakayanahalli wrote:
>> On Thu, Dec 13, 2007 at 11:09:16PM -0800, Andrew Morton wrote:
>>> regular_kernel_text_address()?  Dunno.
>> Sounds better :-)
> 
> The better answer was to invert it and use "discarded_kernel_text_address()",
> which is what you actually care about (rather than the details of whether it
> was init or not).

Requirement is to ensure the address is really a kernel_text address and doesn't
lie in __init section. Hence I used persistent_kernel_text_address().

> 
> However, you have, in fact, located a potential bug.  If someone were to
> kmalloc module text, then symbol_put() could fail.

I don't think so, symbol_put() makes use of lookup_symbol() within __start_ksymtab
and  stop_ksymtab. 

> 
> How's this?
> ---
> Don't report discarded init pages as kernel text.
> 
> In theory this could cause a bug in symbol_put() if an arch used for
> a module: we might think the symbol belongs to the core kernel.

Yes, usage of symbol_put_addr() cause the BUG() if it fails
to find the address in core kernel.
> 
> The downside is that this might make backtraces through (discarded)
> init functions harder to read on some archs.
> 

I think it is better to make use of new function than sacrificing 
__init function symbol information in backtrace.

Thanks
 Srinivasa DS

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Rusty Russell]

On Tuesday 18 December 2007 17:46:15 Srinivasa Ds wrote:
> Rusty Russell wrote:
> > On Friday 14 December 2007 18:51:06 Ananth N Mavinakayanahalli wrote:
> >> On Thu, Dec 13, 2007 at 11:09:16PM -0800, Andrew Morton wrote:
> >>> regular_kernel_text_address()?  Dunno.
> >>
> >> Sounds better :-)
> >
> > The better answer was to invert it and use
> > "discarded_kernel_text_address()", which is what you actually care about
> > (rather than the details of whether it was init or not).
>
> Requirement is to ensure the address is really a kernel_text address and
> doesn't lie in __init section. Hence I used
> persistent_kernel_text_address().

Hi Srinivasa!

    That's not my point.  What you care about is that the text still be there.  
The fact that it's the __init section which is discarded is a detail; if some 
other text section were discarded you'd want that excluded too.  Hence 
non_init_ was a bad name; persistent is a bad name because it usually means 
something else in operating systems...

> > However, you have, in fact, located a potential bug.  If someone were to
> > kmalloc module text, then symbol_put() could fail.
>
> I don't think so, symbol_put() makes use of lookup_symbol() within
> __start_ksymtab and  stop_ksymtab.

Sorry, I meant symbol_put_addr().

> > How's this?
> > ---
> > Don't report discarded init pages as kernel text.
> >
> > In theory this could cause a bug in symbol_put() if an arch used for
> > a module: we might think the symbol belongs to the core kernel.
>
> Yes, usage of symbol_put_addr() cause the BUG() if it fails
> to find the address in core kernel.

No, symbol_put_addr() will fail to decrement the module count, thinking it's 
part of the (now-discarded) init section.

> > The downside is that this might make backtraces through (discarded)
> > init functions harder to read on some archs.
>
> I think it is better to make use of new function than sacrificing
> __init function symbol information in backtrace.

Perhaps, but two new functions is v. ugly.  I'll try to come up with something 
neater, and audit all the callers.

Thanks,
Rusty.

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Rusty Russell]

On Tuesday 18 December 2007 18:23:53 Rusty Russell wrote:
> On Tuesday 18 December 2007 17:46:15 Srinivasa Ds wrote:
> > Rusty Russell wrote:
> > > The downside is that this might make backtraces through (discarded)
> > > init functions harder to read on some archs.
> >
> > I think it is better to make use of new function than sacrificing
> > __init function symbol information in backtrace.
>
> Perhaps, but two new functions is v. ugly.  I'll try to come up with
> something neater, and audit all the callers.

Actually, given that we already have this same issue with modules (you won't
see the discarded init sections in backtraces) where it's a more frequent
issue, I can't justify adding a "discarded_sections" flag to all callers.

So here's a repeat of my original patch:

Subject: Don't report discarded init pages as kernel text.

Current code could cause a bug in symbol_put_addr() if an arch used
kmalloc module text: we might think the symbol belongs to the core
kernel.

The downside is that this might make backtraces through (discarded)
init functions harder to read on some archs, but we already have that
issue for modules and noone has complained.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 0eabf082c13a kernel/extable.c
--- a/kernel/extable.c	Tue Dec 18 13:51:13 2007 +1100
+++ b/kernel/extable.c	Tue Dec 18 15:53:01 2007 +1100
@@ -46,7 +46,8 @@ int core_kernel_text(unsigned long addr)
 	    addr <= (unsigned long)_etext)
 		return 1;
 
-	if (addr >= (unsigned long)_sinittext &&
+	if (system_state == SYSTEM_BOOTING &&
+	    addr >= (unsigned long)_sinittext &&
 	    addr <= (unsigned long)_einittext)
 		return 1;
 	return 0;

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Srinivasa Ds]

Rusty Russell wrote:

> 
> Subject: Don't report discarded init pages as kernel text.
> 
> Current code could cause a bug in symbol_put_addr() if an arch used
> kmalloc module text: we might think the symbol belongs to the core
> kernel.
> 
> The downside is that this might make backtraces through (discarded)
> init functions harder to read on some archs, but we already have that
> issue for modules and noone has complained.


Thanks Rusty,  This patch fixes my problem.
Tested-by: Srinivasa DS <srinivasa@in.ibm.com>
 

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Srinivasa Ds]

Resending the patch, by changing the name as suggested by Andrew.

Since __init functions are discarded and its memory freed once
initialization completes, It would be better if we enable kprobes 
to refuse probing __init functions. The attached patchset will do 
that.

This patch creates non_init_kernel_text_address() to identify
non_init text area.


Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com> 
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>



---
 include/linux/kernel.h |    2 ++
 kernel/extable.c       |   16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.24-rc5-mm1/include/linux/kernel.h
===================================================================
--- linux-2.6.24-rc5-mm1.orig/include/linux/kernel.h
+++ linux-2.6.24-rc5-mm1/include/linux/kernel.h
@@ -169,6 +169,8 @@ extern unsigned long long memparse(char 
 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 regular_kernel_text_address(unsigned long addr);
+extern int non_init_core_kernel_text(unsigned long addr);
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
Index: linux-2.6.24-rc5-mm1/kernel/extable.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/extable.c
+++ linux-2.6.24-rc5-mm1/kernel/extable.c
@@ -40,11 +40,18 @@ const struct exception_table_entry *sear
 	return e;
 }
 
-int core_kernel_text(unsigned long addr)
+int non_init_core_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
 		return 1;
+	return 0;
+}
+
+int core_kernel_text(unsigned long addr)
+{
+	if (non_init_core_kernel_text(addr))
+		return 1;
 
 	if (addr >= (unsigned long)_sinittext &&
 	    addr <= (unsigned long)_einittext)
@@ -65,3 +72,10 @@ int kernel_text_address(unsigned long ad
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
+int regular_kernel_text_address(unsigned long addr)
+{
+	if (non_init_core_kernel_text(addr))
+		return 1;
+	return module_text_address(addr) != NULL;
+}

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Srinivasa Ds]

Forgot to change non_init_core_kernel_text() to regular_core_kernel_text().
Resending the patch.

Since __init functions are discarded and its memory freed once
initialization completes, It would be better if we enable kprobes 
to refuse probing __init functions. The attached patchset will do 
that.

This patch creates non_init_kernel_text_address() to identify
non_init text area.


Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com> 
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>


---
 include/linux/kernel.h |    2 ++
 kernel/extable.c       |   16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.24-rc5-mm1/include/linux/kernel.h
===================================================================
--- linux-2.6.24-rc5-mm1.orig/include/linux/kernel.h
+++ linux-2.6.24-rc5-mm1/include/linux/kernel.h
@@ -169,6 +169,8 @@ extern unsigned long long memparse(char 
 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 regular_kernel_text_address(unsigned long addr);
+extern int regular_core_kernel_text(unsigned long addr);
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
Index: linux-2.6.24-rc5-mm1/kernel/extable.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/extable.c
+++ linux-2.6.24-rc5-mm1/kernel/extable.c
@@ -40,11 +40,18 @@ const struct exception_table_entry *sear
 	return e;
 }
 
-int core_kernel_text(unsigned long addr)
+int regular_core_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
 		return 1;
+	return 0;
+}
+
+int core_kernel_text(unsigned long addr)
+{
+	if (regular_core_kernel_text(addr))
+		return 1;
 
 	if (addr >= (unsigned long)_sinittext &&
 	    addr <= (unsigned long)_einittext)
@@ -65,3 +72,10 @@ int kernel_text_address(unsigned long ad
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
+int regular_kernel_text_address(unsigned long addr)
+{
+	if (regular_core_kernel_text(addr))
+		return 1;
+	return module_text_address(addr) != NULL;
+}

Re: [RFC] [patch 1/2] add non_init_kernel_text_address

[Johannes Weiner]

Hi,

Srinivasa Ds <srinivasa@in.ibm.com> writes:

> Forgot to change non_init_core_kernel_text() to regular_core_kernel_text().
> Resending the patch.
>
> [...]
>
> This patch creates non_init_kernel_text_address() to identify
> non_init text area.

You still refer to the old name here :)

How about persistent_core_kernel_text() and
persistent_kernel_text_address()?

	Hannes

[RFC] [patch 1/2] add non_init_kernel_text_address

[Srinivasa Ds]

Changing regular_kernel_text_address() to persistent_kernel_text_address().

Since __init functions are discarded and its memory freed once
initialization completes, It would be better if we enable kprobes
to refuse probing __init functions. The attached patchset will do
that.

This patch creates persistent_kernel_text_address() to identify
"non_init" text area.


Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>



---
 include/linux/kernel.h |    2 ++
 kernel/extable.c       |   16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.24-rc5-mm1/include/linux/kernel.h
===================================================================
--- linux-2.6.24-rc5-mm1.orig/include/linux/kernel.h
+++ linux-2.6.24-rc5-mm1/include/linux/kernel.h
@@ -169,6 +169,8 @@ extern unsigned long long memparse(char 
 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 persistent_kernel_text_address(unsigned long addr);
+extern int persistent_core_kernel_text(unsigned long addr);
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
Index: linux-2.6.24-rc5-mm1/kernel/extable.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/extable.c
+++ linux-2.6.24-rc5-mm1/kernel/extable.c
@@ -40,11 +40,18 @@ const struct exception_table_entry *sear
 	return e;
 }
 
-int core_kernel_text(unsigned long addr)
+int persistent_core_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_stext &&
 	    addr <= (unsigned long)_etext)
 		return 1;
+	return 0;
+}
+
+int core_kernel_text(unsigned long addr)
+{
+	if (persistent_core_kernel_text(addr))
+		return 1;
 
 	if (addr >= (unsigned long)_sinittext &&
 	    addr <= (unsigned long)_einittext)
@@ -65,3 +72,10 @@ int kernel_text_address(unsigned long ad
 		return 1;
 	return module_text_address(addr) != NULL;
 }
+
+int persistent_kernel_text_address(unsigned long addr)
+{
+	if (persistent_core_kernel_text(addr))
+		return 1;
+	return module_text_address(addr) != NULL;
+}

[RFC] [patch 2/2] Refuse kprobe insertion on __init section code

[Srinivasa Ds]

This patch makes use of persistent_kernel_text_address() to avoid
probing __init functions.

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>




---
 kernel/kprobes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6.24-rc5-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/kprobes.c
+++ linux-2.6.24-rc5-mm1/kernel/kprobes.c
@@ -520,7 +520,7 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = (kprobe_opcode_t *)(((char *)p->addr)+ p->offset);
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
+	if (!persistent_kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr))
 		return -EINVAL;
 
@@ -662,7 +662,7 @@ int __kprobes register_jprobe(struct jpr
 {
 	unsigned long addr = arch_deref_entry_point(jp->entry);
 
-	if (!kernel_text_address(addr))
+	if (!persistent_kernel_text_address(addr))
 		return -EINVAL;
 
 	/* Todo: Verify probepoint is a function entry point */

Re: [RFC] [patch 2/2] Refuse kprobe insertion on __init section code

[Srinivasa Ds]

This patch makes use of regular_kernel_text_address() to avoid
probing __init functions.

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>




---
 kernel/kprobes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6.24-rc5-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.24-rc5-mm1.orig/kernel/kprobes.c
+++ linux-2.6.24-rc5-mm1/kernel/kprobes.c
@@ -520,7 +520,7 @@ static int __kprobes __register_kprobe(s
 		return -EINVAL;
 	p->addr = (kprobe_opcode_t *)(((char *)p->addr)+ p->offset);
 
-	if (!kernel_text_address((unsigned long) p->addr) ||
+	if (!regular_kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr))
 		return -EINVAL;
 
@@ -662,7 +662,7 @@ int __kprobes register_jprobe(struct jpr
 {
 	unsigned long addr = arch_deref_entry_point(jp->entry);
 
-	if (!kernel_text_address(addr))
+	if (!regular_kernel_text_address(addr))
 		return -EINVAL;
 
 	/* Todo: Verify probepoint is a function entry point */