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

Groups > linux.kernel > #1580390 > unrolled thread

[PATCH] kretprobes: reject registration if a symbol offset is specified

Back to article view | Back to linux.kernel

Contents

  [PATCH] kretprobes: reject registration if a symbol offset is specified "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-14 09:40 +0100
    Re: [PATCH] kretprobes: reject registration if a symbol offset is  specified Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com> - 2017-02-14 09:50 +0100
    Re: [PATCH] kretprobes: reject registration if a symbol offset is  specified Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-14 11:40 +0100
      Re: [PATCH] kretprobes: reject registration if a symbol offset is  specified "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-15 19:00 +0100
        [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-15 19:20 +0100
          Re: [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-16 00:50 +0100
        [PATCH 1/3] kretprobes: ensure probe location is at function entry "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-15 19:20 +0100
          [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-15 19:20 +0100
            Re: [PATCH 2/3] trace/kprobes: allow return probes with offsets and  absolute addresses Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-16 00:50 +0100
          Re: [PATCH 1/3] kretprobes: ensure probe location is at function  entry Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-16 00:50 +0100
            Re: [PATCH 1/3] kretprobes: ensure probe location is at function  entry "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-16 09:00 +0100
          [PATCH 0/2] powerpc: kretprobe updates "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-16 09:20 +0100
            [PATCH 1/2] powerpc: kretprobes: override default function entry offset "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-16 09:20 +0100
            [PATCH 2/2] perf: powerpc: choose LEP with kretprobes "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-16 09:20 +0100
            Re: [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-17 11:50 +0100
              Re: [PATCH 0/2] powerpc: kretprobe updates Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-17 21:50 +0100
                Re: [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-19 05:50 +0100
                  Re: [PATCH 0/2] powerpc: kretprobe updates "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-20 11:00 +0100
                    Re: [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-21 14:10 +0100
                      Re: [PATCH 0/2] powerpc: kretprobe updates "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-22 14:50 +0100
                Re: [PATCH 0/2] powerpc: kretprobe updates "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-20 10:50 +0100
                Re: [PATCH 0/2] powerpc: kretprobe updates "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-02-20 12:50 +0100
                  Re: [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-21 14:10 +0100
        Re: [PATCH] kretprobes: reject registration if a symbol offset is  specified Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-16 00:30 +0100

Page 1 of 2  [1] 2  Next page →

#1580390 — [PATCH] kretprobes: reject registration if a symbol offset is specified

Users shouldn't be able to specify an offset with kretprobes, as we always
want to probe at function entry. Otherwise, we won't be able to capture
the proper return address resulting in the kretprobe never firing.

With samples/kprobes/kretprobe_example.c including an offset:
	my_kretprobe.kp.offset = 40;

Before this patch, the probe gets planted but never fires.

After this patch:
	$ sudo insmod samples/kprobes/kretprobe_example.ko
	[sudo] password for naveen: 
	insmod: ERROR: could not insert module samples/kprobes/kretprobe_example.ko: Operation not permitted

And dmesg:
	[48253.757629] register_kretprobe failed, returned -22

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

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 60a702a05684..83ad7e440417 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1847,6 +1847,9 @@ int register_kretprobe(struct kretprobe *rp)
 	int i;
 	void *addr;
 
+	if (rp->kp.offset)
+		return -EINVAL;
+
 	if (kretprobe_blacklist_size) {
 		addr = kprobe_addr(&rp->kp);
 		if (IS_ERR(addr))
-- 
2.11.0

[toc] | [next] | [standalone]

#1580402 — Re: [PATCH] kretprobes: reject registration if a symbol offset is specified

On Tue, Feb 14, 2017 at 02:01:18PM +0530, Naveen N. Rao wrote:
> Users shouldn't be able to specify an offset with kretprobes, as we always
> want to probe at function entry. Otherwise, we won't be able to capture
> the proper return address resulting in the kretprobe never firing.
> 
> With samples/kprobes/kretprobe_example.c including an offset:
> 	my_kretprobe.kp.offset = 40;
> 
> Before this patch, the probe gets planted but never fires.
> 
> After this patch:
> 	$ sudo insmod samples/kprobes/kretprobe_example.ko
> 	[sudo] password for naveen: 
> 	insmod: ERROR: could not insert module samples/kprobes/kretprobe_example.ko: Operation not permitted
> 
> And dmesg:
> 	[48253.757629] register_kretprobe failed, returned -22
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

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

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

#1580465 — Re: [PATCH] kretprobes: reject registration if a symbol offset is specified

On Tue, 14 Feb 2017 14:01:18 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Users shouldn't be able to specify an offset with kretprobes, as we always
> want to probe at function entry. Otherwise, we won't be able to capture
> the proper return address resulting in the kretprobe never firing.
> 

Nack, this should be checked by using kallsyms, since the
many non-exported kernel functions have same name.
Actually perf-probe is trying to put any probes(including return
probe) by using relative address from text-start symbol (_stext
or _text). In this case, kretprobe also can be set by _text+OFFSET.

So please rewrite this by using kallsyms_lookup_size_offset()
which tells you the address is actually on the beginning of
function or not.

Thank you,

> With samples/kprobes/kretprobe_example.c including an offset:
> 	my_kretprobe.kp.offset = 40;
> 
> Before this patch, the probe gets planted but never fires.
> 
> After this patch:
> 	$ sudo insmod samples/kprobes/kretprobe_example.ko
> 	[sudo] password for naveen: 
> 	insmod: ERROR: could not insert module samples/kprobes/kretprobe_example.ko: Operation not permitted
> 
> And dmesg:
> 	[48253.757629] register_kretprobe failed, returned -22
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/kprobes.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 60a702a05684..83ad7e440417 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1847,6 +1847,9 @@ int register_kretprobe(struct kretprobe *rp)
>  	int i;
>  	void *addr;
>  
> +	if (rp->kp.offset)
> +		return -EINVAL;
> +
>  	if (kretprobe_blacklist_size) {
>  		addr = kprobe_addr(&rp->kp);
>  		if (IS_ERR(addr))
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1581520 — Re: [PATCH] kretprobes: reject registration if a symbol offset is specified

Hi Masami,

On 2017/02/14 07:32PM, Masami Hiramatsu wrote:
> On Tue, 14 Feb 2017 14:01:18 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > Users shouldn't be able to specify an offset with kretprobes, as we always
> > want to probe at function entry. Otherwise, we won't be able to capture
> > the proper return address resulting in the kretprobe never firing.
> > 
> 
> Nack, this should be checked by using kallsyms, since the
> many non-exported kernel functions have same name.
> Actually perf-probe is trying to put any probes(including return
> probe) by using relative address from text-start symbol (_stext
> or _text). In this case, kretprobe also can be set by _text+OFFSET.

Interesting. In my tests, 'perf probe' always chose the function name 
for return probes on both x86 and powerpc. So, looking into it further, 
I found commit 25dd9171f51c ("perf probe: Fix probing kretprobes") which 
changed perf probe behavior due to how kprobe_events behaved. And 
kprobe_events has always dis-allowed use of offset with kprobe_events.

But, I agree -- we should allow use of offset with kretprobes.  
Otherwise, we won't be able to probe functions that have the same name.

> 
> So please rewrite this by using kallsyms_lookup_size_offset()
> which tells you the address is actually on the beginning of
> function or not.

Sure. This makes use of offsets and/or absolute addresses with 
kretprobes safe. Patches on the way...

Thanks!
- Naveen

> 
> Thank you,
> 
> > With samples/kprobes/kretprobe_example.c including an offset:
> > 	my_kretprobe.kp.offset = 40;
> > 
> > Before this patch, the probe gets planted but never fires.
> > 
> > After this patch:
> > 	$ sudo insmod samples/kprobes/kretprobe_example.ko
> > 	[sudo] password for naveen: 
> > 	insmod: ERROR: could not insert module samples/kprobes/kretprobe_example.ko: Operation not permitted
> > 
> > And dmesg:
> > 	[48253.757629] register_kretprobe failed, returned -22
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  kernel/kprobes.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 60a702a05684..83ad7e440417 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1847,6 +1847,9 @@ int register_kretprobe(struct kretprobe *rp)
> >  	int i;
> >  	void *addr;
> >  
> > +	if (rp->kp.offset)
> > +		return -EINVAL;
> > +
> >  	if (kretprobe_blacklist_size) {
> >  		addr = kprobe_addr(&rp->kp);
> >  		if (IS_ERR(addr))
> > -- 
> > 2.11.0
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 

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

#1581541 — [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes"

This reverts commit 25dd9171f51c ("perf probe: Fix probing kretprobes").
kprobe_events now accepts offsets for kretprobes.

perf needs to be able to place return probes on static functions that
have the same name. Using the function name doesn't allow us to do that.
Instead, we should use the same scheme we use for kprobes: offset'ing
from _text/_stext.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 6a6f44dd594b..fa7f81af11e8 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -757,7 +757,7 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 	}
 
 	for (i = 0; i < ntevs; i++) {
-		if (!tevs[i].point.address || tevs[i].point.retprobe)
+		if (!tevs[i].point.address)
 			continue;
 		/* If we found a wrong one, mark it by NULL symbol */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
@@ -2841,7 +2841,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	}
 
 	/* Note that the symbols in the kmodule are not relocated */
-	if (!pev->uprobes && !pp->retprobe && !pev->target) {
+	if (!pev->uprobes && !pev->target) {
 		reloc_sym = kernel_get_ref_reloc_sym();
 		if (!reloc_sym) {
 			pr_warning("Relocated base symbol is not found!\n");
-- 
2.11.0

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

#1582046 — Re: [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes"

On Wed, 15 Feb 2017 23:47:54 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> This reverts commit 25dd9171f51c ("perf probe: Fix probing kretprobes").
> kprobe_events now accepts offsets for kretprobes.
> 
> perf needs to be able to place return probes on static functions that
> have the same name. Using the function name doesn't allow us to do that.
> Instead, we should use the same scheme we use for kprobes: offset'ing
> from _text/_stext.

So now we can use retprobe on static functions safely :)

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!


> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6a6f44dd594b..fa7f81af11e8 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -757,7 +757,7 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  	}
>  
>  	for (i = 0; i < ntevs; i++) {
> -		if (!tevs[i].point.address || tevs[i].point.retprobe)
> +		if (!tevs[i].point.address)
>  			continue;
>  		/* If we found a wrong one, mark it by NULL symbol */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
> @@ -2841,7 +2841,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	}
>  
>  	/* Note that the symbols in the kmodule are not relocated */
> -	if (!pev->uprobes && !pp->retprobe && !pev->target) {
> +	if (!pev->uprobes && !pev->target) {
>  		reloc_sym = kernel_get_ref_reloc_sym();
>  		if (!reloc_sym) {
>  			pr_warning("Relocated base symbol is not found!\n");
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1581544 — [PATCH 1/3] kretprobes: ensure probe location is at function entry

kretprobes can be registered by specifying an absolute address or by
specifying offset to a symbol. However, we need to ensure this falls at
function entry so as to be able to determine the return address.

Validate the same during kretprobe registration. By default, there
should not be any offset from a function entry, as determined through a
kallsyms_lookup(). Introduce arch_function_offset_within_entry() as a
way for architectures to override this.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
powerpc64 ABIv2 will need to use the over-ride as we want to use the
local entry point which will be at an offset of 8 bytes from the
(global) entry point. I have a patch that I will post separately.

Thanks,
Naveen

 include/linux/kprobes.h |  1 +
 kernel/kprobes.c        | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 8f6849084248..0c2489435117 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -266,6 +266,7 @@ extern int arch_init_kprobes(void);
 extern void show_registers(struct pt_regs *regs);
 extern void kprobes_inc_nmissed_count(struct kprobe *p);
 extern bool arch_within_kprobe_blacklist(unsigned long addr);
+extern bool arch_function_offset_within_entry(unsigned long offset);
 
 extern bool within_kprobe_blacklist(unsigned long addr);
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 43460104f119..72ecbf5a6312 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1834,12 +1834,25 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(pre_handler_kretprobe);
 
+bool __weak arch_function_offset_within_entry(unsigned long offset)
+{
+	return !offset;
+}
+
 int register_kretprobe(struct kretprobe *rp)
 {
 	int ret = 0;
 	struct kretprobe_instance *inst;
 	int i;
 	void *addr;
+	unsigned long offset;
+
+	addr = kprobe_addr(&rp->kp);
+	if (!kallsyms_lookup_size_offset((unsigned long)addr, NULL, &offset))
+		return -EINVAL;
+
+	if (!arch_function_offset_within_entry(offset))
+		return -EINVAL;
 
 	if (kretprobe_blacklist_size) {
 		addr = kprobe_addr(&rp->kp);
-- 
2.11.0

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

#1581550 — [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses

Since the kernel includes many non-global functions with same names, we
will need to use offsets from other symbols (typically _text/_stext) or
absolute addresses to place return probes on specific functions. Also,
the core register_kretprobe() API never forbid use of offsets or
absolute addresses with kretprobes.

Allow its use with the trace infrastructure.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/trace/trace_kprobe.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 7ad9e53ad174..2768cb60ebd7 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -679,10 +679,6 @@ static int create_trace_kprobe(int argc, char **argv)
 		return -EINVAL;
 	}
 	if (isdigit(argv[1][0])) {
-		if (is_return) {
-			pr_info("Return probe point must be a symbol.\n");
-			return -EINVAL;
-		}
 		/* an address specified */
 		ret = kstrtoul(&argv[1][0], 0, (unsigned long *)&addr);
 		if (ret) {
@@ -698,10 +694,6 @@ static int create_trace_kprobe(int argc, char **argv)
 			pr_info("Failed to parse symbol.\n");
 			return ret;
 		}
-		if (offset && is_return) {
-			pr_info("Return probe must be used without offset.\n");
-			return -EINVAL;
-		}
 	}
 	argc -= 2; argv += 2;
 
-- 
2.11.0

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

#1582027 — Re: [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses

On Wed, 15 Feb 2017 23:47:53 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Since the kernel includes many non-global functions with same names, we
> will need to use offsets from other symbols (typically _text/_stext) or
> absolute addresses to place return probes on specific functions. Also,
> the core register_kretprobe() API never forbid use of offsets or
> absolute addresses with kretprobes.
> 
> Allow its use with the trace infrastructure.
> 

OK, Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!


> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_kprobe.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 7ad9e53ad174..2768cb60ebd7 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -679,10 +679,6 @@ static int create_trace_kprobe(int argc, char **argv)
>  		return -EINVAL;
>  	}
>  	if (isdigit(argv[1][0])) {
> -		if (is_return) {
> -			pr_info("Return probe point must be a symbol.\n");
> -			return -EINVAL;
> -		}
>  		/* an address specified */
>  		ret = kstrtoul(&argv[1][0], 0, (unsigned long *)&addr);
>  		if (ret) {
> @@ -698,10 +694,6 @@ static int create_trace_kprobe(int argc, char **argv)
>  			pr_info("Failed to parse symbol.\n");
>  			return ret;
>  		}
> -		if (offset && is_return) {
> -			pr_info("Return probe must be used without offset.\n");
> -			return -EINVAL;
> -		}
>  	}
>  	argc -= 2; argv += 2;
>  
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1582014 — Re: [PATCH 1/3] kretprobes: ensure probe location is at function entry

On Wed, 15 Feb 2017 23:47:52 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> kretprobes can be registered by specifying an absolute address or by
> specifying offset to a symbol. However, we need to ensure this falls at
> function entry so as to be able to determine the return address.
> 
> Validate the same during kretprobe registration. By default, there
> should not be any offset from a function entry, as determined through a
> kallsyms_lookup(). Introduce arch_function_offset_within_entry() as a
> way for architectures to override this.
> 

Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> powerpc64 ABIv2 will need to use the over-ride as we want to use the
> local entry point which will be at an offset of 8 bytes from the
> (global) entry point. I have a patch that I will post separately.
> 
> Thanks,
> Naveen
> 
>  include/linux/kprobes.h |  1 +
>  kernel/kprobes.c        | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 8f6849084248..0c2489435117 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -266,6 +266,7 @@ extern int arch_init_kprobes(void);
>  extern void show_registers(struct pt_regs *regs);
>  extern void kprobes_inc_nmissed_count(struct kprobe *p);
>  extern bool arch_within_kprobe_blacklist(unsigned long addr);
> +extern bool arch_function_offset_within_entry(unsigned long offset);
>  
>  extern bool within_kprobe_blacklist(unsigned long addr);
>  
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 43460104f119..72ecbf5a6312 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1834,12 +1834,25 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(pre_handler_kretprobe);
>  
> +bool __weak arch_function_offset_within_entry(unsigned long offset)
> +{
> +	return !offset;
> +}
> +
>  int register_kretprobe(struct kretprobe *rp)
>  {
>  	int ret = 0;
>  	struct kretprobe_instance *inst;
>  	int i;
>  	void *addr;
> +	unsigned long offset;
> +
> +	addr = kprobe_addr(&rp->kp);
> +	if (!kallsyms_lookup_size_offset((unsigned long)addr, NULL, &offset))
> +		return -EINVAL;
> +
> +	if (!arch_function_offset_within_entry(offset))
> +		return -EINVAL;
>  
>  	if (kretprobe_blacklist_size) {
>  		addr = kprobe_addr(&rp->kp);
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1582371 — Re: [PATCH 1/3] kretprobes: ensure probe location is at function entry

On 2017/02/16 08:39AM, Masami Hiramatsu wrote:
> On Wed, 15 Feb 2017 23:47:52 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > kretprobes can be registered by specifying an absolute address or by
> > specifying offset to a symbol. However, we need to ensure this falls at
> > function entry so as to be able to determine the return address.
> > 
> > Validate the same during kretprobe registration. By default, there
> > should not be any offset from a function entry, as determined through a
> > kallsyms_lookup(). Introduce arch_function_offset_within_entry() as a
> > way for architectures to override this.
> > 
> 
> Looks good to me.
> 
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks, Masami! I am cc'ing linux-arch in case any other architectures 
need to override the arch-specific helper with a custom offset.

- Naveen

> 
> Thanks!
> 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> > powerpc64 ABIv2 will need to use the over-ride as we want to use the
> > local entry point which will be at an offset of 8 bytes from the
> > (global) entry point. I have a patch that I will post separately.
> > 
> > Thanks,
> > Naveen
> > 
> >  include/linux/kprobes.h |  1 +
> >  kernel/kprobes.c        | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> > index 8f6849084248..0c2489435117 100644
> > --- a/include/linux/kprobes.h
> > +++ b/include/linux/kprobes.h
> > @@ -266,6 +266,7 @@ extern int arch_init_kprobes(void);
> >  extern void show_registers(struct pt_regs *regs);
> >  extern void kprobes_inc_nmissed_count(struct kprobe *p);
> >  extern bool arch_within_kprobe_blacklist(unsigned long addr);
> > +extern bool arch_function_offset_within_entry(unsigned long offset);
> >  
> >  extern bool within_kprobe_blacklist(unsigned long addr);
> >  
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 43460104f119..72ecbf5a6312 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1834,12 +1834,25 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> >  }
> >  NOKPROBE_SYMBOL(pre_handler_kretprobe);
> >  
> > +bool __weak arch_function_offset_within_entry(unsigned long offset)
> > +{
> > +	return !offset;
> > +}
> > +
> >  int register_kretprobe(struct kretprobe *rp)
> >  {
> >  	int ret = 0;
> >  	struct kretprobe_instance *inst;
> >  	int i;
> >  	void *addr;
> > +	unsigned long offset;
> > +
> > +	addr = kprobe_addr(&rp->kp);
> > +	if (!kallsyms_lookup_size_offset((unsigned long)addr, NULL, &offset))
> > +		return -EINVAL;
> > +
> > +	if (!arch_function_offset_within_entry(offset))
> > +		return -EINVAL;
> >  
> >  	if (kretprobe_blacklist_size) {
> >  		addr = kprobe_addr(&rp->kp);
> > -- 
> > 2.11.0
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 

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

#1582373 — [PATCH 0/2] powerpc: kretprobe updates

I am posting the powerpc bits in the same thread so as to keep these
changes together. I am not sure how this should be taken upstream as
there are atleast three different trees involved: one for the core
kprobes infrastructure, one for powerpc and one for perf.

Thanks,
Naveen

Naveen N. Rao (2):
  powerpc: kretprobes: override default function entry offset
  perf: powerpc: choose LEP with kretprobes

 arch/powerpc/kernel/kprobes.c               | 9 +++++++++
 tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.11.0

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

#1582374 — [PATCH 1/2] powerpc: kretprobes: override default function entry offset

With ABIv2, we offset 8 bytes into a function to get at the local entry
point.

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

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index fce05a38851c..331751701fed 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -131,6 +131,15 @@ static void __kprobes set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+bool arch_function_offset_within_entry(unsigned long offset)
+{
+#ifdef PPC64_ELF_ABI_v2
+	return offset <= 8;
+#else
+	return !offset;
+#endif
+}
+
 void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
-- 
2.11.0

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

#1582377 — [PATCH 2/2] perf: powerpc: choose LEP with kretprobes

perf now uses an offset from _text/_stext for kretprobes, rather than
the actual function name. As such, let's choose the LEP for powerpc
ABIv2 so as to ensure the probe gets hit.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 1030a6e504bb..9fe0f20aa56f 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -79,11 +79,8 @@ void arch__fix_tev_from_maps(struct perf_probe_event *pev,
 	 * However, if the user specifies an offset, we fall back to using the
 	 * GEP since all userspace applications (objdump/readelf) show function
 	 * disassembly with offsets from the GEP.
-	 *
-	 * In addition, we shouldn't specify an offset for kretprobes.
 	 */
-	if (pev->point.offset || (!pev->uprobes && pev->point.retprobe) ||
-	    !map || !sym)
+	if (pev->point.offset || !map || !sym)
 		return;
 
 	lep_offset = PPC64_LOCAL_ENTRY_OFFSET(sym->arch_sym);
-- 
2.11.0

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

#1583310 — Re: [PATCH 0/2] powerpc: kretprobe updates

On Thu, 16 Feb 2017 13:47:37 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> I am posting the powerpc bits in the same thread so as to keep these
> changes together. I am not sure how this should be taken upstream as
> there are atleast three different trees involved: one for the core
> kprobes infrastructure, one for powerpc and one for perf.

Hmm, could you make these (and other related) patches and
other series in one series? Or wait for the other series
are merged correctly.

Thank you,

> 
> Thanks,
> Naveen
> 
> Naveen N. Rao (2):
>   powerpc: kretprobes: override default function entry offset
>   perf: powerpc: choose LEP with kretprobes
> 
>  arch/powerpc/kernel/kprobes.c               | 9 +++++++++
>  tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1583733 — Re: [PATCH 0/2] powerpc: kretprobe updates

Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> On Thu, 16 Feb 2017 13:47:37 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > I am posting the powerpc bits in the same thread so as to keep these
> > changes together. I am not sure how this should be taken upstream as
> > there are atleast three different trees involved: one for the core
> > kprobes infrastructure, one for powerpc and one for perf.

> Hmm, could you make these (and other related) patches and
> other series in one series? Or wait for the other series
> are merged correctly.

Well, patches like these should be done in a way that the tooling parts
can deal with kernels with or without the kernel changes, so that older
tools work with new kernels and new tools work with older kernels.

"work" as in the previous behaviour is kept when a new tool deals with
an older kernel and an older tool would warn the user that what it needs
is not present in that kernel.

Is this the case? I just looked briefly at the patch commit logs.

If it is, then I can pick the tool ones, and the others can be submitted
to the relevant trees, at some point all will be in, kernels eventually
gets updated everywhere, ditto for the tooling, all gets well.

Regards,

- Arnaldo


 
> Thank you,
> 
> > 
> > Thanks,
> > Naveen
> > 
> > Naveen N. Rao (2):
> >   powerpc: kretprobes: override default function entry offset
> >   perf: powerpc: choose LEP with kretprobes
> > 
> >  arch/powerpc/kernel/kprobes.c               | 9 +++++++++
> >  tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.11.0
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

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

#1584064 — Re: [PATCH 0/2] powerpc: kretprobe updates

On Fri, 17 Feb 2017 17:42:54 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > On Thu, 16 Feb 2017 13:47:37 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > I am posting the powerpc bits in the same thread so as to keep these
> > > changes together. I am not sure how this should be taken upstream as
> > > there are atleast three different trees involved: one for the core
> > > kprobes infrastructure, one for powerpc and one for perf.
> 
> > Hmm, could you make these (and other related) patches and
> > other series in one series? Or wait for the other series
> > are merged correctly.
> 
> Well, patches like these should be done in a way that the tooling parts
> can deal with kernels with or without the kernel changes, so that older
> tools work with new kernels and new tools work with older kernels.
> 
> "work" as in the previous behaviour is kept when a new tool deals with
> an older kernel and an older tool would warn the user that what it needs
> is not present in that kernel.
> 
> Is this the case? I just looked briefly at the patch commit logs.

Thanks Arnaldo,

Naveen, I think this one and your previous series are incompatible
with older kernel. So those should be merged in one series and
at least (1) update ftrace's README special file to show explicitly
which can accept text+offset style for kretprobes, and (2) update
perf probe side to ensure that (and fallback to previous logic if not).

Thank you,

> 
> If it is, then I can pick the tool ones, and the others can be submitted
> to the relevant trees, at some point all will be in, kernels eventually
> gets updated everywhere, ditto for the tooling, all gets well.
> 
> Regards,
> 
> - Arnaldo
> 
> 
>  
> > Thank you,
> > 
> > > 
> > > Thanks,
> > > Naveen
> > > 
> > > Naveen N. Rao (2):
> > >   powerpc: kretprobes: override default function entry offset
> > >   perf: powerpc: choose LEP with kretprobes
> > > 
> > >  arch/powerpc/kernel/kprobes.c               | 9 +++++++++
> > >  tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
> > >  2 files changed, 10 insertions(+), 4 deletions(-)
> > > 
> > > -- 
> > > 2.11.0
> > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1584458 — Re: [PATCH 0/2] powerpc: kretprobe updates

On 2017/02/19 01:42PM, Masami Hiramatsu wrote:
> On Fri, 17 Feb 2017 17:42:54 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > 
> > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > changes together. I am not sure how this should be taken upstream as
> > > > there are atleast three different trees involved: one for the core
> > > > kprobes infrastructure, one for powerpc and one for perf.
> > 
> > > Hmm, could you make these (and other related) patches and
> > > other series in one series? Or wait for the other series
> > > are merged correctly.
> > 
> > Well, patches like these should be done in a way that the tooling parts
> > can deal with kernels with or without the kernel changes, so that older
> > tools work with new kernels and new tools work with older kernels.
> > 
> > "work" as in the previous behaviour is kept when a new tool deals with
> > an older kernel and an older tool would warn the user that what it needs
> > is not present in that kernel.
> > 
> > Is this the case? I just looked briefly at the patch commit logs.
> 
> Thanks Arnaldo,
> 
> Naveen, I think this one and your previous series are incompatible
> with older kernel. So those should be merged in one series and
> at least (1) update ftrace's README special file to show explicitly
> which can accept text+offset style for kretprobes, and 

Sure - do you mean Documentation/trace/kprobetrace.txt? And, do you want 
me to include kernel version where this changed?

> (2) update
> perf probe side to ensure that (and fallback to previous logic if not).

Sure. I am trying out an approach and will post it as soon as it's 
ready.

Thanks!
- Naveen

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

#1585280 — Re: [PATCH 0/2] powerpc: kretprobe updates

On Mon, 20 Feb 2017 15:20:24 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/02/19 01:42PM, Masami Hiramatsu wrote:
> > On Fri, 17 Feb 2017 17:42:54 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > 
> > > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > > changes together. I am not sure how this should be taken upstream as
> > > > > there are atleast three different trees involved: one for the core
> > > > > kprobes infrastructure, one for powerpc and one for perf.
> > > 
> > > > Hmm, could you make these (and other related) patches and
> > > > other series in one series? Or wait for the other series
> > > > are merged correctly.
> > > 
> > > Well, patches like these should be done in a way that the tooling parts
> > > can deal with kernels with or without the kernel changes, so that older
> > > tools work with new kernels and new tools work with older kernels.
> > > 
> > > "work" as in the previous behaviour is kept when a new tool deals with
> > > an older kernel and an older tool would warn the user that what it needs
> > > is not present in that kernel.
> > > 
> > > Is this the case? I just looked briefly at the patch commit logs.
> > 
> > Thanks Arnaldo,
> > 
> > Naveen, I think this one and your previous series are incompatible
> > with older kernel. So those should be merged in one series and
> > at least (1) update ftrace's README special file to show explicitly
> > which can accept text+offset style for kretprobes, and 
> 
> Sure - do you mean Documentation/trace/kprobetrace.txt? And, do you want 
> me to include kernel version where this changed?

No, I meant /sys/kernel/debug/tracing/README. For some reasons, perf
probe already parse it in util/probe-file.c. 
Please see commit 180b20616ce57e93eb692170c793be94c456b1e2 and 
864256255597aad86abcecbe6c53da8852ded15b

Thank you,

> 
> > (2) update
> > perf probe side to ensure that (and fallback to previous logic if not).
> 
> Sure. I am trying out an approach and will post it as soon as it's 
> ready.
> 
> Thanks!
> - Naveen
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

#1586161 — Re: [PATCH 0/2] powerpc: kretprobe updates

On 2017/02/21 10:07PM, Masami Hiramatsu wrote:
> On Mon, 20 Feb 2017 15:20:24 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > On 2017/02/19 01:42PM, Masami Hiramatsu wrote:
> > > On Fri, 17 Feb 2017 17:42:54 -0300
> > > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > 
> > > > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > > > changes together. I am not sure how this should be taken upstream as
> > > > > > there are atleast three different trees involved: one for the core
> > > > > > kprobes infrastructure, one for powerpc and one for perf.
> > > > 
> > > > > Hmm, could you make these (and other related) patches and
> > > > > other series in one series? Or wait for the other series
> > > > > are merged correctly.
> > > > 
> > > > Well, patches like these should be done in a way that the tooling parts
> > > > can deal with kernels with or without the kernel changes, so that older
> > > > tools work with new kernels and new tools work with older kernels.
> > > > 
> > > > "work" as in the previous behaviour is kept when a new tool deals with
> > > > an older kernel and an older tool would warn the user that what it needs
> > > > is not present in that kernel.
> > > > 
> > > > Is this the case? I just looked briefly at the patch commit logs.
> > > 
> > > Thanks Arnaldo,
> > > 
> > > Naveen, I think this one and your previous series are incompatible
> > > with older kernel. So those should be merged in one series and
> > > at least (1) update ftrace's README special file to show explicitly
> > > which can accept text+offset style for kretprobes, and 
> > 
> > Sure - do you mean Documentation/trace/kprobetrace.txt? And, do you want 
> > me to include kernel version where this changed?
> 
> No, I meant /sys/kernel/debug/tracing/README. For some reasons, perf
> probe already parse it in util/probe-file.c. 
> Please see commit 180b20616ce57e93eb692170c793be94c456b1e2 and 
> 864256255597aad86abcecbe6c53da8852ded15b

Got it.

Thanks,
Naveen

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web