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

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

[Naveen N. Rao]

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

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

[Ananth N Mavinakayanahalli]

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>

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

[Masami Hiramatsu]

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>

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

[Naveen N. Rao]

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>
> 

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

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

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

[Masami Hiramatsu]

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>

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

[Naveen N. Rao]

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>
> 

[PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

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>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Arnaldo Carvalho de Melo]

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>

PowerMac G5 Quad Strage lspci

[luigi burdo]

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

Hi all devs,

from 4.10 i have on G5 Quad this strange lspci

with this 0001:01:01.0 Non-VGA unclassified device: Device 0800:0002 (rev 08)

some one know if it a bug or a new feature of the kernel.


Thanks!


My complete

lspci
0000:00:0b.0 PCI bridge: Apple Inc. CPC945 PCIe Bridge
0000:0a:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Whistler LE [Radeon HD 6610M/7610M]
0000:0a:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Turks HDMI Audio [Radeon HD 6500/6600 / 6700M Series]
0001:00:00.0 Host bridge: Apple Inc. U4 HT Bridge
0001:00:01.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-X bridge (rev a3)
0001:00:02.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-X bridge (rev a3)
0001:00:03.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:04.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:05.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:06.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:07.0 PCI bridge: Apple Inc. Shasta PCI Bridge
0001:00:08.0 PCI bridge: Apple Inc. Shasta PCI Bridge
0001:00:09.0 PCI bridge: Apple Inc. Shasta PCI Bridge
0001:01:01.0 Non-VGA unclassified device: Device 0800:0002 (rev 08)
0001:01:07.0 Unassigned class [ff00]: Apple Inc. Shasta Mac I/O
0001:01:0b.0 USB controller: NEC Corporation OHCI USB Controller (rev 43)
0001:01:0b.1 USB controller: NEC Corporation OHCI USB Controller (rev 43)
0001:01:0b.2 USB controller: NEC Corporation uPD72010x USB 2.0 Controller (rev 04)
0001:03:0c.0 IDE interface: Broadcom K2 SATA
0001:03:0d.0 Unassigned class [ff00]: Apple Inc. Shasta IDE
0001:03:0e.0 FireWire (IEEE 1394): Apple Inc. Shasta Firewire
0001:05:04.0 Ethernet controller: Broadcom Limited NetXtreme BCM5780 Gigabit Ethernet (rev 03)
0001:05:04.1 Ethernet controller: Broadcom Limited NetXtreme BCM5780 Gigabit Ethernet (rev 03)
0001:06:00.0 VGA compatible controller: NVIDIA Corporation G70GL [Quadro FX 4500] (rev a1)

Luigi


[-- Attachment #2: Type: text/html, Size: 2656 bytes --]

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

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>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

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

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

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>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

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

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

On 2017/02/17 05:42PM, Arnaldo Carvalho de Melo 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, that makes sense.

All the kernel patches here can go in as they are about removing the 
restrictions around use of kretprobes with kprobe_event, except for the 
first patch which hardens use of kretprobes in general by validating 
addresses. None of that should cause issues with the existing tools.

> 
> 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.

Older perf tools with newer kernels will be fine. However, with the 
current perf patches, newer perf will fail with older kernels. I will 
redo the perf patches to address this. So, please don't merge the perf 
bits in this series.


Regards,
Naveen

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

On 2017/02/17 05:42PM, Arnaldo Carvalho de Melo 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.

Does the below work?

The idea is to just prefer the real function names when probing 
functions that do not have duplicate names. Offset from _text only if we 
detect that a function name has multiple entries. In this manner,
existing perf will continue to work with newer kernels and vice-versa.

Before this patch:

naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
p:probe/_do_fork _text+857288
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
r:probe/_do_fork _text+857288
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
p:probe/read_mem _text+10019704
p:probe/read_mem_1 _text+6228424
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
r:probe/read_mem _text+10019704
r:probe/read_mem_1 _text+6228424


With the below patch (lightly tested):

naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
p:probe/_do_fork _do_fork+8
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
r:probe/_do_fork _do_fork+8
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
p:probe/read_mem _text+10019704
p:probe/read_mem_1 _text+6228424
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
r:probe/read_mem _text+10019704
r:probe/read_mem_1 _text+6228424

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/machine.h     |  7 +++++
 tools/perf/util/map.c         | 41 +++++++++++++++++++++++++++++
 tools/perf/util/map.h         | 13 ++++++++++
 tools/perf/util/probe-event.c | 18 +++++++++++++
 tools/perf/util/symbol.c      | 60 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/symbol.h      |  2 ++
 6 files changed, 141 insertions(+)

diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index 354de6e56109..277ffb1e0896 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -203,6 +203,13 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
 	return map_groups__find_function_by_name(&machine->kmaps, name, mapp);
 }
 
+static inline
+unsigned int machine__find_kernel_function_count_by_name(struct machine *machine,
+						     const char *name)
+{
+	return map_groups__find_function_count_by_name(&machine->kmaps, name);
+}
+
 struct map *machine__findnew_module_map(struct machine *machine, u64 start,
 					const char *filename);
 int arch__fix_module_text_start(u64 *start, const char *name);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 4f9a71c63026..b75b35dc54ad 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -349,6 +349,17 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name)
 	return dso__find_symbol_by_name(map->dso, map->type, name);
 }
 
+unsigned int map__find_symbol_count_by_name(struct map *map, const char *name)
+{
+	if (map__load(map) < 0)
+		return 0;
+
+	if (!dso__sorted_by_name(map->dso, map->type))
+		dso__sort_by_name(map->dso, map->type);
+
+	return dso__find_symbol_count_by_name(map->dso, map->type, name);
+}
+
 struct map *map__clone(struct map *from)
 {
 	struct map *map = memdup(from, sizeof(*map));
@@ -593,6 +604,29 @@ struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
 	return sym;
 }
 
+unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name)
+{
+	struct symbol *sym;
+	struct rb_node *nd;
+	unsigned int count = 0;
+
+	pthread_rwlock_rdlock(&maps->lock);
+
+	for (nd = rb_first(&maps->entries); nd; nd = rb_next(nd)) {
+		struct map *pos = rb_entry(nd, struct map, rb_node);
+
+		sym = map__find_symbol_by_name(pos, name);
+
+		if (sym != NULL) {
+			count = map__find_symbol_count_by_name(pos, name);
+			break;
+		}
+	}
+
+	pthread_rwlock_unlock(&maps->lock);
+	return count;
+}
+
 struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 					       enum map_type type,
 					       const char *name,
@@ -603,6 +637,13 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 	return sym;
 }
 
+unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
+					       enum map_type type,
+					       const char *name)
+{
+	return maps__find_symbol_count_by_name(&mg->maps[type], name);
+}
+
 int map_groups__find_ams(struct addr_map_symbol *ams)
 {
 	if (ams->addr < ams->map->start || ams->addr >= ams->map->end) {
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index abdacf800c98..ecf995f033ed 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -173,6 +173,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
 int map__load(struct map *map);
 struct symbol *map__find_symbol(struct map *map, u64 addr);
 struct symbol *map__find_symbol_by_name(struct map *map, const char *name);
+unsigned int map__find_symbol_count_by_name(struct map *map, const char *name);
 void map__fixup_start(struct map *map);
 void map__fixup_end(struct map *map);
 
@@ -187,6 +188,7 @@ struct map *maps__first(struct maps *maps);
 struct map *map__next(struct map *map);
 struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
                                          struct map **mapp);
+unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name);
 void map_groups__init(struct map_groups *mg, struct machine *machine);
 void map_groups__exit(struct map_groups *mg);
 int map_groups__clone(struct thread *thread,
@@ -233,6 +235,10 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 					       const char *name,
 					       struct map **mapp);
 
+unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
+					       enum map_type type,
+					       const char *name);
+
 struct addr_map_symbol;
 
 int map_groups__find_ams(struct addr_map_symbol *ams);
@@ -244,6 +250,13 @@ struct symbol *map_groups__find_function_by_name(struct map_groups *mg,
 	return map_groups__find_symbol_by_name(mg, MAP__FUNCTION, name, mapp);
 }
 
+static inline
+unsigned int map_groups__find_function_count_by_name(struct map_groups *mg,
+						 const char *name)
+{
+	return map_groups__find_symbol_count_by_name(mg, MAP__FUNCTION, name);
+}
+
 int map_groups__fixup_overlappings(struct map_groups *mg, struct map *map,
 				   FILE *fp);
 
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index fa7f81af11e8..011e938a0a84 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -113,6 +113,11 @@ static struct symbol *__find_kernel_function_by_name(const char *name,
 	return machine__find_kernel_function_by_name(host_machine, name, mapp);
 }
 
+static unsigned int __find_kernel_function_count_by_name(const char *name)
+{
+	return machine__find_kernel_function_count_by_name(host_machine, name);
+}
+
 static struct symbol *__find_kernel_function(u64 addr, struct map **mapp)
 {
 	return machine__find_kernel_function(host_machine, addr, mapp);
@@ -155,6 +160,11 @@ static int kernel_get_symbol_address_by_name(const char *name, u64 *addr,
 	return 0;
 }
 
+static unsigned int kernel_get_symbol_count_by_name(const char *name)
+{
+	return __find_kernel_function_count_by_name(name);
+}
+
 static struct map *kernel_get_module_map(const char *module)
 {
 	struct map_groups *grp = &host_machine->kmaps;
@@ -259,6 +269,11 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
 	return true;
 }
 
+static bool kprobe_is_duplicate_symbol(const char *symbol)
+{
+	return kernel_get_symbol_count_by_name(symbol) > 1;
+}
+
 /*
  * @module can be module name of module file path. In case of path,
  * inspect elf and find out what is actual module name.
@@ -759,6 +774,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 	for (i = 0; i < ntevs; i++) {
 		if (!tevs[i].point.address)
 			continue;
+		if (!kprobe_is_duplicate_symbol(tevs[i].point.symbol))
+			continue;
+
 		/* If we found a wrong one, mark it by NULL symbol */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
 					  tevs[i].point.address)) {
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dc93940de351..ca4a11a92c6f 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -438,6 +438,60 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 	return &s->sym;
 }
 
+static unsigned int symbols__find_count_by_name(struct rb_root *symbols,
+					    const char *name)
+{
+	struct rb_node *n, *m;
+	struct symbol_name_rb_node *s = NULL;
+	unsigned int count = 0;
+
+	if (symbols == NULL)
+		return 0;
+
+	n = symbols->rb_node;
+
+	while (n) {
+		int cmp;
+
+		s = rb_entry(n, struct symbol_name_rb_node, rb_node);
+		cmp = arch__compare_symbol_names(name, s->sym.name);
+
+		if (cmp < 0)
+			n = n->rb_left;
+		else if (cmp > 0)
+			n = n->rb_right;
+		else
+			break;
+	}
+
+	if (n == NULL)
+		return 0;
+
+	count++;
+
+	for (m = rb_prev(n); m; m = rb_prev(m)) {
+		struct symbol_name_rb_node *tmp;
+
+		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
+		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
+			break;
+
+		count++;
+	}
+
+	for (m = rb_next(n); m; m = rb_next(m)) {
+		struct symbol_name_rb_node *tmp;
+
+		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
+		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
+			break;
+
+		count++;
+	}
+
+	return count;
+}
+
 void dso__reset_find_symbol_cache(struct dso *dso)
 {
 	enum map_type type;
@@ -503,6 +557,12 @@ struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
 	return symbols__find_by_name(&dso->symbol_names[type], name);
 }
 
+unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
+					const char *name)
+{
+	return symbols__find_count_by_name(&dso->symbol_names[type], name);
+}
+
 void dso__sort_by_name(struct dso *dso, enum map_type type)
 {
 	dso__set_sorted_by_name(dso, type);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 6c358b7ed336..c4bd1b035a9b 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -260,6 +260,8 @@ struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
 				u64 addr);
 struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
 					const char *name);
+unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
+					const char *name);
 struct symbol *symbol__next_by_name(struct symbol *sym);
 
 struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
-- 
2.11.0

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

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

> On 2017/02/17 05:42PM, Arnaldo Carvalho de Melo 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.
> 
> Does the below work?

Sorry, no that is not what we meant. Please see my previous reply.

> 
> The idea is to just prefer the real function names when probing 
> functions that do not have duplicate names. Offset from _text only if we 
> detect that a function name has multiple entries. In this manner,
> existing perf will continue to work with newer kernels and vice-versa.

Even with this, the latter will fail (instead of putting rprobe on the
first symbol) on older kernel. Instead, we'll check the ftrace's README
on current kernel and if it supports sym+offs style on retprobe,
we'll use _text+offs event definition. If not, we continue to use
older style.

Thank you,

> 
> Before this patch:
> 
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
> p:probe/_do_fork _text+857288
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
> r:probe/_do_fork _text+857288
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
> p:probe/read_mem _text+10019704
> p:probe/read_mem_1 _text+6228424
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
> r:probe/read_mem _text+10019704
> r:probe/read_mem_1 _text+6228424
> 
> 
> With the below patch (lightly tested):
> 
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
> p:probe/_do_fork _do_fork+8
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
> r:probe/_do_fork _do_fork+8
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
> p:probe/read_mem _text+10019704
> p:probe/read_mem_1 _text+6228424
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
> r:probe/read_mem _text+10019704
> r:probe/read_mem_1 _text+6228424
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/machine.h     |  7 +++++
>  tools/perf/util/map.c         | 41 +++++++++++++++++++++++++++++
>  tools/perf/util/map.h         | 13 ++++++++++
>  tools/perf/util/probe-event.c | 18 +++++++++++++
>  tools/perf/util/symbol.c      | 60 +++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/symbol.h      |  2 ++
>  6 files changed, 141 insertions(+)
> 
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index 354de6e56109..277ffb1e0896 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -203,6 +203,13 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
>  	return map_groups__find_function_by_name(&machine->kmaps, name, mapp);
>  }
>  
> +static inline
> +unsigned int machine__find_kernel_function_count_by_name(struct machine *machine,
> +						     const char *name)
> +{
> +	return map_groups__find_function_count_by_name(&machine->kmaps, name);
> +}
> +
>  struct map *machine__findnew_module_map(struct machine *machine, u64 start,
>  					const char *filename);
>  int arch__fix_module_text_start(u64 *start, const char *name);
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 4f9a71c63026..b75b35dc54ad 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -349,6 +349,17 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name)
>  	return dso__find_symbol_by_name(map->dso, map->type, name);
>  }
>  
> +unsigned int map__find_symbol_count_by_name(struct map *map, const char *name)
> +{
> +	if (map__load(map) < 0)
> +		return 0;
> +
> +	if (!dso__sorted_by_name(map->dso, map->type))
> +		dso__sort_by_name(map->dso, map->type);
> +
> +	return dso__find_symbol_count_by_name(map->dso, map->type, name);
> +}
> +
>  struct map *map__clone(struct map *from)
>  {
>  	struct map *map = memdup(from, sizeof(*map));
> @@ -593,6 +604,29 @@ struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
>  	return sym;
>  }
>  
> +unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name)
> +{
> +	struct symbol *sym;
> +	struct rb_node *nd;
> +	unsigned int count = 0;
> +
> +	pthread_rwlock_rdlock(&maps->lock);
> +
> +	for (nd = rb_first(&maps->entries); nd; nd = rb_next(nd)) {
> +		struct map *pos = rb_entry(nd, struct map, rb_node);
> +
> +		sym = map__find_symbol_by_name(pos, name);
> +
> +		if (sym != NULL) {
> +			count = map__find_symbol_count_by_name(pos, name);
> +			break;
> +		}
> +	}
> +
> +	pthread_rwlock_unlock(&maps->lock);
> +	return count;
> +}
> +
>  struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
>  					       enum map_type type,
>  					       const char *name,
> @@ -603,6 +637,13 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
>  	return sym;
>  }
>  
> +unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
> +					       enum map_type type,
> +					       const char *name)
> +{
> +	return maps__find_symbol_count_by_name(&mg->maps[type], name);
> +}
> +
>  int map_groups__find_ams(struct addr_map_symbol *ams)
>  {
>  	if (ams->addr < ams->map->start || ams->addr >= ams->map->end) {
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index abdacf800c98..ecf995f033ed 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -173,6 +173,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
>  int map__load(struct map *map);
>  struct symbol *map__find_symbol(struct map *map, u64 addr);
>  struct symbol *map__find_symbol_by_name(struct map *map, const char *name);
> +unsigned int map__find_symbol_count_by_name(struct map *map, const char *name);
>  void map__fixup_start(struct map *map);
>  void map__fixup_end(struct map *map);
>  
> @@ -187,6 +188,7 @@ struct map *maps__first(struct maps *maps);
>  struct map *map__next(struct map *map);
>  struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
>                                           struct map **mapp);
> +unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name);
>  void map_groups__init(struct map_groups *mg, struct machine *machine);
>  void map_groups__exit(struct map_groups *mg);
>  int map_groups__clone(struct thread *thread,
> @@ -233,6 +235,10 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
>  					       const char *name,
>  					       struct map **mapp);
>  
> +unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
> +					       enum map_type type,
> +					       const char *name);
> +
>  struct addr_map_symbol;
>  
>  int map_groups__find_ams(struct addr_map_symbol *ams);
> @@ -244,6 +250,13 @@ struct symbol *map_groups__find_function_by_name(struct map_groups *mg,
>  	return map_groups__find_symbol_by_name(mg, MAP__FUNCTION, name, mapp);
>  }
>  
> +static inline
> +unsigned int map_groups__find_function_count_by_name(struct map_groups *mg,
> +						 const char *name)
> +{
> +	return map_groups__find_symbol_count_by_name(mg, MAP__FUNCTION, name);
> +}
> +
>  int map_groups__fixup_overlappings(struct map_groups *mg, struct map *map,
>  				   FILE *fp);
>  
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index fa7f81af11e8..011e938a0a84 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -113,6 +113,11 @@ static struct symbol *__find_kernel_function_by_name(const char *name,
>  	return machine__find_kernel_function_by_name(host_machine, name, mapp);
>  }
>  
> +static unsigned int __find_kernel_function_count_by_name(const char *name)
> +{
> +	return machine__find_kernel_function_count_by_name(host_machine, name);
> +}
> +
>  static struct symbol *__find_kernel_function(u64 addr, struct map **mapp)
>  {
>  	return machine__find_kernel_function(host_machine, addr, mapp);
> @@ -155,6 +160,11 @@ static int kernel_get_symbol_address_by_name(const char *name, u64 *addr,
>  	return 0;
>  }
>  
> +static unsigned int kernel_get_symbol_count_by_name(const char *name)
> +{
> +	return __find_kernel_function_count_by_name(name);
> +}
> +
>  static struct map *kernel_get_module_map(const char *module)
>  {
>  	struct map_groups *grp = &host_machine->kmaps;
> @@ -259,6 +269,11 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>  	return true;
>  }
>  
> +static bool kprobe_is_duplicate_symbol(const char *symbol)
> +{
> +	return kernel_get_symbol_count_by_name(symbol) > 1;
> +}
> +
>  /*
>   * @module can be module name of module file path. In case of path,
>   * inspect elf and find out what is actual module name.
> @@ -759,6 +774,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  	for (i = 0; i < ntevs; i++) {
>  		if (!tevs[i].point.address)
>  			continue;
> +		if (!kprobe_is_duplicate_symbol(tevs[i].point.symbol))
> +			continue;
> +
>  		/* If we found a wrong one, mark it by NULL symbol */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
>  					  tevs[i].point.address)) {
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index dc93940de351..ca4a11a92c6f 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -438,6 +438,60 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>  	return &s->sym;
>  }
>  
> +static unsigned int symbols__find_count_by_name(struct rb_root *symbols,
> +					    const char *name)
> +{
> +	struct rb_node *n, *m;
> +	struct symbol_name_rb_node *s = NULL;
> +	unsigned int count = 0;
> +
> +	if (symbols == NULL)
> +		return 0;
> +
> +	n = symbols->rb_node;
> +
> +	while (n) {
> +		int cmp;
> +
> +		s = rb_entry(n, struct symbol_name_rb_node, rb_node);
> +		cmp = arch__compare_symbol_names(name, s->sym.name);
> +
> +		if (cmp < 0)
> +			n = n->rb_left;
> +		else if (cmp > 0)
> +			n = n->rb_right;
> +		else
> +			break;
> +	}
> +
> +	if (n == NULL)
> +		return 0;
> +
> +	count++;
> +
> +	for (m = rb_prev(n); m; m = rb_prev(m)) {
> +		struct symbol_name_rb_node *tmp;
> +
> +		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
> +		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> +			break;
> +
> +		count++;
> +	}
> +
> +	for (m = rb_next(n); m; m = rb_next(m)) {
> +		struct symbol_name_rb_node *tmp;
> +
> +		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
> +		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> +			break;
> +
> +		count++;
> +	}
> +
> +	return count;
> +}
> +
>  void dso__reset_find_symbol_cache(struct dso *dso)
>  {
>  	enum map_type type;
> @@ -503,6 +557,12 @@ struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
>  	return symbols__find_by_name(&dso->symbol_names[type], name);
>  }
>  
> +unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
> +					const char *name)
> +{
> +	return symbols__find_count_by_name(&dso->symbol_names[type], name);
> +}
> +
>  void dso__sort_by_name(struct dso *dso, enum map_type type)
>  {
>  	dso__set_sorted_by_name(dso, type);
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 6c358b7ed336..c4bd1b035a9b 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -260,6 +260,8 @@ struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
>  				u64 addr);
>  struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
>  					const char *name);
> +unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
> +					const char *name);
>  struct symbol *symbol__next_by_name(struct symbol *sym);
>  
>  struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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

> 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.

Oops, it was so. That should be fixed correctly...

> 
> 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.

Yeah.

> 
> > 
> > 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!

> 
> 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>
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>