[PATCH 1/3] Make struct jprobe.entry a void *

[PATCH 1/3] Make struct jprobe.entry a void *

[Michael Ellerman]

Currently jprobe.entry is a kprobe_opcode_t *, but that's a lie. On some
platforms it doesn't point to an opcode at all, it points to a function
descriptor.

It's really a pointer to something that the arch code can turn into a
function entry point. And that's what actually happens, none of the
generic code ever looks at jprobe.entry, it's only ever dereferenced
by arch code.

So just make it a void *.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

It isn't obvious where kprobes patches should go, is anyone "the" maintainer?
Instead I've just sent this to everyone who'd touched the code lately, or
might be otherwise interested.


 include/linux/kprobes.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 23adf60..f4e53b7 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -116,7 +116,7 @@ struct kprobe {
  */
 struct jprobe {
 	struct kprobe kp;
-	kprobe_opcode_t *entry;	/* probe handling code to jump to */
+	void *entry;	/* probe handling code to jump to */
 };
 
 DECLARE_PER_CPU(struct kprobe *, current_kprobe);
-- 
1.5.1.3.g7a33b

[PATCH 2/3] Remove JPROBE_ENTRY()

[Michael Ellerman]

AFAICT now that jprobe.entry is a void *, JPROBE_ENTRY doesn't do
anything useful - so remove it ..

I've left a do-nothing version so that out-of-tree jprobes code will still
compile without modifications.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 Documentation/kprobes.txt     |    8 +-------
 include/asm-i386/kprobes.h    |    1 -
 include/asm-ia64/kprobes.h    |    2 --
 include/asm-powerpc/kprobes.h |    2 --
 include/asm-s390/kprobes.h    |    2 --
 include/asm-sparc64/kprobes.h |    1 -
 include/asm-x86_64/kprobes.h  |    1 -
 include/linux/kprobes.h       |    3 +++
 net/dccp/probe.c              |    2 +-
 net/ipv4/tcp_probe.c          |    2 +-
 10 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index da5404a..cb12ae1 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -247,12 +247,6 @@ control to Kprobes.)  If the probed function is declared asmlinkage,
 fastcall, or anything else that affects how args are passed, the
 handler's declaration must match.
 
-NOTE: A macro JPROBE_ENTRY is provided to handle architecture-specific
-aliasing of jp->entry. In the interest of portability, it is advised
-to use:
-
-	jp->entry = JPROBE_ENTRY(handler);
-
 register_jprobe() returns 0 on success, or a negative errno otherwise.
 
 4.3 register_kretprobe
@@ -518,7 +512,7 @@ long jdo_fork(unsigned long clone_flags, unsigned long stack_start,
 }
 
 static struct jprobe my_jprobe = {
-	.entry = JPROBE_ENTRY(jdo_fork)
+	.entry = jdo_fork
 };
 
 static int __init jprobe_init(void)
diff --git a/include/asm-i386/kprobes.h b/include/asm-i386/kprobes.h
index 8774d06..06f7303 100644
--- a/include/asm-i386/kprobes.h
+++ b/include/asm-i386/kprobes.h
@@ -42,7 +42,6 @@ typedef u8 kprobe_opcode_t;
 	? (MAX_STACK_SIZE) \
 	: (((unsigned long)current_thread_info()) + THREAD_SIZE - (ADDR)))
 
-#define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define ARCH_SUPPORTS_KRETPROBES
 #define  ARCH_INACTIVE_KPROBE_COUNT 0
 #define flush_insn_slot(p)	do { } while (0)
diff --git a/include/asm-ia64/kprobes.h b/include/asm-ia64/kprobes.h
index 6382e52..067d9de 100644
--- a/include/asm-ia64/kprobes.h
+++ b/include/asm-ia64/kprobes.h
@@ -82,8 +82,6 @@ struct kprobe_ctlblk {
 	struct prev_kprobe prev_kprobe[ARCH_PREV_KPROBE_SZ];
 };
 
-#define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
-
 #define ARCH_SUPPORTS_KRETPROBES
 #define  ARCH_INACTIVE_KPROBE_COUNT 1
 
diff --git a/include/asm-powerpc/kprobes.h b/include/asm-powerpc/kprobes.h
index b0e40ff..3481ade 100644
--- a/include/asm-powerpc/kprobes.h
+++ b/include/asm-powerpc/kprobes.h
@@ -73,12 +73,10 @@ typedef unsigned int kprobe_opcode_t;
 	}								\
 }
 
-#define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)((func_descr_t *)pentry)
 #define is_trap(instr)	(IS_TW(instr) || IS_TD(instr) || \
 			IS_TWI(instr) || IS_TDI(instr))
 #else
 /* Use stock kprobe_lookup_name since ppc32 doesn't use function descriptors */
-#define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)(pentry)
 #define is_trap(instr)	(IS_TW(instr) || IS_TWI(instr))
 #endif
 
diff --git a/include/asm-s390/kprobes.h b/include/asm-s390/kprobes.h
index 830fe4c..340ba10 100644
--- a/include/asm-s390/kprobes.h
+++ b/include/asm-s390/kprobes.h
@@ -46,8 +46,6 @@ typedef u16 kprobe_opcode_t;
 	? (MAX_STACK_SIZE) \
 	: (((unsigned long)current_thread_info()) + THREAD_SIZE - (ADDR)))
 
-#define JPROBE_ENTRY(pentry) (kprobe_opcode_t *)(pentry)
-
 #define ARCH_SUPPORTS_KRETPROBES
 #define ARCH_INACTIVE_KPROBE_COUNT 0
 
diff --git a/include/asm-sparc64/kprobes.h b/include/asm-sparc64/kprobes.h
index a331b7b..7f6774d 100644
--- a/include/asm-sparc64/kprobes.h
+++ b/include/asm-sparc64/kprobes.h
@@ -10,7 +10,6 @@ typedef u32 kprobe_opcode_t;
 #define BREAKPOINT_INSTRUCTION_2 0x91d02071 /* ta 0x71 */
 #define MAX_INSN_SIZE 2
 
-#define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define arch_remove_kprobe(p)	do {} while (0)
 #define  ARCH_INACTIVE_KPROBE_COUNT 0
 
diff --git a/include/asm-x86_64/kprobes.h b/include/asm-x86_64/kprobes.h
index cf53178..7db8254 100644
--- a/include/asm-x86_64/kprobes.h
+++ b/include/asm-x86_64/kprobes.h
@@ -41,7 +41,6 @@ typedef u8 kprobe_opcode_t;
 	? (MAX_STACK_SIZE) \
 	: (((unsigned long)current_thread_info()) + THREAD_SIZE - (ADDR)))
 
-#define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define ARCH_SUPPORTS_KRETPROBES
 #define  ARCH_INACTIVE_KPROBE_COUNT 1
 
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index f4e53b7..bd89285 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -119,6 +119,9 @@ struct jprobe {
 	void *entry;	/* probe handling code to jump to */
 };
 
+/* For backward compatibility with old code using JPROBE_ENTRY() */
+#define JPROBE_ENTRY(handler)	(handler)
+
 DECLARE_PER_CPU(struct kprobe *, current_kprobe);
 DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
diff --git a/net/dccp/probe.c b/net/dccp/probe.c
index 43a3adb..bae10b0 100644
--- a/net/dccp/probe.c
+++ b/net/dccp/probe.c
@@ -112,7 +112,7 @@ static struct jprobe dccp_send_probe = {
 	.kp	= {
 		.symbol_name = "dccp_sendmsg",
 	},
-	.entry	= JPROBE_ENTRY(jdccp_sendmsg),
+	.entry	= jdccp_sendmsg,
 };
 
 static int dccpprobe_open(struct inode *inode, struct file *file)
diff --git a/net/ipv4/tcp_probe.c b/net/ipv4/tcp_probe.c
index d9323df..4a7ef10 100644
--- a/net/ipv4/tcp_probe.c
+++ b/net/ipv4/tcp_probe.c
@@ -115,7 +115,7 @@ static struct jprobe tcp_probe = {
 	.kp = {
 		.symbol_name	= "tcp_rcv_established",
 	},
-	.entry	= JPROBE_ENTRY(jtcp_rcv_established),
+	.entry	= jtcp_rcv_established,
 };
 
 
-- 
1.5.1.3.g7a33b

[PATCH 3/3] Make jprobes a little safer for users

[Michael Ellerman]

I realise jprobes are a razor-blades-included type of interface, but
that doesn't mean we can't try and make them safer to use. This guy I
know once wrote code like this:

struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };

And then his kernel exploded. Oops.

This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
which takes the void * in a struct jprobe, and gives back the text address
that it represents.

We can then use that in register_jprobe() to check that the entry point
we're passed is actually in the kernel text, rather than just some random
value.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 arch/ia64/kernel/kprobes.c    |    7 ++++++-
 arch/powerpc/kernel/kprobes.c |   11 ++++++++---
 kernel/kprobes.c              |    9 +++++++++
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 5bc46f1..5dc98b5 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -936,10 +936,15 @@ static void ia64_get_bsp_cfm(struct unw_frame_info *info, void *arg)
 	return;
 }
 
+unsigned long arch_deref_entry_point(void *entry)
+{
+	return ((struct fnptr *)entry)->ip;
+}
+
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
-	unsigned long addr = ((struct fnptr *)(jp->entry))->ip;
+	unsigned long addr = arch_deref_entry_point(jp->entry);
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 	struct param_bsp_cfm pa;
 	int bytes;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 0c96611..440f5a8 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -492,6 +492,13 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
 	return ret;
 }
 
+#ifdef CONFIG_PPC64
+unsigned long arch_deref_entry_point(void *entry)
+{
+	return (unsigned long)(((func_descr_t *)entry)->entry);
+}
+#endif
+
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
@@ -500,11 +507,9 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 	memcpy(&kcb->jprobe_saved_regs, regs, sizeof(struct pt_regs));
 
 	/* setup return addr to the jprobe handler routine */
+	regs->nip = arch_deref_entry_point(jp->entry);
 #ifdef CONFIG_PPC64
-	regs->nip = (unsigned long)(((func_descr_t *)jp->entry)->entry);
 	regs->gpr[2] = (unsigned long)(((func_descr_t *)jp->entry)->toc);
-#else
-	regs->nip = (unsigned long)jp->entry;
 #endif
 
 	return 1;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9e47d8c..3e9f513 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -675,9 +675,18 @@ static struct notifier_block kprobe_exceptions_nb = {
 	.priority = 0x7fffffff /* we need to be notified first */
 };
 
+unsigned long __weak arch_deref_entry_point(void *entry)
+{
+	return (unsigned long)entry;
+}
 
 int __kprobes register_jprobe(struct jprobe *jp)
 {
+	unsigned long addr = arch_deref_entry_point(jp->entry);
+
+	if (!kernel_text_address(addr))
+		return -EINVAL;
+
 	/* Todo: Verify probepoint is a function entry point */
 	jp->kp.pre_handler = setjmp_pre_handler;
 	jp->kp.break_handler = longjmp_break_handler;
-- 
1.5.1.3.g7a33b

Re: [PATCH 3/3] Make jprobes a little safer for users

[Andrew Morton]

On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman <michael@ellerman.id.au> wrote:

> I realise jprobes are a razor-blades-included type of interface, but
> that doesn't mean we can't try and make them safer to use. This guy I
> know once wrote code like this:
> 
> struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> 
> And then his kernel exploded. Oops.
> 
> This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
> which takes the void * in a struct jprobe, and gives back the text address
> that it represents.
> 
> We can then use that in register_jprobe() to check that the entry point
> we're passed is actually in the kernel text, rather than just some random
> value.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>  arch/ia64/kernel/kprobes.c    |    7 ++++++-
>  arch/powerpc/kernel/kprobes.c |   11 ++++++++---
>  kernel/kprobes.c              |    9 +++++++++

We're missing a declaration of arch_deref_entry_point() in some header file?

Re: [PATCH 3/3] Make jprobes a little safer for users

[Michael Ellerman]

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

On Mon, 2007-06-25 at 19:00 -0700, Andrew Morton wrote:
> On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman <michael@ellerman.id.au> wrote:
> 
> > I realise jprobes are a razor-blades-included type of interface, but
> > that doesn't mean we can't try and make them safer to use. This guy I
> > know once wrote code like this:
> > 
> > struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> > 
> > And then his kernel exploded. Oops.
> > 
> > This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
> > which takes the void * in a struct jprobe, and gives back the text address
> > that it represents.
> > 
> > We can then use that in register_jprobe() to check that the entry point
> > we're passed is actually in the kernel text, rather than just some random
> > value.
> > 
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > ---
> >  arch/ia64/kernel/kprobes.c    |    7 ++++++-
> >  arch/powerpc/kernel/kprobes.c |   11 ++++++++---
> >  kernel/kprobes.c              |    9 +++++++++
> 
> We're missing a declaration of arch_deref_entry_point() in some header file?

Yeah I guess. It's declared weak in kernel/kprobes.c, but there should
be a definition somewhere to make sure the three versions don't get out
of sync. I'll send a patch.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 1/1] Add a prototype for arch_deref_entry_point()

[Michael Ellerman]

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 include/linux/kprobes.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index bd89285..51464d1 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -214,6 +214,7 @@ int longjmp_break_handler(struct kprobe *, struct pt_regs *);
 int register_jprobe(struct jprobe *p);
 void unregister_jprobe(struct jprobe *p);
 void jprobe_return(void);
+unsigned long arch_deref_entry_point(void *);
 
 int register_kretprobe(struct kretprobe *rp);
 void unregister_kretprobe(struct kretprobe *rp);
-- 
1.5.1.3.g7a33b

Re: [PATCH 1/3] Make struct jprobe.entry a void *

[Ananth N Mavinakayanahalli]

On Tue, Jun 26, 2007 at 11:48:50AM +1000, Michael Ellerman wrote:
> Currently jprobe.entry is a kprobe_opcode_t *, but that's a lie. On some
> platforms it doesn't point to an opcode at all, it points to a function
> descriptor.
> 
> It's really a pointer to something that the arch code can turn into a
> function entry point. And that's what actually happens, none of the
> generic code ever looks at jprobe.entry, it's only ever dereferenced
> by arch code.
> 
> So just make it a void *.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

Tested on powerpc. Ack to all three patches plus Andrew's declaration
fixup.

Thanks Michael for the patches.

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

> ---
> 
> It isn't obvious where kprobes patches should go, is anyone "the" maintainer?
> Instead I've just sent this to everyone who'd touched the code lately, or
> might be otherwise interested.
> 
> 
>  include/linux/kprobes.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 23adf60..f4e53b7 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -116,7 +116,7 @@ struct kprobe {
>   */
>  struct jprobe {
>  	struct kprobe kp;
> -	kprobe_opcode_t *entry;	/* probe handling code to jump to */
> +	void *entry;	/* probe handling code to jump to */
>  };
> 
>  DECLARE_PER_CPU(struct kprobe *, current_kprobe);
> -- 
> 1.5.1.3.g7a33b

Re: [PATCH 1/3] Make struct jprobe.entry a void *

[Ananth N Mavinakayanahalli]

On Tue, Jun 26, 2007 at 11:48:50AM +1000, Michael Ellerman wrote:
> ---
> 
> It isn't obvious where kprobes patches should go, is anyone "the" maintainer?
> Instead I've just sent this to everyone who'd touched the code lately, or
> might be otherwise interested.

There isn't a single maintainer for the kprobes infrastructure as it
contains quite a bit of low level arch specific code. The working model
currently is that the patches are sent to lkml with a cc to the
maintainers listed, as you've rightly done.

Ananth

Re: [PATCH 1/3] Make struct jprobe.entry a void *

[Michael Ellerman]

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

On Tue, 2007-06-26 at 09:29 +0530, Ananth N Mavinakayanahalli wrote:
> On Tue, Jun 26, 2007 at 11:48:50AM +1000, Michael Ellerman wrote:
> > ---
> > 
> > It isn't obvious where kprobes patches should go, is anyone "the" maintainer?
> > Instead I've just sent this to everyone who'd touched the code lately, or
> > might be otherwise interested.
> 
> There isn't a single maintainer for the kprobes infrastructure as it
> contains quite a bit of low level arch specific code. The working model
> currently is that the patches are sent to lkml with a cc to the
> maintainers listed, as you've rightly done.

OK, no worries. I guess that's a bit messy to put into a MAINTAINERS entry.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] Remove JPROBE_ENTRY()

[Christoph Hellwig]

On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> AFAICT now that jprobe.entry is a void *, JPROBE_ENTRY doesn't do
> anything useful - so remove it ..
> 
> I've left a do-nothing version so that out-of-tree jprobes code will still
> compile without modifications.

Please kill the definition.  We don't want to keep unused crap around
just to let code compile.  And I have some plans for even deeper change
in this area, so they'll have to change anyway.

Re: [PATCH 3/3] Make jprobes a little safer for users

[Christoph Hellwig]

On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> I realise jprobes are a razor-blades-included type of interface, but
> that doesn't mean we can't try and make them safer to use. This guy I
> know once wrote code like this:
> 
> struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> 
> And then his kernel exploded. Oops.
> 
> This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
> which takes the void * in a struct jprobe, and gives back the text address
> that it represents.
> 
> We can then use that in register_jprobe() to check that the entry point
> we're passed is actually in the kernel text, rather than just some random
> value.

Please don't add more weak functions, they're utterly horrible for
anyone trying to understand the code.  Otherwise this looks fine to me.

Re: [PATCH 3/3] Make jprobes a little safer for users

[Michael Ellerman]

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

On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
> On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> > I realise jprobes are a razor-blades-included type of interface, but
> > that doesn't mean we can't try and make them safer to use. This guy I
> > know once wrote code like this:
> > 
> > struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> > 
> > And then his kernel exploded. Oops.
> > 
> > This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
> > which takes the void * in a struct jprobe, and gives back the text address
> > that it represents.
> > 
> > We can then use that in register_jprobe() to check that the entry point
> > we're passed is actually in the kernel text, rather than just some random
> > value.
> 
> Please don't add more weak functions, they're utterly horrible for
> anyone trying to understand the code.  Otherwise this looks fine to me.

What do you recommend instead? #define ARCH_HAS_FOO_BAR ?

I don't see what's utterly horrible about them. The fact that they're
weak is fairly reasonable documentation that they're overridden
somewhere else. And grep/cscope/ctags will find both the weak and
non-weak versions for you?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] Make jprobes a little safer for users

[Abhishek Sagar]

On 6/26/07, Michael Ellerman <michael@ellerman.id.au> wrote:

> We can then use that in register_jprobe() to check that the entry point
> we're passed is actually in the kernel text, rather than just some random
> value.

A similar cleanup is possible even for return probes then. I wonder if
there are any kprobe related scenarios where the executable code may
be located outside the core kernel text region (e.g, ITCM?). In that
case would it also be wrong to assume that the jprobe handler may be
situated outside the kernel core text / module  region? Would it then
make sense to move this check from register_jprobe() to the arch
dependent code?

>  int __kprobes register_jprobe(struct jprobe *jp)
>  {
> +       unsigned long addr = arch_deref_entry_point(jp->entry);
> +
> +       if (!kernel_text_address(addr))
> +               return -EINVAL;

Seems like you're checking for the jprobe handler to be within
kernel/module range. Why not narrow this down to just module range
(!module_text_address(addr), say)? Core kernel functions would not be
ending with a 'jprobe_return()' anyway.

--
Abhishek Sagar

-
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] Make jprobes a little safer for users

[Michael Ellerman]

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

On Tue, 2007-06-26 at 11:49 +0530, Abhishek Sagar wrote:
> On 6/26/07, Michael Ellerman <michael@ellerman.id.au> wrote:
> 
> > We can then use that in register_jprobe() to check that the entry point
> > we're passed is actually in the kernel text, rather than just some random
> > value.
> 
> A similar cleanup is possible even for return probes then. I wonder if
> there are any kprobe related scenarios where the executable code may
> be located outside the core kernel text region (e.g, ITCM?). In that
> case would it also be wrong to assume that the jprobe handler may be
> situated outside the kernel core text / module  region? Would it then
> make sense to move this check from register_jprobe() to the arch
> dependent code?

It did occur to me that someone might be doing something crazy like
branching to code that's not in the kernel/module text - but I was
hoping that wouldn't be the case. I'm not sure what ITCM is?


> >  int __kprobes register_jprobe(struct jprobe *jp)
> >  {
> > +       unsigned long addr = arch_deref_entry_point(jp->entry);
> > +
> > +       if (!kernel_text_address(addr))
> > +               return -EINVAL;
> 
> Seems like you're checking for the jprobe handler to be within
> kernel/module range. Why not narrow this down to just module range
> (!module_text_address(addr), say)? Core kernel functions would not be
> ending with a 'jprobe_return()' anyway.

There's jprobe code in net/ipv4/tcp_probe.c and net/dccp/probe.c that
can be builtin or modular, so I think kernel_text_address() is right.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] Make jprobes a little safer for users

[Andrew Morton]

On Tue, 26 Jun 2007 16:03:58 +1000 Michael Ellerman <michael@ellerman.id.au> wrote:

> On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
> > On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
> > > I realise jprobes are a razor-blades-included type of interface, but
> > > that doesn't mean we can't try and make them safer to use. This guy I
> > > know once wrote code like this:
> > > 
> > > struct jprobe jp = { .kp.symbol_name = "foo", .entry = "jprobe_foo" };
> > > 
> > > And then his kernel exploded. Oops.
> > > 
> > > This patch adds an arch hook, arch_deref_entry_point() (I don't like it either)
> > > which takes the void * in a struct jprobe, and gives back the text address
> > > that it represents.
> > > 
> > > We can then use that in register_jprobe() to check that the entry point
> > > we're passed is actually in the kernel text, rather than just some random
> > > value.
> > 
> > Please don't add more weak functions, they're utterly horrible for
> > anyone trying to understand the code.  Otherwise this looks fine to me.
> 
> What do you recommend instead? #define ARCH_HAS_FOO_BAR ?

o lord, save us, no.

> I don't see what's utterly horrible about them.

Me either.

> The fact that they're
> weak is fairly reasonable documentation that they're overridden
> somewhere else. And grep/cscope/ctags will find both the weak and
> non-weak versions for you?

yup.

In this case we could just require that each jprobes-supporting
architecture implement arch_deref_entry_point().


Or one could do the Linus trick.  In each architecture which implements
arch_deref_entry_point() do:

#define arch_deref_entry_point arch_deref_entry_point

in the per-arch header file then, in non-arch code, do

#ifndef arch_deref_entry_point
static unsigned long arch_deref_entry_point(...)
{
	<generic implementation>
}
#endif

That's just the ARCH_HAS_FOO_BAR thing, only less fugly.

Re: [PATCH 3/3] Make jprobes a little safer for users

[Abhishek Sagar]

On 6/26/07, Michael Ellerman <michael@ellerman.id.au> wrote:
> It did occur to me that someone might be doing something crazy like
> branching to code that's not in the kernel/module text - but I was
> hoping that wouldn't be the case. I'm not sure what ITCM is?

The reference to tightly coupled memory (ITCM) was just to have you
consider the possibility of the jprobe handler being outside kernel
text region. Totally paranoid really.

> > >  int __kprobes register_jprobe(struct jprobe *jp)
> > >  {
> > > +       unsigned long addr = arch_deref_entry_point(jp->entry);
> > > +
> > > +       if (!kernel_text_address(addr))
> > > +               return -EINVAL;
> >
> > Seems like you're checking for the jprobe handler to be within
> > kernel/module range. Why not narrow this down to just module range
> > (!module_text_address(addr), say)? Core kernel functions would not be
> > ending with a 'jprobe_return()' anyway.
>
> There's jprobe code in net/ipv4/tcp_probe.c and net/dccp/probe.c that
> can be builtin or modular, so I think kernel_text_address() is right.

Ok..thanks for that clarification.

--
Abhishek Sagar

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/