[PATCH] perf probe: Fix segfault when glob matching function without debuginfo

[PATCH] perf probe: Fix segfault when glob matching function without debuginfo

[Wang Nan]

Commit 4c859351226c920b227fec040a3b447f0d482af3 ("perf probe: Support
glob wildcards for function name") introduces segfault problems when
debuginfo is not available:

 # perf probe 'sys_w*'
  Added new events:
  Segmentation fault

The first problem resides in find_probe_trace_events_from_map(). In
that function, find_probe_functions() is called to match each symbol
against glob to find the number of matching functions, but still use
map__for_each_symbol_by_name() to find 'struct symbol' for matching
functions. Unfortunately, map__for_each_symbol_by_name() does
exact matching by searching in an rbtree. It doesn't know glob
matching, and not easy for it to support it because it use rbtree based
binary search, but we are unable to ensure all names matched by the
glob (any glob passed by user) reside in one subtree.

This patch drops map__for_each_symbol_by_name(). Since there is no
rbtree again, re-matching all symbols costs a lot. This patch avoid it
by saving all matching results into an array (syms).

The second problem is the lost of tp->realname. In
__add_probe_trace_events(), if pev->point.function is glob, the event
name should be set to tev->point.realname. This patch ensures its
existence by strdup sym->name instead of leaving a NULL pointer there.

After this patch:

 # perf probe 'sys_w*'
 Added new events:
   probe:sys_waitid     (on sys_w*)
   probe:sys_wait4      (on sys_w*)
   probe:sys_waitpid    (on sys_w*)
   probe:sys_write      (on sys_w*)
   probe:sys_writev     (on sys_w*)

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

         perf record -e probe:sys_writev -aR sleep 1

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/util/probe-event.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 9052e7b..4f1bc78 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2493,7 +2493,8 @@ close_out:
 	return ret;
 }
 
-static int find_probe_functions(struct map *map, char *name)
+static int find_probe_functions(struct map *map, char *name,
+				struct symbol **syms)
 {
 	int found = 0;
 	struct symbol *sym;
@@ -2503,8 +2504,11 @@ static int find_probe_functions(struct map *map, char *name)
 		return 0;
 
 	map__for_each_symbol(map, sym, tmp) {
-		if (strglobmatch(sym->name, name))
+		if (strglobmatch(sym->name, name)) {
 			found++;
+			if (syms && found < probe_conf.max_probes)
+				syms[found - 1] = sym;
+		}
 	}
 
 	return found;
@@ -2527,11 +2531,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	struct map *map = NULL;
 	struct ref_reloc_sym *reloc_sym = NULL;
 	struct symbol *sym;
+	struct symbol **syms = NULL;
 	struct probe_trace_event *tev;
 	struct perf_probe_point *pp = &pev->point;
 	struct probe_trace_point *tp;
 	int num_matched_functions;
-	int ret, i;
+	int ret, i, j;
 
 	map = get_target_map(pev->target, pev->uprobes);
 	if (!map) {
@@ -2539,11 +2544,17 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		goto out;
 	}
 
+	syms = malloc(sizeof(struct symbol *) * probe_conf.max_probes);
+	if (!syms) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/*
 	 * Load matched symbols: Since the different local symbols may have
 	 * same name but different addresses, this lists all the symbols.
 	 */
-	num_matched_functions = find_probe_functions(map, pp->function);
+	num_matched_functions = find_probe_functions(map, pp->function, syms);
 	if (num_matched_functions == 0) {
 		pr_err("Failed to find symbol %s in %s\n", pp->function,
 			pev->target ? : "kernel");
@@ -2574,7 +2585,9 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 
 	ret = 0;
 
-	map__for_each_symbol_by_name(map, pp->function, sym) {
+	for (j = 0; j < num_matched_functions; j++) {
+		sym = syms[j];
+
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {
@@ -2598,6 +2611,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 			tp->symbol = strdup_or_goto(sym->name, nomem_out);
 			tp->offset = pp->offset;
 		}
+		tp->realname = strdup_or_goto(sym->name, nomem_out);
+
 		tp->retprobe = pp->retprobe;
 		if (pev->target)
 			tev->point.module = strdup_or_goto(pev->target,
@@ -2628,6 +2643,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 
 out:
 	put_target_map(map, pev->uprobes);
+	free(syms);
 	return ret;
 
 nomem_out:
-- 
1.8.3.4

Re: [PATCH] perf probe: Fix segfault when glob matching function without debuginfo

[Arnaldo Carvalho de Melo]

Em Fri, May 29, 2015 at 09:45:47AM +0000, Wang Nan escreveu:
> Commit 4c859351226c920b227fec040a3b447f0d482af3 ("perf probe: Support
> glob wildcards for function name") introduces segfault problems when
> debuginfo is not available:
> 
>  # perf probe 'sys_w*'
>   Added new events:
>   Segmentation fault
> 
> The first problem resides in find_probe_trace_events_from_map(). In
> that function, find_probe_functions() is called to match each symbol
> against glob to find the number of matching functions, but still use
> map__for_each_symbol_by_name() to find 'struct symbol' for matching
> functions. Unfortunately, map__for_each_symbol_by_name() does
> exact matching by searching in an rbtree. It doesn't know glob
> matching, and not easy for it to support it because it use rbtree based
> binary search, but we are unable to ensure all names matched by the
> glob (any glob passed by user) reside in one subtree.
> 
> This patch drops map__for_each_symbol_by_name(). Since there is no
> rbtree again, re-matching all symbols costs a lot. This patch avoid it
> by saving all matching results into an array (syms).
> 
> The second problem is the lost of tp->realname. In
> __add_probe_trace_events(), if pev->point.function is glob, the event
> name should be set to tev->point.realname. This patch ensures its
> existence by strdup sym->name instead of leaving a NULL pointer there.
> 
> After this patch:
> 
>  # perf probe 'sys_w*'
>  Added new events:
>    probe:sys_waitid     (on sys_w*)

The second part looks strange :-\ Masami?

>    probe:sys_wait4      (on sys_w*)
>    probe:sys_waitpid    (on sys_w*)
>    probe:sys_write      (on sys_w*)
>    probe:sys_writev     (on sys_w*)
> 
>  You can now use it in all perf tools, such as:
> 
>          perf record -e probe:sys_writev -aR sleep 1
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/util/probe-event.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 9052e7b..4f1bc78 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2493,7 +2493,8 @@ close_out:
>  	return ret;
>  }
>  
> -static int find_probe_functions(struct map *map, char *name)
> +static int find_probe_functions(struct map *map, char *name,
> +				struct symbol **syms)
>  {
>  	int found = 0;
>  	struct symbol *sym;
> @@ -2503,8 +2504,11 @@ static int find_probe_functions(struct map *map, char *name)
>  		return 0;
>  
>  	map__for_each_symbol(map, sym, tmp) {
> -		if (strglobmatch(sym->name, name))
> +		if (strglobmatch(sym->name, name)) {
>  			found++;
> +			if (syms && found < probe_conf.max_probes)
> +				syms[found - 1] = sym;
> +		}
>  	}
>  
>  	return found;
> @@ -2527,11 +2531,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	struct map *map = NULL;
>  	struct ref_reloc_sym *reloc_sym = NULL;
>  	struct symbol *sym;
> +	struct symbol **syms = NULL;
>  	struct probe_trace_event *tev;
>  	struct perf_probe_point *pp = &pev->point;
>  	struct probe_trace_point *tp;
>  	int num_matched_functions;
> -	int ret, i;
> +	int ret, i, j;
>  
>  	map = get_target_map(pev->target, pev->uprobes);
>  	if (!map) {
> @@ -2539,11 +2544,17 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  		goto out;
>  	}
>  
> +	syms = malloc(sizeof(struct symbol *) * probe_conf.max_probes);
> +	if (!syms) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	/*
>  	 * Load matched symbols: Since the different local symbols may have
>  	 * same name but different addresses, this lists all the symbols.
>  	 */
> -	num_matched_functions = find_probe_functions(map, pp->function);
> +	num_matched_functions = find_probe_functions(map, pp->function, syms);
>  	if (num_matched_functions == 0) {
>  		pr_err("Failed to find symbol %s in %s\n", pp->function,
>  			pev->target ? : "kernel");
> @@ -2574,7 +2585,9 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  
>  	ret = 0;
>  
> -	map__for_each_symbol_by_name(map, pp->function, sym) {
> +	for (j = 0; j < num_matched_functions; j++) {
> +		sym = syms[j];
> +
>  		tev = (*tevs) + ret;
>  		tp = &tev->point;
>  		if (ret == num_matched_functions) {
> @@ -2598,6 +2611,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  			tp->symbol = strdup_or_goto(sym->name, nomem_out);
>  			tp->offset = pp->offset;
>  		}
> +		tp->realname = strdup_or_goto(sym->name, nomem_out);
> +
>  		tp->retprobe = pp->retprobe;
>  		if (pev->target)
>  			tev->point.module = strdup_or_goto(pev->target,
> @@ -2628,6 +2643,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  
>  out:
>  	put_target_map(map, pev->uprobes);
> +	free(syms);
>  	return ret;
>  
>  nomem_out:
> -- 
> 1.8.3.4

Re: [PATCH] perf probe: Fix segfault when glob matching function without debuginfo

[Masami Hiramatsu]

On 2015/05/29 18:45, Wang Nan wrote:
> Commit 4c859351226c920b227fec040a3b447f0d482af3 ("perf probe: Support
> glob wildcards for function name") introduces segfault problems when
> debuginfo is not available:
> 
>  # perf probe 'sys_w*'
>   Added new events:
>   Segmentation fault
> 
> The first problem resides in find_probe_trace_events_from_map(). In
> that function, find_probe_functions() is called to match each symbol
> against glob to find the number of matching functions, but still use
> map__for_each_symbol_by_name() to find 'struct symbol' for matching
> functions. Unfortunately, map__for_each_symbol_by_name() does
> exact matching by searching in an rbtree. It doesn't know glob
> matching, and not easy for it to support it because it use rbtree based
> binary search, but we are unable to ensure all names matched by the
> glob (any glob passed by user) reside in one subtree.
> 
> This patch drops map__for_each_symbol_by_name(). Since there is no
> rbtree again, re-matching all symbols costs a lot. This patch avoid it
> by saving all matching results into an array (syms).
> 
> The second problem is the lost of tp->realname. In
> __add_probe_trace_events(), if pev->point.function is glob, the event
> name should be set to tev->point.realname. This patch ensures its
> existence by strdup sym->name instead of leaving a NULL pointer there.

Good catch!

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thank you!

> 
> After this patch:
> 
>  # perf probe 'sys_w*'
>  Added new events:
>    probe:sys_waitid     (on sys_w*)
>    probe:sys_wait4      (on sys_w*)
>    probe:sys_waitpid    (on sys_w*)
>    probe:sys_write      (on sys_w*)
>    probe:sys_writev     (on sys_w*)
> 
>  You can now use it in all perf tools, such as:
> 
>          perf record -e probe:sys_writev -aR sleep 1
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/util/probe-event.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 9052e7b..4f1bc78 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2493,7 +2493,8 @@ close_out:
>  	return ret;
>  }
>  
> -static int find_probe_functions(struct map *map, char *name)
> +static int find_probe_functions(struct map *map, char *name,
> +				struct symbol **syms)
>  {
>  	int found = 0;
>  	struct symbol *sym;
> @@ -2503,8 +2504,11 @@ static int find_probe_functions(struct map *map, char *name)
>  		return 0;
>  
>  	map__for_each_symbol(map, sym, tmp) {
> -		if (strglobmatch(sym->name, name))
> +		if (strglobmatch(sym->name, name)) {
>  			found++;
> +			if (syms && found < probe_conf.max_probes)
> +				syms[found - 1] = sym;
> +		}
>  	}
>  
>  	return found;
> @@ -2527,11 +2531,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	struct map *map = NULL;
>  	struct ref_reloc_sym *reloc_sym = NULL;
>  	struct symbol *sym;
> +	struct symbol **syms = NULL;
>  	struct probe_trace_event *tev;
>  	struct perf_probe_point *pp = &pev->point;
>  	struct probe_trace_point *tp;
>  	int num_matched_functions;
> -	int ret, i;
> +	int ret, i, j;
>  
>  	map = get_target_map(pev->target, pev->uprobes);
>  	if (!map) {
> @@ -2539,11 +2544,17 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  		goto out;
>  	}
>  
> +	syms = malloc(sizeof(struct symbol *) * probe_conf.max_probes);
> +	if (!syms) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	/*
>  	 * Load matched symbols: Since the different local symbols may have
>  	 * same name but different addresses, this lists all the symbols.
>  	 */
> -	num_matched_functions = find_probe_functions(map, pp->function);
> +	num_matched_functions = find_probe_functions(map, pp->function, syms);
>  	if (num_matched_functions == 0) {
>  		pr_err("Failed to find symbol %s in %s\n", pp->function,
>  			pev->target ? : "kernel");
> @@ -2574,7 +2585,9 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  
>  	ret = 0;
>  
> -	map__for_each_symbol_by_name(map, pp->function, sym) {
> +	for (j = 0; j < num_matched_functions; j++) {
> +		sym = syms[j];
> +
>  		tev = (*tevs) + ret;
>  		tp = &tev->point;
>  		if (ret == num_matched_functions) {
> @@ -2598,6 +2611,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  			tp->symbol = strdup_or_goto(sym->name, nomem_out);
>  			tp->offset = pp->offset;
>  		}
> +		tp->realname = strdup_or_goto(sym->name, nomem_out);
> +
>  		tp->retprobe = pp->retprobe;
>  		if (pev->target)
>  			tev->point.module = strdup_or_goto(pev->target,
> @@ -2628,6 +2643,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  
>  out:
>  	put_target_map(map, pev->uprobes);
> +	free(syms);
>  	return ret;
>  
>  nomem_out:
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

[tip:perf/core] perf probe: Fix segfault when glob matching function without debuginfo

[tip-bot for Wang Nan]

Commit-ID:  6bb536cc4b06df70012cec6168cc1788e200bc9d
Gitweb:     http://git.kernel.org/tip/6bb536cc4b06df70012cec6168cc1788e200bc9d
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Fri, 29 May 2015 09:45:47 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Sat, 30 May 2015 11:08:52 -0300

perf probe: Fix segfault when glob matching function without debuginfo

Commit 4c859351226c920b227fec040a3b447f0d482af3 ("perf probe: Support
glob wildcards for function name") introduces segfault problems when
debuginfo is not available:

 # perf probe 'sys_w*'
  Added new events:
  Segmentation fault

The first problem resides in find_probe_trace_events_from_map(). In
that function, find_probe_functions() is called to match each symbol
against glob to find the number of matching functions, but still use
map__for_each_symbol_by_name() to find 'struct symbol' for matching
functions. Unfortunately, map__for_each_symbol_by_name() does
exact matching by searching in an rbtree.

It doesn't know glob matching, and not easy for it to support it because
it use rbtree based binary search, but we are unable to ensure all names
matched by the glob (any glob passed by user) reside in one subtree.

This patch drops map__for_each_symbol_by_name(). Since there is no
rbtree again, re-matching all symbols costs a lot. This patch avoid it
by saving all matching results into an array (syms).

The second problem is the lost of tp->realname. In
__add_probe_trace_events(), if pev->point.function is glob, the event
name should be set to tev->point.realname. This patch ensures its
existence by strdup sym->name instead of leaving a NULL pointer there.

After this patch:

 # perf probe 'sys_w*'
 Added new events:
   probe:sys_waitid     (on sys_w*)
   probe:sys_wait4      (on sys_w*)
   probe:sys_waitpid    (on sys_w*)
   probe:sys_write      (on sys_w*)
   probe:sys_writev     (on sys_w*)

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

         perf record -e probe:sys_writev -aR sleep 1

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Link: http://lkml.kernel.org/r/1432892747-232506-1-git-send-email-wangnan0@huawei.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index d27edef..e6f215b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2494,7 +2494,8 @@ close_out:
 	return ret;
 }
 
-static int find_probe_functions(struct map *map, char *name)
+static int find_probe_functions(struct map *map, char *name,
+				struct symbol **syms)
 {
 	int found = 0;
 	struct symbol *sym;
@@ -2504,8 +2505,11 @@ static int find_probe_functions(struct map *map, char *name)
 		return 0;
 
 	map__for_each_symbol(map, sym, tmp) {
-		if (strglobmatch(sym->name, name))
+		if (strglobmatch(sym->name, name)) {
 			found++;
+			if (syms && found < probe_conf.max_probes)
+				syms[found - 1] = sym;
+		}
 	}
 
 	return found;
@@ -2528,11 +2532,12 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	struct map *map = NULL;
 	struct ref_reloc_sym *reloc_sym = NULL;
 	struct symbol *sym;
+	struct symbol **syms = NULL;
 	struct probe_trace_event *tev;
 	struct perf_probe_point *pp = &pev->point;
 	struct probe_trace_point *tp;
 	int num_matched_functions;
-	int ret, i;
+	int ret, i, j;
 
 	map = get_target_map(pev->target, pev->uprobes);
 	if (!map) {
@@ -2540,11 +2545,17 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		goto out;
 	}
 
+	syms = malloc(sizeof(struct symbol *) * probe_conf.max_probes);
+	if (!syms) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/*
 	 * Load matched symbols: Since the different local symbols may have
 	 * same name but different addresses, this lists all the symbols.
 	 */
-	num_matched_functions = find_probe_functions(map, pp->function);
+	num_matched_functions = find_probe_functions(map, pp->function, syms);
 	if (num_matched_functions == 0) {
 		pr_err("Failed to find symbol %s in %s\n", pp->function,
 			pev->target ? : "kernel");
@@ -2575,7 +2586,9 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 
 	ret = 0;
 
-	map__for_each_symbol_by_name(map, pp->function, sym) {
+	for (j = 0; j < num_matched_functions; j++) {
+		sym = syms[j];
+
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {
@@ -2599,6 +2612,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 			tp->symbol = strdup_or_goto(sym->name, nomem_out);
 			tp->offset = pp->offset;
 		}
+		tp->realname = strdup_or_goto(sym->name, nomem_out);
+
 		tp->retprobe = pp->retprobe;
 		if (pev->target)
 			tev->point.module = strdup_or_goto(pev->target,
@@ -2629,6 +2644,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 
 out:
 	put_target_map(map, pev->uprobes);
+	free(syms);
 	return ret;
 
 nomem_out: