linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] perf-probe: Fix __init function and blacklist checking
@ 2020-04-23 11:00 Masami Hiramatsu
  2020-04-23 11:01 ` [PATCH 1/3] perf-probe: Fix to check blacklist address correctly Masami Hiramatsu
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Masami Hiramatsu @ 2020-04-23 11:00 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Masami Hiramatsu, Jiri Olsa, Namhyung Kim,
	Linux Kernel Mailing List, stable

Hi,

Here is a series of fixes related to __init function and
blacklist checking routines. Arnaldo noticed me some cases
which don't check the __init function checking. I found that
the blacklist checking is also not working with KASLR, and
also skipped probes are shown in result list unexpectedly.

Thank you,

---

Masami Hiramatsu (3):
      perf-probe: Fix to check blacklist address correctly
      perf-probe: Check address correctness by map instead of _etext
      perf-probe: Do not show the skipped events


 tools/perf/builtin-probe.c    |    3 +++
 tools/perf/util/probe-event.c |   46 +++++++++++++++++++++++++----------------
 2 files changed, 31 insertions(+), 18 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] perf-probe: Fix to check blacklist address correctly
  2020-04-23 11:00 [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Masami Hiramatsu
@ 2020-04-23 11:01 ` Masami Hiramatsu
  2020-05-06 16:44   ` Arnaldo Carvalho de Melo
  2020-04-23 11:01 ` [PATCH 2/3] perf-probe: Check address correctness by map instead of _etext Masami Hiramatsu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu @ 2020-04-23 11:01 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Masami Hiramatsu, Jiri Olsa, Namhyung Kim,
	Linux Kernel Mailing List, stable

Fix to check kprobe blacklist address correctly with
relocated address by adjusting debuginfo address.

Since the address in the debuginfo is same as objdump,
it is different from relocated kernel address with KASLR.
Thus, the perf-probe always misses to catch the
blacklisted addresses.

Without this patch, perf probe can not detect the blacklist
addresses on KASLR enabled kernel.

=========
  # perf probe kprobe_dispatcher
  Failed to write event: Invalid argument
    Error: Failed to add events.
=========

With this patch, it correctly shows the error message.

=========
  # perf probe kprobe_dispatcher
  kprobe_dispatcher is blacklisted function, skip it.
  Probe point 'kprobe_dispatcher' not found.
    Error: Failed to add events.
=========

Fixes: 9aaf5a5f479b ("perf probe: Check kprobes blacklist when adding new events")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 tools/perf/util/probe-event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index eea132f512b0..f75df63309be 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -102,7 +102,7 @@ void exit_probe_symbol_maps(void)
 	symbol__exit();
 }
 
-static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
+static struct ref_reloc_sym *kernel_get_ref_reloc_sym(struct map **pmap)
 {
 	/* kmap->ref_reloc_sym should be set if host_machine is initialized */
 	struct kmap *kmap;
@@ -114,6 +114,10 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
 	kmap = map__kmap(map);
 	if (!kmap)
 		return NULL;
+
+	if (pmap)
+		*pmap = map;
+
 	return kmap->ref_reloc_sym;
 }
 
@@ -125,7 +129,7 @@ static int kernel_get_symbol_address_by_name(const char *name, u64 *addr,
 	struct map *map;
 
 	/* ref_reloc_sym is just a label. Need a special fix*/
-	reloc_sym = kernel_get_ref_reloc_sym();
+	reloc_sym = kernel_get_ref_reloc_sym(NULL);
 	if (reloc_sym && strcmp(name, reloc_sym->name) == 0)
 		*addr = (reloc) ? reloc_sym->addr : reloc_sym->unrelocated_addr;
 	else {
@@ -745,6 +749,7 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 				       int ntevs)
 {
 	struct ref_reloc_sym *reloc_sym;
+	struct map *map;
 	char *tmp;
 	int i, skipped = 0;
 
@@ -753,7 +758,7 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 		return post_process_offline_probe_trace_events(tevs, ntevs,
 						symbol_conf.vmlinux_name);
 
-	reloc_sym = kernel_get_ref_reloc_sym();
+	reloc_sym = kernel_get_ref_reloc_sym(&map);
 	if (!reloc_sym) {
 		pr_warning("Relocated base symbol is not found!\n");
 		return -EINVAL;
@@ -764,9 +769,13 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 			continue;
 		if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
 			continue;
-		/* If we found a wrong one, mark it by NULL symbol */
+		/*
+		 * If we found a wrong one, mark it by NULL symbol.
+		 * Since addresses in debuginfo is same as objdump, we need
+		 * to convert it to addresses on memory.
+		 */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
-					  tevs[i].point.address)) {
+			map__objdump_2mem(map, tevs[i].point.address))) {
 			tmp = NULL;
 			skipped++;
 		} else {
@@ -2936,7 +2945,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 && !pev->target &&
 			(!pp->retprobe || kretprobe_offset_is_supported())) {
-		reloc_sym = kernel_get_ref_reloc_sym();
+		reloc_sym = kernel_get_ref_reloc_sym(NULL);
 		if (!reloc_sym) {
 			pr_warning("Relocated base symbol is not found!\n");
 			ret = -EINVAL;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] perf-probe: Check address correctness by map instead of _etext
  2020-04-23 11:00 [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Masami Hiramatsu
  2020-04-23 11:01 ` [PATCH 1/3] perf-probe: Fix to check blacklist address correctly Masami Hiramatsu
@ 2020-04-23 11:01 ` Masami Hiramatsu
  2020-04-23 14:01   ` Arnaldo Carvalho de Melo
  2020-04-23 11:01 ` [PATCH 3/3] perf-probe: Do not show the skipped events Masami Hiramatsu
  2020-05-06 16:48 ` [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Arnaldo Carvalho de Melo
  3 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu @ 2020-04-23 11:01 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Masami Hiramatsu, Jiri Olsa, Namhyung Kim,
	Linux Kernel Mailing List, stable

Since commit 03db8b583d1c ("perf tools: Fix maps__find_symbol_by_name()")
introduced map address range check in maps__find_symbol_by_name(),
we can not get "_etext" from kernel map because _etext is placed
on the edge of the kernel .text section (= kernel map in perf.)

To fix this issue, this checks the address correctness
by map address range information (map->start and map->end)
instead of using _etext address.

This can cause an error if the target inlined function is
embedded in both __init function and normal function.

For exaample, request_resource() is a normal function but also
embedded in __init reserve_setup(). In this case, the probe point
in reserve_setup() must be skipped. However, without this fix,
it failes to setup all probe points.
================
  # ./perf probe -v request_resource
  probe-definition(0): request_resource
  symbol:request_resource file:(null) line:0 offset:0 return:0 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (8 entries long)
  Using /usr/lib/debug/lib/modules/5.5.17-200.fc31.x86_64/vmlinux for symbols
  Open Debuginfo file: /usr/lib/debug/lib/modules/5.5.17-200.fc31.x86_64/vmlinux
  Try to find probe point from debuginfo.
  Matched function: request_resource [15e29ad]
  found inline addr: 0xffffffff82fbf892
  Probe point found: reserve_setup+204
  found inline addr: 0xffffffff810e9790
  Probe point found: request_resource+0
  Found 2 probe_trace_events.
  Opening /sys/kernel/debug/tracing//kprobe_events write=1
  Opening /sys/kernel/debug/tracing//README write=0
  Writing event: p:probe/request_resource _text+33290386
  Failed to write event: Invalid argument
    Error: Failed to add events. Reason: Invalid argument (Code: -22)
================

With this fix,

================
  # ./perf probe request_resource
  reserve_setup is out of .text, skip it.
  Added new events:
    (null):(null)        (on request_resource)
    probe:request_resource (on request_resource)

  You can now use it in all perf tools, such as:

  	perf record -e probe:request_resource -aR sleep 1

================

Fixes: 03db8b583d1c ("perf tools: Fix maps__find_symbol_by_name()")
Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 tools/perf/util/probe-event.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index f75df63309be..a5387e03e365 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -236,21 +236,22 @@ static void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
 static bool kprobe_blacklist__listed(unsigned long address);
 static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
 {
-	u64 etext_addr = 0;
-	int ret;
-
-	/* Get the address of _etext for checking non-probable text symbol */
-	ret = kernel_get_symbol_address_by_name("_etext", &etext_addr,
-						false, false);
+	struct map *map;
+	bool ret = false;
 
-	if (ret == 0 && etext_addr < address)
-		pr_warning("%s is out of .text, skip it.\n", symbol);
-	else if (kprobe_blacklist__listed(address))
+	map = kernel_get_module_map(NULL);
+	if (map) {
+		ret = address <= map->start || map->end < address;
+		if (ret)
+			pr_warning("%s is out of .text, skip it.\n", symbol);
+		map__put(map);
+	}
+	if (!ret && kprobe_blacklist__listed(address)) {
 		pr_warning("%s is blacklisted function, skip it.\n", symbol);
-	else
-		return false;
+		ret = true;
+	}
 
-	return true;
+	return ret;
 }
 
 /*


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] perf-probe: Do not show the skipped events
  2020-04-23 11:00 [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Masami Hiramatsu
  2020-04-23 11:01 ` [PATCH 1/3] perf-probe: Fix to check blacklist address correctly Masami Hiramatsu
  2020-04-23 11:01 ` [PATCH 2/3] perf-probe: Check address correctness by map instead of _etext Masami Hiramatsu
@ 2020-04-23 11:01 ` Masami Hiramatsu
  2020-04-23 14:01   ` Arnaldo Carvalho de Melo
  2020-05-06 16:48 ` [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Arnaldo Carvalho de Melo
  3 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu @ 2020-04-23 11:01 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Masami Hiramatsu, Jiri Olsa, Namhyung Kim,
	Linux Kernel Mailing List, stable

When a probe point is expanded to several places (like inlined) and
if some of them are skipped because of blacklisted or __init function,
those trace_events has no event name. It must be skipped while showing
results.

Without this fix, you can see "(null):(null)" on the list,
===========
  # ./perf probe request_resource
  reserve_setup is out of .text, skip it.
  Added new events:
    (null):(null)        (on request_resource)
    probe:request_resource (on request_resource)

  You can now use it in all perf tools, such as:

  	perf record -e probe:request_resource -aR sleep 1

===========

With this fix, it is ignored.
===========
  # ./perf probe request_resource
  reserve_setup is out of .text, skip it.
  Added new events:
    probe:request_resource (on request_resource)

  You can now use it in all perf tools, such as:

  	perf record -e probe:request_resource -aR sleep 1

===========

Fixes: 5a51fcd1f30c ("perf probe: Skip kernel symbols which is out of .text")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 tools/perf/builtin-probe.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 70548df2abb9..6b1507566770 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -364,6 +364,9 @@ static int perf_add_probe_events(struct perf_probe_event *pevs, int npevs)
 
 		for (k = 0; k < pev->ntevs; k++) {
 			struct probe_trace_event *tev = &pev->tevs[k];
+			/* Skipped events have no event name */
+			if (!tev->event)
+				continue;
 
 			/* We use tev's name for showing new events */
 			show_perf_probe_event(tev->group, tev->event, pev,


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] perf-probe: Check address correctness by map instead of _etext
  2020-04-23 11:01 ` [PATCH 2/3] perf-probe: Check address correctness by map instead of _etext Masami Hiramatsu
@ 2020-04-23 14:01   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-23 14:01 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List, stable

Em Thu, Apr 23, 2020 at 08:01:13PM +0900, Masami Hiramatsu escreveu:
> Since commit 03db8b583d1c ("perf tools: Fix maps__find_symbol_by_name()")
> introduced map address range check in maps__find_symbol_by_name(),
> we can not get "_etext" from kernel map because _etext is placed
> on the edge of the kernel .text section (= kernel map in perf.)
> 
> To fix this issue, this checks the address correctness
> by map address range information (map->start and map->end)
> instead of using _etext address.
> 
> This can cause an error if the target inlined function is
> embedded in both __init function and normal function.
> 
> For exaample, request_resource() is a normal function but also
> embedded in __init reserve_setup(). In this case, the probe point
> in reserve_setup() must be skipped. However, without this fix,
> it failes to setup all probe points.
> ================
>   # ./perf probe -v request_resource
>   probe-definition(0): request_resource
>   symbol:request_resource file:(null) line:0 offset:0 return:0 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (8 entries long)
>   Using /usr/lib/debug/lib/modules/5.5.17-200.fc31.x86_64/vmlinux for symbols
>   Open Debuginfo file: /usr/lib/debug/lib/modules/5.5.17-200.fc31.x86_64/vmlinux
>   Try to find probe point from debuginfo.
>   Matched function: request_resource [15e29ad]
>   found inline addr: 0xffffffff82fbf892
>   Probe point found: reserve_setup+204
>   found inline addr: 0xffffffff810e9790
>   Probe point found: request_resource+0
>   Found 2 probe_trace_events.
>   Opening /sys/kernel/debug/tracing//kprobe_events write=1
>   Opening /sys/kernel/debug/tracing//README write=0
>   Writing event: p:probe/request_resource _text+33290386
>   Failed to write event: Invalid argument
>     Error: Failed to add events. Reason: Invalid argument (Code: -22)
> ================
> 
> With this fix,
> 
> ================
>   # ./perf probe request_resource
>   reserve_setup is out of .text, skip it.
>   Added new events:
>     (null):(null)        (on request_resource)

But what is this (null):(null) probe? Confusing :-)

Thanks for working on this!

- Arnaldo

>     probe:request_resource (on request_resource)
> 
>   You can now use it in all perf tools, such as:
> 
>   	perf record -e probe:request_resource -aR sleep 1
> 
> ================
> 
> Fixes: 03db8b583d1c ("perf tools: Fix maps__find_symbol_by_name()")
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  tools/perf/util/probe-event.c |   25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index f75df63309be..a5387e03e365 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -236,21 +236,22 @@ static void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
>  static bool kprobe_blacklist__listed(unsigned long address);
>  static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>  {
> -	u64 etext_addr = 0;
> -	int ret;
> -
> -	/* Get the address of _etext for checking non-probable text symbol */
> -	ret = kernel_get_symbol_address_by_name("_etext", &etext_addr,
> -						false, false);
> +	struct map *map;
> +	bool ret = false;
>  
> -	if (ret == 0 && etext_addr < address)
> -		pr_warning("%s is out of .text, skip it.\n", symbol);
> -	else if (kprobe_blacklist__listed(address))
> +	map = kernel_get_module_map(NULL);
> +	if (map) {
> +		ret = address <= map->start || map->end < address;
> +		if (ret)
> +			pr_warning("%s is out of .text, skip it.\n", symbol);
> +		map__put(map);
> +	}
> +	if (!ret && kprobe_blacklist__listed(address)) {
>  		pr_warning("%s is blacklisted function, skip it.\n", symbol);
> -	else
> -		return false;
> +		ret = true;
> +	}
>  
> -	return true;
> +	return ret;
>  }
>  
>  /*
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] perf-probe: Do not show the skipped events
  2020-04-23 11:01 ` [PATCH 3/3] perf-probe: Do not show the skipped events Masami Hiramatsu
@ 2020-04-23 14:01   ` Arnaldo Carvalho de Melo
  2020-04-23 23:33     ` Masami Hiramatsu
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-23 14:01 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List, stable

Em Thu, Apr 23, 2020 at 08:01:22PM +0900, Masami Hiramatsu escreveu:
> When a probe point is expanded to several places (like inlined) and
> if some of them are skipped because of blacklisted or __init function,
> those trace_events has no event name. It must be skipped while showing
> results.
> 
> Without this fix, you can see "(null):(null)" on the list,
> ===========

Ok, you broke the patch in two, I think its better to combine both, ok?

- Arnaldo

>   # ./perf probe request_resource
>   reserve_setup is out of .text, skip it.
>   Added new events:
>     (null):(null)        (on request_resource)
>     probe:request_resource (on request_resource)
> 
>   You can now use it in all perf tools, such as:
> 
>   	perf record -e probe:request_resource -aR sleep 1
> 
> ===========
> 
> With this fix, it is ignored.
> ===========
>   # ./perf probe request_resource
>   reserve_setup is out of .text, skip it.
>   Added new events:
>     probe:request_resource (on request_resource)
> 
>   You can now use it in all perf tools, such as:
> 
>   	perf record -e probe:request_resource -aR sleep 1
> 
> ===========
> 
> Fixes: 5a51fcd1f30c ("perf probe: Skip kernel symbols which is out of .text")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  tools/perf/builtin-probe.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 70548df2abb9..6b1507566770 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -364,6 +364,9 @@ static int perf_add_probe_events(struct perf_probe_event *pevs, int npevs)
>  
>  		for (k = 0; k < pev->ntevs; k++) {
>  			struct probe_trace_event *tev = &pev->tevs[k];
> +			/* Skipped events have no event name */
> +			if (!tev->event)
> +				continue;
>  
>  			/* We use tev's name for showing new events */
>  			show_perf_probe_event(tev->group, tev->event, pev,
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] perf-probe: Do not show the skipped events
  2020-04-23 14:01   ` Arnaldo Carvalho de Melo
@ 2020-04-23 23:33     ` Masami Hiramatsu
  2020-04-24 13:08       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu @ 2020-04-23 23:33 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List, stable

On Thu, 23 Apr 2020 11:01:39 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Apr 23, 2020 at 08:01:22PM +0900, Masami Hiramatsu escreveu:
> > When a probe point is expanded to several places (like inlined) and
> > if some of them are skipped because of blacklisted or __init function,
> > those trace_events has no event name. It must be skipped while showing
> > results.
> > 
> > Without this fix, you can see "(null):(null)" on the list,
> > ===========
> 
> Ok, you broke the patch in two, I think its better to combine both, ok?

No, if an inlined function is embedded in blacklisted areas, it also
shows same "(null):(null)" without [2/3].

Reordering the patches is OK, but this is still an independent fix.

Thank you,

> 
> - Arnaldo
> 
> >   # ./perf probe request_resource
> >   reserve_setup is out of .text, skip it.
> >   Added new events:
> >     (null):(null)        (on request_resource)
> >     probe:request_resource (on request_resource)
> > 
> >   You can now use it in all perf tools, such as:
> > 
> >   	perf record -e probe:request_resource -aR sleep 1
> > 
> > ===========
> > 
> > With this fix, it is ignored.
> > ===========
> >   # ./perf probe request_resource
> >   reserve_setup is out of .text, skip it.
> >   Added new events:
> >     probe:request_resource (on request_resource)
> > 
> >   You can now use it in all perf tools, such as:
> > 
> >   	perf record -e probe:request_resource -aR sleep 1
> > 
> > ===========
> > 
> > Fixes: 5a51fcd1f30c ("perf probe: Skip kernel symbols which is out of .text")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  tools/perf/builtin-probe.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> > index 70548df2abb9..6b1507566770 100644
> > --- a/tools/perf/builtin-probe.c
> > +++ b/tools/perf/builtin-probe.c
> > @@ -364,6 +364,9 @@ static int perf_add_probe_events(struct perf_probe_event *pevs, int npevs)
> >  
> >  		for (k = 0; k < pev->ntevs; k++) {
> >  			struct probe_trace_event *tev = &pev->tevs[k];
> > +			/* Skipped events have no event name */
> > +			if (!tev->event)
> > +				continue;
> >  
> >  			/* We use tev's name for showing new events */
> >  			show_perf_probe_event(tev->group, tev->event, pev,
> > 
> 
> -- 
> 
> - Arnaldo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] perf-probe: Do not show the skipped events
  2020-04-23 23:33     ` Masami Hiramatsu
@ 2020-04-24 13:08       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-24 13:08 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List, stable

Em Fri, Apr 24, 2020 at 08:33:05AM +0900, Masami Hiramatsu escreveu:
> On Thu, 23 Apr 2020 11:01:39 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > Em Thu, Apr 23, 2020 at 08:01:22PM +0900, Masami Hiramatsu escreveu:
> > > When a probe point is expanded to several places (like inlined) and
> > > if some of them are skipped because of blacklisted or __init function,
> > > those trace_events has no event name. It must be skipped while showing
> > > results.
> > > 
> > > Without this fix, you can see "(null):(null)" on the list,
> > > ===========
> > 
> > Ok, you broke the patch in two, I think its better to combine both, ok?
> 
> No, if an inlined function is embedded in blacklisted areas, it also
> shows same "(null):(null)" without [2/3].
> 
> Reordering the patches is OK, but this is still an independent fix.

Ok, so I'll try reordering, so that we don't see it in the cset log for
the other fix.

Thanks for the clarification,

- Arnaldo
 
> Thank you,
> 
> > 
> > - Arnaldo
> > 
> > >   # ./perf probe request_resource
> > >   reserve_setup is out of .text, skip it.
> > >   Added new events:
> > >     (null):(null)        (on request_resource)
> > >     probe:request_resource (on request_resource)
> > > 
> > >   You can now use it in all perf tools, such as:
> > > 
> > >   	perf record -e probe:request_resource -aR sleep 1
> > > 
> > > ===========
> > > 
> > > With this fix, it is ignored.
> > > ===========
> > >   # ./perf probe request_resource
> > >   reserve_setup is out of .text, skip it.
> > >   Added new events:
> > >     probe:request_resource (on request_resource)
> > > 
> > >   You can now use it in all perf tools, such as:
> > > 
> > >   	perf record -e probe:request_resource -aR sleep 1
> > > 
> > > ===========
> > > 
> > > Fixes: 5a51fcd1f30c ("perf probe: Skip kernel symbols which is out of .text")
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  tools/perf/builtin-probe.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> > > index 70548df2abb9..6b1507566770 100644
> > > --- a/tools/perf/builtin-probe.c
> > > +++ b/tools/perf/builtin-probe.c
> > > @@ -364,6 +364,9 @@ static int perf_add_probe_events(struct perf_probe_event *pevs, int npevs)
> > >  
> > >  		for (k = 0; k < pev->ntevs; k++) {
> > >  			struct probe_trace_event *tev = &pev->tevs[k];
> > > +			/* Skipped events have no event name */
> > > +			if (!tev->event)
> > > +				continue;
> > >  
> > >  			/* We use tev's name for showing new events */
> > >  			show_perf_probe_event(tev->group, tev->event, pev,
> > > 
> > 
> > -- 
> > 
> > - Arnaldo
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] perf-probe: Fix to check blacklist address correctly
  2020-04-23 11:01 ` [PATCH 1/3] perf-probe: Fix to check blacklist address correctly Masami Hiramatsu
@ 2020-05-06 16:44   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-06 16:44 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List, stable

Em Thu, Apr 23, 2020 at 08:01:04PM +0900, Masami Hiramatsu escreveu:
> Fix to check kprobe blacklist address correctly with
> relocated address by adjusting debuginfo address.
> 
> Since the address in the debuginfo is same as objdump,
> it is different from relocated kernel address with KASLR.
> Thus, the perf-probe always misses to catch the
> blacklisted addresses.

Thanks, applied, sorry for the delay,

- Arnaldo
 
> Without this patch, perf probe can not detect the blacklist
> addresses on KASLR enabled kernel.
> 
> =========
>   # perf probe kprobe_dispatcher
>   Failed to write event: Invalid argument
>     Error: Failed to add events.
> =========
> 
> With this patch, it correctly shows the error message.
> 
> =========
>   # perf probe kprobe_dispatcher
>   kprobe_dispatcher is blacklisted function, skip it.
>   Probe point 'kprobe_dispatcher' not found.
>     Error: Failed to add events.
> =========
> 
> Fixes: 9aaf5a5f479b ("perf probe: Check kprobes blacklist when adding new events")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  tools/perf/util/probe-event.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index eea132f512b0..f75df63309be 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -102,7 +102,7 @@ void exit_probe_symbol_maps(void)
>  	symbol__exit();
>  }
>  
> -static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
> +static struct ref_reloc_sym *kernel_get_ref_reloc_sym(struct map **pmap)
>  {
>  	/* kmap->ref_reloc_sym should be set if host_machine is initialized */
>  	struct kmap *kmap;
> @@ -114,6 +114,10 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
>  	kmap = map__kmap(map);
>  	if (!kmap)
>  		return NULL;
> +
> +	if (pmap)
> +		*pmap = map;
> +
>  	return kmap->ref_reloc_sym;
>  }
>  
> @@ -125,7 +129,7 @@ static int kernel_get_symbol_address_by_name(const char *name, u64 *addr,
>  	struct map *map;
>  
>  	/* ref_reloc_sym is just a label. Need a special fix*/
> -	reloc_sym = kernel_get_ref_reloc_sym();
> +	reloc_sym = kernel_get_ref_reloc_sym(NULL);
>  	if (reloc_sym && strcmp(name, reloc_sym->name) == 0)
>  		*addr = (reloc) ? reloc_sym->addr : reloc_sym->unrelocated_addr;
>  	else {
> @@ -745,6 +749,7 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  				       int ntevs)
>  {
>  	struct ref_reloc_sym *reloc_sym;
> +	struct map *map;
>  	char *tmp;
>  	int i, skipped = 0;
>  
> @@ -753,7 +758,7 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  		return post_process_offline_probe_trace_events(tevs, ntevs,
>  						symbol_conf.vmlinux_name);
>  
> -	reloc_sym = kernel_get_ref_reloc_sym();
> +	reloc_sym = kernel_get_ref_reloc_sym(&map);
>  	if (!reloc_sym) {
>  		pr_warning("Relocated base symbol is not found!\n");
>  		return -EINVAL;
> @@ -764,9 +769,13 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  			continue;
>  		if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
>  			continue;
> -		/* If we found a wrong one, mark it by NULL symbol */
> +		/*
> +		 * If we found a wrong one, mark it by NULL symbol.
> +		 * Since addresses in debuginfo is same as objdump, we need
> +		 * to convert it to addresses on memory.
> +		 */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
> -					  tevs[i].point.address)) {
> +			map__objdump_2mem(map, tevs[i].point.address))) {
>  			tmp = NULL;
>  			skipped++;
>  		} else {
> @@ -2936,7 +2945,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 && !pev->target &&
>  			(!pp->retprobe || kretprobe_offset_is_supported())) {
> -		reloc_sym = kernel_get_ref_reloc_sym();
> +		reloc_sym = kernel_get_ref_reloc_sym(NULL);
>  		if (!reloc_sym) {
>  			pr_warning("Relocated base symbol is not found!\n");
>  			ret = -EINVAL;
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/3] perf-probe: Fix __init function and blacklist checking
  2020-04-23 11:00 [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Masami Hiramatsu
                   ` (2 preceding siblings ...)
  2020-04-23 11:01 ` [PATCH 3/3] perf-probe: Do not show the skipped events Masami Hiramatsu
@ 2020-05-06 16:48 ` Arnaldo Carvalho de Melo
  3 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-06 16:48 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List, stable

Em Thu, Apr 23, 2020 at 08:00:54PM +0900, Masami Hiramatsu escreveu:
> Hi,
> 
> Here is a series of fixes related to __init function and
> blacklist checking routines. Arnaldo noticed me some cases
> which don't check the __init function checking. I found that
> the blacklist checking is also not working with KASLR, and
> also skipped probes are shown in result list unexpectedly.

thanks, tested and applied.

- Arnaldo
 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       perf-probe: Fix to check blacklist address correctly
>       perf-probe: Check address correctness by map instead of _etext
>       perf-probe: Do not show the skipped events
> 
> 
>  tools/perf/builtin-probe.c    |    3 +++
>  tools/perf/util/probe-event.c |   46 +++++++++++++++++++++++++----------------
>  2 files changed, 31 insertions(+), 18 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-06 16:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 11:00 [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Masami Hiramatsu
2020-04-23 11:01 ` [PATCH 1/3] perf-probe: Fix to check blacklist address correctly Masami Hiramatsu
2020-05-06 16:44   ` Arnaldo Carvalho de Melo
2020-04-23 11:01 ` [PATCH 2/3] perf-probe: Check address correctness by map instead of _etext Masami Hiramatsu
2020-04-23 14:01   ` Arnaldo Carvalho de Melo
2020-04-23 11:01 ` [PATCH 3/3] perf-probe: Do not show the skipped events Masami Hiramatsu
2020-04-23 14:01   ` Arnaldo Carvalho de Melo
2020-04-23 23:33     ` Masami Hiramatsu
2020-04-24 13:08       ` Arnaldo Carvalho de Melo
2020-05-06 16:48 ` [PATCH 0/3] perf-probe: Fix __init function and blacklist checking Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).