[PATCH] jprobes: Ensure that the probepoint is at function entry

[PATCH] jprobes: Ensure that the probepoint is at function entry

[Naveen N. Rao]

Similar to commit 90ec5e89e393c ("kretprobes: Ensure probe location is
at function entry"), ensure that the jprobe probepoint is at function
entry.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/kprobes.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index adfe3b4cfe05..950018609339 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1776,9 +1776,14 @@ int register_jprobes(struct jprobe **jps, int num)
 		jp = jps[i];
 		addr = arch_deref_entry_point(jp->entry);
 
-		/* Verify probepoint is a function entry point */
+		/*
+		 * Verify probepoint as well as the jprobe handler are
+		 * function entry points.
+		 */
 		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
-		    offset == 0) {
+				offset == 0 &&
+				function_offset_within_entry(jp->kp.addr,
+					jp->kp.symbol_name, jp->kp.offset)) {
 			jp->kp.pre_handler = setjmp_pre_handler;
 			jp->kp.break_handler = longjmp_break_handler;
 			ret = register_kprobe(&jp->kp);
-- 
2.13.1

Re: [PATCH] jprobes: Ensure that the probepoint is at function entry

[Ingo Molnar]

* Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Similar to commit 90ec5e89e393c ("kretprobes: Ensure probe location is
> at function entry"), ensure that the jprobe probepoint is at function
> entry.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/kprobes.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index adfe3b4cfe05..950018609339 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1776,9 +1776,14 @@ int register_jprobes(struct jprobe **jps, int num)
>  		jp = jps[i];
>  		addr = arch_deref_entry_point(jp->entry);
>  
> -		/* Verify probepoint is a function entry point */
> +		/*
> +		 * Verify probepoint as well as the jprobe handler are
> +		 * function entry points.
> +		 */
>  		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
> -		    offset == 0) {
> +				offset == 0 &&
> +				function_offset_within_entry(jp->kp.addr,
> +					jp->kp.symbol_name, jp->kp.offset)) {
>  			jp->kp.pre_handler = setjmp_pre_handler;
>  			jp->kp.break_handler = longjmp_break_handler;
>  			ret = register_kprobe(&jp->kp);

Yeah, so I agree with the fix, but the line breaks there are disgusting.

One solution would be to split out the iterator into a register_jprobe() function. 
Also, introduce a 'kp' temporary variable to simplify and shorten usage.

Also, 'function_offset_within_entry' is way too long a name, and it's also a 
minomer I think. The purpose of this function is to enforce that the relative 
'offset' of a new probe is at the standard function entry offset: i.e. 0 on most 
architectures, and some ABI dependent constant on PowerPC, right?

That's not at all clear from that name, plus it's a global namespace symbol, yet 
has no 'kprobes' prefix. So it should be named something like 
'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart.

All of these cleanups should be in separate patches - with the fix in the end.

Thanks,

	Ingo

Re: [PATCH] jprobes: Ensure that the probepoint is at function entry

[Masami Hiramatsu]

On Wed, 5 Jul 2017 12:42:16 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > Similar to commit 90ec5e89e393c ("kretprobes: Ensure probe location is
> > at function entry"), ensure that the jprobe probepoint is at function
> > entry.

Sorry I missed it.

> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  kernel/kprobes.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index adfe3b4cfe05..950018609339 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1776,9 +1776,14 @@ int register_jprobes(struct jprobe **jps, int num)
> >  		jp = jps[i];
> >  		addr = arch_deref_entry_point(jp->entry);
> >  
> > -		/* Verify probepoint is a function entry point */
> > +		/*
> > +		 * Verify probepoint as well as the jprobe handler are
> > +		 * function entry points.
> > +		 */
> >  		if (kallsyms_lookup_size_offset(addr, NULL, &offset) &&
> > -		    offset == 0) {
> > +				offset == 0 &&
> > +				function_offset_within_entry(jp->kp.addr,
> > +					jp->kp.symbol_name, jp->kp.offset)) {

Here, you are agressively use tab, please align the indent to 
same level of kallsyms_lookup_size_offset?

> >  			jp->kp.pre_handler = setjmp_pre_handler;
> >  			jp->kp.break_handler = longjmp_break_handler;
> >  			ret = register_kprobe(&jp->kp);
> 
> Yeah, so I agree with the fix, but the line breaks there are disgusting.
> 
> One solution would be to split out the iterator into a register_jprobe() function. 
> Also, introduce a 'kp' temporary variable to simplify and shorten usage.
> 

Agreed.

> Also, 'function_offset_within_entry' is way too long a name, and it's also a 
> minomer I think. The purpose of this function is to enforce that the relative 
> 'offset' of a new probe is at the standard function entry offset: i.e. 0 on most 
> architectures, and some ABI dependent constant on PowerPC, right?
> 
> That's not at all clear from that name, plus it's a global namespace symbol, yet 
> has no 'kprobes' prefix. So it should be named something like 
> 'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart.

Hmm, I would rather like kprobe_within_entry(), since offset != 0 is
actually valid for normal kprobe, that is kretprobe and jprobe limitation.

Thank you,

> 
> All of these cleanups should be in separate patches - with the fix in the end.
> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] jprobes: Ensure that the probepoint is at function entry

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > Also, 'function_offset_within_entry' is way too long a name, and it's also a 
> > minomer I think. The purpose of this function is to enforce that the relative 
> > 'offset' of a new probe is at the standard function entry offset: i.e. 0 on most 
> > architectures, and some ABI dependent constant on PowerPC, right?
> > 
> > That's not at all clear from that name, plus it's a global namespace symbol, yet 
> > has no 'kprobes' prefix. So it should be named something like 
> > 'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart.
> 
> Hmm, I would rather like kprobe_within_entry(), since offset != 0 is
> actually valid for normal kprobe, that is kretprobe and jprobe limitation.

But what entry? That it's within a range or that offset is always 0 is really an 
implementational detail: depending on what type of kprobe it is, it is either 
validly within the confines of the specified function symbol or not.

What _really_ matters to callers is whether it's a valid kprobe to be inserted 
into that function, right?

I.e. the long name came from over-specifying what is done by the function - while 
simplifying makes it actually more meaningful to read.

Thanks,

	Ingo

Re: [PATCH] jprobes: Ensure that the probepoint is at function entry

[Masami Hiramatsu]

On Thu, 6 Jul 2017 14:15:49 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > Also, 'function_offset_within_entry' is way too long a name, and it's also a 
> > > minomer I think. The purpose of this function is to enforce that the relative 
> > > 'offset' of a new probe is at the standard function entry offset: i.e. 0 on most 
> > > architectures, and some ABI dependent constant on PowerPC, right?
> > > 
> > > That's not at all clear from that name, plus it's a global namespace symbol, yet 
> > > has no 'kprobes' prefix. So it should be named something like 
> > > 'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart.
> > 
> > Hmm, I would rather like kprobe_within_entry(), since offset != 0 is
> > actually valid for normal kprobe, that is kretprobe and jprobe limitation.
> 
> But what entry? That it's within a range or that offset is always 0 is really an 
> implementational detail: depending on what type of kprobe it is, it is either 
> validly within the confines of the specified function symbol or not.

Hmm, right. In most cases, it just checks the address (symbol+offset) is
on the function entry.

> What _really_ matters to callers is whether it's a valid kprobe to be inserted 
> into that function, right?

No, for that purpose, kprobes checks it in other places (kprobe_addr() and check_kprobe_address_safe()). This function is an additional safety check
only for kretprobe and jprobe which must be placed on the function entry.
(kprobe can probe function body but kretprobe and jprobes are not)
 
> I.e. the long name came from over-specifying what is done by the function - while 
> simplifying makes it actually more meaningful to read.

I see, but kprobe_offset_valid is too simple. How about kprobe_on_func_entry()?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] jprobes: Ensure that the probepoint is at function entry

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 6 Jul 2017 14:15:49 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > > Also, 'function_offset_within_entry' is way too long a name, and it's also a 
> > > > minomer I think. The purpose of this function is to enforce that the relative 
> > > > 'offset' of a new probe is at the standard function entry offset: i.e. 0 on most 
> > > > architectures, and some ABI dependent constant on PowerPC, right?
> > > > 
> > > > That's not at all clear from that name, plus it's a global namespace symbol, yet 
> > > > has no 'kprobes' prefix. So it should be named something like 
> > > > 'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart.
> > > 
> > > Hmm, I would rather like kprobe_within_entry(), since offset != 0 is
> > > actually valid for normal kprobe, that is kretprobe and jprobe limitation.
> > 
> > But what entry? That it's within a range or that offset is always 0 is really an 
> > implementational detail: depending on what type of kprobe it is, it is either 
> > validly within the confines of the specified function symbol or not.
> 
> Hmm, right. In most cases, it just checks the address (symbol+offset) is
> on the function entry.
> 
> > What _really_ matters to callers is whether it's a valid kprobe to be inserted 
> > into that function, right?
> 
> No, for that purpose, kprobes checks it in other places (kprobe_addr() and check_kprobe_address_safe()). This function is an additional safety check
> only for kretprobe and jprobe which must be placed on the function entry.
> (kprobe can probe function body but kretprobe and jprobes are not)
>  
> > I.e. the long name came from over-specifying what is done by the function - while 
> > simplifying makes it actually more meaningful to read.
> 
> I see, but kprobe_offset_valid is too simple. How about kprobe_on_func_entry()?

Ok, kprobe_on_func_entry() works for me.

Thanks,

	Ingo

Re: [PATCH] jprobes: Ensure that the probepoint is at function entry

[Naveen N. Rao]

On 2017/07/07 12:49PM, Ingo Molnar wrote:
> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu, 6 Jul 2017 14:15:49 +0200
> > Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > > * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > 
> > > > > Also, 'function_offset_within_entry' is way too long a name, and it's also a 
> > > > > minomer I think. The purpose of this function is to enforce that the relative 
> > > > > 'offset' of a new probe is at the standard function entry offset: i.e. 0 on most 
> > > > > architectures, and some ABI dependent constant on PowerPC, right?
> > > > > 
> > > > > That's not at all clear from that name, plus it's a global namespace symbol, yet 
> > > > > has no 'kprobes' prefix. So it should be named something like 
> > > > > 'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart.
> > > > 
> > > > Hmm, I would rather like kprobe_within_entry(), since offset != 0 is
> > > > actually valid for normal kprobe, that is kretprobe and jprobe limitation.
> > > 
> > > But what entry? That it's within a range or that offset is always 0 is really an 
> > > implementational detail: depending on what type of kprobe it is, it is either 
> > > validly within the confines of the specified function symbol or not.
> > 
> > Hmm, right. In most cases, it just checks the address (symbol+offset) is
> > on the function entry.
> > 
> > > What _really_ matters to callers is whether it's a valid kprobe to be inserted 
> > > into that function, right?
> > 
> > No, for that purpose, kprobes checks it in other places (kprobe_addr() and check_kprobe_address_safe()). This function is an additional safety check
> > only for kretprobe and jprobe which must be placed on the function entry.
> > (kprobe can probe function body but kretprobe and jprobes are not)
> >  
> > > I.e. the long name came from over-specifying what is done by the function - while 
> > > simplifying makes it actually more meaningful to read.
> > 
> > I see, but kprobe_offset_valid is too simple. How about kprobe_on_func_entry()?
> 
> Ok, kprobe_on_func_entry() works for me.

Ingo, Masami,
Thanks for the review/feedback. I will accommodate these changes and 
post a revised version.

- Naveen