Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681460 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] jprobes: Ensure that the probepoint is at function entry Ingo Molnar <mingo@kernel.org> - 2017-07-05 12:50 +0200
    Re: [PATCH] jprobes: Ensure that the probepoint is at function  entry Masami Hiramatsu <mhiramat@kernel.org> - 2017-07-06 12:10 +0200
      Re: [PATCH] jprobes: Ensure that the probepoint is at function entry Ingo Molnar <mingo@kernel.org> - 2017-07-06 14:20 +0200
        Re: [PATCH] jprobes: Ensure that the probepoint is at function  entry Masami Hiramatsu <mhiramat@kernel.org> - 2017-07-07 03:10 +0200
          Re: [PATCH] jprobes: Ensure that the probepoint is at function entry Ingo Molnar <mingo@kernel.org> - 2017-07-07 12:50 +0200
            Re: [PATCH] jprobes: Ensure that the probepoint is at function entry "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-07-07 13:30 +0200

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

* 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

[toc] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

#1682379

* 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

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

#1683116

* 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

[toc] | [prev] | [next] | [standalone]

#1683140

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web