* [PATCH] kretprobes: reject registration if a symbol offset is specified @ 2017-02-14 8:31 Naveen N. Rao 2017-02-14 8:41 ` Ananth N Mavinakayanahalli 2017-02-14 10:32 ` Masami Hiramatsu 0 siblings, 2 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-14 8:31 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Ingo Molnar; +Cc: linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] kretprobes: reject registration if a symbol offset is specified 2017-02-14 8:31 [PATCH] kretprobes: reject registration if a symbol offset is specified Naveen N. Rao @ 2017-02-14 8:41 ` Ananth N Mavinakayanahalli 2017-02-14 10:32 ` Masami Hiramatsu 1 sibling, 0 replies; 25+ messages in thread From: Ananth N Mavinakayanahalli @ 2017-02-14 8:41 UTC (permalink / raw) To: Naveen N. Rao; +Cc: Masami Hiramatsu, Ingo Molnar, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] kretprobes: reject registration if a symbol offset is specified 2017-02-14 8:31 [PATCH] kretprobes: reject registration if a symbol offset is specified Naveen N. Rao 2017-02-14 8:41 ` Ananth N Mavinakayanahalli @ 2017-02-14 10:32 ` Masami Hiramatsu 2017-02-15 17:53 ` Naveen N. Rao 1 sibling, 1 reply; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-14 10:32 UTC (permalink / raw) To: Naveen N. Rao; +Cc: Ananth N Mavinakayanahalli, Ingo Molnar, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] kretprobes: reject registration if a symbol offset is specified 2017-02-14 10:32 ` Masami Hiramatsu @ 2017-02-15 17:53 ` Naveen N. Rao 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao 2017-02-15 23:28 ` [PATCH] kretprobes: reject registration if a symbol offset is specified Masami Hiramatsu 0 siblings, 2 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-15 17:53 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ananth N Mavinakayanahalli, Ingo Molnar, linux-kernel, Arnaldo Carvalho de Melo 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> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] kretprobes: ensure probe location is at function entry 2017-02-15 17:53 ` Naveen N. Rao @ 2017-02-15 18:17 ` Naveen N. Rao 2017-02-15 18:17 ` [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses Naveen N. Rao ` (3 more replies) 2017-02-15 23:28 ` [PATCH] kretprobes: reject registration if a symbol offset is specified Masami Hiramatsu 1 sibling, 4 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-15 18:17 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Arnaldo Carvalho de Melo Cc: Ingo Molnar, Namhyung Kim, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao @ 2017-02-15 18:17 ` Naveen N. Rao 2017-02-15 23:43 ` Masami Hiramatsu 2017-02-15 18:17 ` [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" Naveen N. Rao ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Naveen N. Rao @ 2017-02-15 18:17 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Arnaldo Carvalho de Melo Cc: Ingo Molnar, Namhyung Kim, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses 2017-02-15 18:17 ` [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses Naveen N. Rao @ 2017-02-15 23:43 ` Masami Hiramatsu 0 siblings, 0 replies; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-15 23:43 UTC (permalink / raw) To: Naveen N. Rao Cc: Ananth N Mavinakayanahalli, Arnaldo Carvalho de Melo, Ingo Molnar, Namhyung Kim, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao 2017-02-15 18:17 ` [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses Naveen N. Rao @ 2017-02-15 18:17 ` Naveen N. Rao 2017-02-15 23:43 ` Masami Hiramatsu 2017-02-15 23:39 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Masami Hiramatsu 2017-02-16 8:17 ` [PATCH 0/2] powerpc: kretprobe updates Naveen N. Rao 3 siblings, 1 reply; 25+ messages in thread From: Naveen N. Rao @ 2017-02-15 18:17 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Arnaldo Carvalho de Melo Cc: Ingo Molnar, Namhyung Kim, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" 2017-02-15 18:17 ` [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" Naveen N. Rao @ 2017-02-15 23:43 ` Masami Hiramatsu 0 siblings, 0 replies; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-15 23:43 UTC (permalink / raw) To: Naveen N. Rao Cc: Ananth N Mavinakayanahalli, Arnaldo Carvalho de Melo, Ingo Molnar, Namhyung Kim, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] kretprobes: ensure probe location is at function entry 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao 2017-02-15 18:17 ` [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses Naveen N. Rao 2017-02-15 18:17 ` [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" Naveen N. Rao @ 2017-02-15 23:39 ` Masami Hiramatsu 2017-02-16 7:51 ` Naveen N. Rao 2017-02-16 8:17 ` [PATCH 0/2] powerpc: kretprobe updates Naveen N. Rao 3 siblings, 1 reply; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-15 23:39 UTC (permalink / raw) To: Naveen N. Rao Cc: Ananth N Mavinakayanahalli, Arnaldo Carvalho de Melo, Ingo Molnar, Namhyung Kim, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] kretprobes: ensure probe location is at function entry 2017-02-15 23:39 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Masami Hiramatsu @ 2017-02-16 7:51 ` Naveen N. Rao 0 siblings, 0 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-16 7:51 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ananth N Mavinakayanahalli, Arnaldo Carvalho de Melo, Ingo Molnar, Namhyung Kim, linux-kernel, linux-arch 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> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 0/2] powerpc: kretprobe updates 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao ` (2 preceding siblings ...) 2017-02-15 23:39 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Masami Hiramatsu @ 2017-02-16 8:17 ` Naveen N. Rao 2017-02-16 8:17 ` [PATCH 1/2] powerpc: kretprobes: override default function entry offset Naveen N. Rao ` (2 more replies) 3 siblings, 3 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-16 8:17 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Arnaldo Carvalho de Melo, Michael Ellerman Cc: Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] powerpc: kretprobes: override default function entry offset 2017-02-16 8:17 ` [PATCH 0/2] powerpc: kretprobe updates Naveen N. Rao @ 2017-02-16 8:17 ` Naveen N. Rao 2017-02-16 8:17 ` [PATCH 2/2] perf: powerpc: choose LEP with kretprobes Naveen N. Rao 2017-02-17 10:44 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu 2 siblings, 0 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-16 8:17 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Arnaldo Carvalho de Melo, Michael Ellerman Cc: Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] perf: powerpc: choose LEP with kretprobes 2017-02-16 8:17 ` [PATCH 0/2] powerpc: kretprobe updates Naveen N. Rao 2017-02-16 8:17 ` [PATCH 1/2] powerpc: kretprobes: override default function entry offset Naveen N. Rao @ 2017-02-16 8:17 ` Naveen N. Rao 2017-02-17 10:44 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu 2 siblings, 0 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-16 8:17 UTC (permalink / raw) To: Ananth N Mavinakayanahalli, Masami Hiramatsu, Arnaldo Carvalho de Melo, Michael Ellerman Cc: Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-16 8:17 ` [PATCH 0/2] powerpc: kretprobe updates Naveen N. Rao 2017-02-16 8:17 ` [PATCH 1/2] powerpc: kretprobes: override default function entry offset Naveen N. Rao 2017-02-16 8:17 ` [PATCH 2/2] perf: powerpc: choose LEP with kretprobes Naveen N. Rao @ 2017-02-17 10:44 ` Masami Hiramatsu 2017-02-17 20:42 ` Arnaldo Carvalho de Melo 2 siblings, 1 reply; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-17 10:44 UTC (permalink / raw) To: Naveen N. Rao Cc: Ananth N Mavinakayanahalli, Arnaldo Carvalho de Melo, Michael Ellerman, Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-17 10:44 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu @ 2017-02-17 20:42 ` Arnaldo Carvalho de Melo 2017-02-17 20:50 ` PowerMac G5 Quad Strage lspci luigi burdo ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Arnaldo Carvalho de Melo @ 2017-02-17 20:42 UTC (permalink / raw) To: Masami Hiramatsu Cc: Naveen N. Rao, Ananth N Mavinakayanahalli, Michael Ellerman, Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* PowerMac G5 Quad Strage lspci 2017-02-17 20:42 ` Arnaldo Carvalho de Melo @ 2017-02-17 20:50 ` luigi burdo 2017-02-19 4:42 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu ` (2 subsequent siblings) 3 siblings, 0 replies; 25+ messages in thread From: luigi burdo @ 2017-02-17 20:50 UTC (permalink / raw) To: linux-kernel, linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-17 20:42 ` Arnaldo Carvalho de Melo 2017-02-17 20:50 ` PowerMac G5 Quad Strage lspci luigi burdo @ 2017-02-19 4:42 ` Masami Hiramatsu 2017-02-20 9:50 ` Naveen N. Rao 2017-02-20 9:46 ` Naveen N. Rao 2017-02-20 11:43 ` Naveen N. Rao 3 siblings, 1 reply; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-19 4:42 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Naveen N. Rao, Ananth N Mavinakayanahalli, Michael Ellerman, Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-19 4:42 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu @ 2017-02-20 9:50 ` Naveen N. Rao 2017-02-21 13:07 ` Masami Hiramatsu 0 siblings, 1 reply; 25+ messages in thread From: Naveen N. Rao @ 2017-02-20 9:50 UTC (permalink / raw) To: Masami Hiramatsu Cc: Arnaldo Carvalho de Melo, Ananth N Mavinakayanahalli, Michael Ellerman, Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-20 9:50 ` Naveen N. Rao @ 2017-02-21 13:07 ` Masami Hiramatsu 2017-02-22 13:39 ` Naveen N. Rao 0 siblings, 1 reply; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-21 13:07 UTC (permalink / raw) To: Naveen N. Rao Cc: Arnaldo Carvalho de Melo, Ananth N Mavinakayanahalli, Michael Ellerman, Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-21 13:07 ` Masami Hiramatsu @ 2017-02-22 13:39 ` Naveen N. Rao 0 siblings, 0 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-22 13:39 UTC (permalink / raw) To: Masami Hiramatsu Cc: linux-kernel, Arnaldo Carvalho de Melo, Namhyung Kim, linuxppc-dev, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-17 20:42 ` Arnaldo Carvalho de Melo 2017-02-17 20:50 ` PowerMac G5 Quad Strage lspci luigi burdo 2017-02-19 4:42 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu @ 2017-02-20 9:46 ` Naveen N. Rao 2017-02-20 11:43 ` Naveen N. Rao 3 siblings, 0 replies; 25+ messages in thread From: Naveen N. Rao @ 2017-02-20 9:46 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Masami Hiramatsu, Ananth N Mavinakayanahalli, Michael Ellerman, Ingo Molnar, Namhyung Kim, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-17 20:42 ` Arnaldo Carvalho de Melo ` (2 preceding siblings ...) 2017-02-20 9:46 ` Naveen N. Rao @ 2017-02-20 11:43 ` Naveen N. Rao 2017-02-21 13:06 ` Masami Hiramatsu 3 siblings, 1 reply; 25+ messages in thread From: Naveen N. Rao @ 2017-02-20 11:43 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Masami Hiramatsu, linux-kernel, Namhyung Kim, linuxppc-dev, Ingo Molnar 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] powerpc: kretprobe updates 2017-02-20 11:43 ` Naveen N. Rao @ 2017-02-21 13:06 ` Masami Hiramatsu 0 siblings, 0 replies; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-21 13:06 UTC (permalink / raw) To: Naveen N. Rao Cc: Arnaldo Carvalho de Melo, Masami Hiramatsu, linux-kernel, Namhyung Kim, linuxppc-dev, Ingo Molnar 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] kretprobes: reject registration if a symbol offset is specified 2017-02-15 17:53 ` Naveen N. Rao 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao @ 2017-02-15 23:28 ` Masami Hiramatsu 1 sibling, 0 replies; 25+ messages in thread From: Masami Hiramatsu @ 2017-02-15 23:28 UTC (permalink / raw) To: Naveen N. Rao Cc: Ananth N Mavinakayanahalli, Ingo Molnar, linux-kernel, Arnaldo Carvalho de Melo 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-02-22 13:40 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-14 8:31 [PATCH] kretprobes: reject registration if a symbol offset is specified Naveen N. Rao 2017-02-14 8:41 ` Ananth N Mavinakayanahalli 2017-02-14 10:32 ` Masami Hiramatsu 2017-02-15 17:53 ` Naveen N. Rao 2017-02-15 18:17 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Naveen N. Rao 2017-02-15 18:17 ` [PATCH 2/3] trace/kprobes: allow return probes with offsets and absolute addresses Naveen N. Rao 2017-02-15 23:43 ` Masami Hiramatsu 2017-02-15 18:17 ` [PATCH 3/3] perf: revert "perf probe: Fix probing kretprobes" Naveen N. Rao 2017-02-15 23:43 ` Masami Hiramatsu 2017-02-15 23:39 ` [PATCH 1/3] kretprobes: ensure probe location is at function entry Masami Hiramatsu 2017-02-16 7:51 ` Naveen N. Rao 2017-02-16 8:17 ` [PATCH 0/2] powerpc: kretprobe updates Naveen N. Rao 2017-02-16 8:17 ` [PATCH 1/2] powerpc: kretprobes: override default function entry offset Naveen N. Rao 2017-02-16 8:17 ` [PATCH 2/2] perf: powerpc: choose LEP with kretprobes Naveen N. Rao 2017-02-17 10:44 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu 2017-02-17 20:42 ` Arnaldo Carvalho de Melo 2017-02-17 20:50 ` PowerMac G5 Quad Strage lspci luigi burdo 2017-02-19 4:42 ` [PATCH 0/2] powerpc: kretprobe updates Masami Hiramatsu 2017-02-20 9:50 ` Naveen N. Rao 2017-02-21 13:07 ` Masami Hiramatsu 2017-02-22 13:39 ` Naveen N. Rao 2017-02-20 9:46 ` Naveen N. Rao 2017-02-20 11:43 ` Naveen N. Rao 2017-02-21 13:06 ` Masami Hiramatsu 2017-02-15 23:28 ` [PATCH] kretprobes: reject registration if a symbol offset is specified Masami Hiramatsu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.