From: Michael Ellerman <hidden> Date: 2007-06-26 01:49:15
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 <redacted>
---
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(-)
@@ -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)
@@ -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(structkprobe*,current_kprobe);DECLARE_PER_CPU(structkprobe_ctlblk,kprobe_ctlblk);
From: Michael Ellerman <hidden> Date: 2007-06-26 01:49:45
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 <redacted>
---
arch/ia64/kernel/kprobes.c | 7 ++++++-
arch/powerpc/kernel/kprobes.c | 11 ++++++++---
kernel/kprobes.c | 9 +++++++++
3 files changed, 23 insertions(+), 4 deletions(-)
@@ -675,9 +675,18 @@ static struct notifier_block kprobe_exceptions_nb = {.priority=0x7fffffff/* we need to be notified first */};+unsignedlong__weakarch_deref_entry_point(void*entry)+{+return(unsignedlong)entry;+}int__kprobesregister_jprobe(structjprobe*jp){+unsignedlongaddr=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;
From: Andrew Morton <akpm@linux-foundation.org> Date: 2007-06-26 02:02:16
On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman [off-list ref] 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 <redacted>
---
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?
From: Michael Ellerman <hidden> Date: 2007-06-26 02:06:59
On Mon, 2007-06-25 at 19:00 -0700, Andrew Morton wrote:
On Tue, 26 Jun 2007 11:48:51 +1000 (EST) Michael Ellerman [off-list ref] wrote:
quoted
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 <redacted>
---
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
From: Ananth N Mavinakayanahalli <hidden> Date: 2007-06-26 03:48:19
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 <redacted>
Tested on powerpc. Ack to all three patches plus Andrew's declaration
fixup.
Thanks Michael for the patches.
Acked-by: Ananth N Mavinakayanahalli <redacted>
quoted hunk
---
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(-)
From: Ananth N Mavinakayanahalli <hidden> Date: 2007-06-26 03:56:00
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
From: Michael Ellerman <hidden> Date: 2007-06-26 04:36:15
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:
quoted
---
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
From: Christoph Hellwig <hch@lst.de> Date: 2007-06-26 05:53:24
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.
From: Christoph Hellwig <hch@lst.de> Date: 2007-06-26 05:54:47
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.
From: Michael Ellerman <hidden> Date: 2007-06-26 06:04:18
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:
quoted
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
On 6/26/07, Michael Ellerman [off-list ref] 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
-
From: Michael Ellerman <hidden> Date: 2007-06-26 06:34:58
On Tue, 2007-06-26 at 11:49 +0530, Abhishek Sagar wrote:
On 6/26/07, Michael Ellerman [off-list ref] wrote:
quoted
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?
quoted
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
From: Andrew Morton <akpm@linux-foundation.org> Date: 2007-06-26 06:51:55
On Tue, 26 Jun 2007 16:03:58 +1000 Michael Ellerman [off-list ref] wrote:
On Tue, 2007-06-26 at 07:53 +0200, Christoph Hellwig wrote:
quoted
On Tue, Jun 26, 2007 at 11:48:51AM +1000, Michael Ellerman wrote:
quoted
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.
On 6/26/07, Michael Ellerman [off-list ref] 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.
quoted
quoted
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