[PATCH 0/2] perf tools: Two bugfixs related to perf probe

[PATCH 0/2] perf tools: Two bugfixs related to perf probe

[Wang Nan]

I found two bugs in perf probe related code. This two patches
fix them.

Wang Nan (2):
  perf probe: Only call probe_file__get_events() when fd is valid
  perf tools: Fix find_perf_probe_point_from_map() which incorrectly
    returns success

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

-- 
1.8.3.4

[PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid

[Wang Nan]

In system with kprobe enabled but uprobe turned off, 'perf probe -d'
causes segfault because it calls probe_file__get_events() with a
negative fd (when deleting uprobe events).

This patch validates fds before calling probe_file__get_events().

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-probe.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 132afc9..861aa89 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -384,7 +384,11 @@ static int perf_del_probe_events(struct strfilter *filter)
 		goto out;
 	}
 
-	ret = probe_file__get_events(kfd, filter, klist);
+	if (kfd < 0)
+		ret = -ENOENT;
+	else
+		ret = probe_file__get_events(kfd, filter, klist);
+
 	if (ret == 0) {
 		strlist__for_each(ent, klist)
 			pr_info("Removed event: %s\n", ent->s);
@@ -394,7 +398,11 @@ static int perf_del_probe_events(struct strfilter *filter)
 			goto error;
 	}
 
-	ret2 = probe_file__get_events(ufd, filter, ulist);
+	if (ufd < 0)
+		ret2 = -ENOENT;
+	else
+		ret2 = probe_file__get_events(ufd, filter, ulist);
+
 	if (ret2 == 0) {
 		strlist__for_each(ent, ulist)
 			pr_info("Removed event: %s\n", ent->s);
-- 
1.8.3.4

[PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[Wang Nan]

It is possible that find_perf_probe_point_from_map() fails to find
symbol but still returns 0 because of an small error when coding:
find_perf_probe_point_from_map() set 'ret' to error code at first,
but also use it to hold return value of
kernel_get_symbol_address_by_name().

This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
success, so if !sym, the whole function returns error correctly.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b51a8bf..e659c4f 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1905,6 +1905,9 @@ static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
 			sym = __find_kernel_function(addr, &map);
 		}
 	}
+
+	/* ret may has be overwritten so reset it */
+	ret = -ENOENT;
 	if (!sym)
 		goto out;
 
-- 
1.8.3.4

RE: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1740 bytes --]

From: Wang Nan [mailto:wangnan0@huawei.com]
>
>It is possible that find_perf_probe_point_from_map() fails to find
>symbol but still returns 0 because of an small error when coding:
>find_perf_probe_point_from_map() set 'ret' to error code at first,
>but also use it to hold return value of
>kernel_get_symbol_address_by_name().

OK, I didn't expect that there is a symbol which can be found by 
kernel_get_symbol_address_by_name() but not by __find_kernel_function()...
Would you have any example of the error?

>
>This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
>success, so if !sym, the whole function returns error correctly.

Hmm, that sounds tricky. I'd rather like to add *psym to kernel_get_symbol_address_by_name() 
to save symbol and don't use __find_kernel_function() instead.

Thank you,

>
>Signed-off-by: Wang Nan <wangnan0@huawei.com>
>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>Cc: Jiri Olsa <jolsa@kernel.org>
>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>Cc: Namhyung Kim <namhyung@kernel.org>
>---
> tools/perf/util/probe-event.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>index b51a8bf..e659c4f 100644
>--- a/tools/perf/util/probe-event.c
>+++ b/tools/perf/util/probe-event.c
>@@ -1905,6 +1905,9 @@ static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
> 			sym = __find_kernel_function(addr, &map);
> 		}
> 	}
>+
>+	/* ret may has be overwritten so reset it */
>+	ret = -ENOENT;
> 	if (!sym)
> 		goto out;
>
>--
>1.8.3.4

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1971 bytes --]

From: Wang Nan [mailto:wangnan0@huawei.com]
>
>In system with kprobe enabled but uprobe turned off, 'perf probe -d'
>causes segfault because it calls probe_file__get_events() with a
>negative fd (when deleting uprobe events).

Hmm, OK. This may happen if user runs perf probe on the kernel
which only enables either CONFIG_KPROBE_EVENTS or CONFIG_UPROBE_EVENTS.

>
>This patch validates fds before calling probe_file__get_events().

Hmm, could you improve probe_file__get_events() to check the fd instead
of checking it at call-site? I think that is more generic fixup.

Thank you,

>
>Signed-off-by: Wang Nan <wangnan0@huawei.com>
>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>Cc: Jiri Olsa <jolsa@kernel.org>
>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>Cc: Namhyung Kim <namhyung@kernel.org>
>---
> tools/perf/builtin-probe.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
>index 132afc9..861aa89 100644
>--- a/tools/perf/builtin-probe.c
>+++ b/tools/perf/builtin-probe.c
>@@ -384,7 +384,11 @@ static int perf_del_probe_events(struct strfilter *filter)
> 		goto out;
> 	}
>
>-	ret = probe_file__get_events(kfd, filter, klist);
>+	if (kfd < 0)
>+		ret = -ENOENT;
>+	else
>+		ret = probe_file__get_events(kfd, filter, klist);
>+
> 	if (ret == 0) {
> 		strlist__for_each(ent, klist)
> 			pr_info("Removed event: %s\n", ent->s);
>@@ -394,7 +398,11 @@ static int perf_del_probe_events(struct strfilter *filter)
> 			goto error;
> 	}
>
>-	ret2 = probe_file__get_events(ufd, filter, ulist);
>+	if (ufd < 0)
>+		ret2 = -ENOENT;
>+	else
>+		ret2 = probe_file__get_events(ufd, filter, ulist);
>+
> 	if (ret2 == 0) {
> 		strlist__for_each(ent, ulist)
> 			pr_info("Removed event: %s\n", ent->s);
>--
>1.8.3.4

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid

[Arnaldo Carvalho de Melo]

Em Thu, Nov 05, 2015 at 01:19:24PM +0000, Wang Nan escreveu:
> In system with kprobe enabled but uprobe turned off, 'perf probe -d'
> causes segfault because it calls probe_file__get_events() with a
> negative fd (when deleting uprobe events).
> 
> This patch validates fds before calling probe_file__get_events().

Wouldn't this shorter patch be more robust by deferring the validation
to just before using the 'fd' value?

The end result is that probe_file__get_events() will return -ENOENT in
both calls, so ret and ret2 will both be set to -ENOENT, as in your
patch.

Masami?

- Arnaldo

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 89dbeb92c68e..f04a8318a1a7 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
 	char *p;
 	struct strlist *sl;
 
+	if (fd < 0)
+		return NULL;
+
 	sl = strlist__new(NULL, NULL);
 
 	fp = fdopen(dup(fd), "r");


diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 89dbeb92c68e..e5dc8e62f0f1 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -169,6 +169,9 @@ static struct strlist *__probe_file__get_namelist(int fd, bool include_group)
 	struct probe_trace_event tev;
 	int ret = 0;
 
+	if (fd < 0)
+		return NULL;
+
 	memset(&tev, 0, sizeof(tev));
 	rawlist = probe_file__get_rawlist(fd);
 	if (!rawlist)

 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-probe.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 132afc9..861aa89 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -384,7 +384,11 @@ static int perf_del_probe_events(struct strfilter *filter)
>  		goto out;
>  	}
>  
> -	ret = probe_file__get_events(kfd, filter, klist);
> +	if (kfd < 0)
> +		ret = -ENOENT;
> +	else
> +		ret = probe_file__get_events(kfd, filter, klist);
> +
>  	if (ret == 0) {
>  		strlist__for_each(ent, klist)
>  			pr_info("Removed event: %s\n", ent->s);
> @@ -394,7 +398,11 @@ static int perf_del_probe_events(struct strfilter *filter)
>  			goto error;
>  	}
>  
> -	ret2 = probe_file__get_events(ufd, filter, ulist);
> +	if (ufd < 0)
> +		ret2 = -ENOENT;
> +	else
> +		ret2 = probe_file__get_events(ufd, filter, ulist);
> +
>  	if (ret2 == 0) {
>  		strlist__for_each(ent, ulist)
>  			pr_info("Removed event: %s\n", ent->s);
> -- 
> 1.8.3.4

RE: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid

[平松雅巳 / HIRAMATU，MASAMI]

From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>Em Thu, Nov 05, 2015 at 01:19:24PM +0000, Wang Nan escreveu:
>> In system with kprobe enabled but uprobe turned off, 'perf probe -d'
>> causes segfault because it calls probe_file__get_events() with a
>> negative fd (when deleting uprobe events).
>>
>> This patch validates fds before calling probe_file__get_events().
>
>Wouldn't this shorter patch be more robust by deferring the validation
>to just before using the 'fd' value?
>
>The end result is that probe_file__get_events() will return -ENOENT in
>both calls, so ret and ret2 will both be set to -ENOENT, as in your
>patch.
>
>Masami?

Yes, I've suggested so :)
https://lkml.org/lkml/2015/11/5/353

Thanks!



>
>- Arnaldo
>
>diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>index 89dbeb92c68e..f04a8318a1a7 100644
>--- a/tools/perf/util/probe-file.c
>+++ b/tools/perf/util/probe-file.c
>@@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
> 	char *p;
> 	struct strlist *sl;
>
>+	if (fd < 0)
>+		return NULL;
>+
> 	sl = strlist__new(NULL, NULL);
>
> 	fp = fdopen(dup(fd), "r");
>
>
>diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>index 89dbeb92c68e..e5dc8e62f0f1 100644
>--- a/tools/perf/util/probe-file.c
>+++ b/tools/perf/util/probe-file.c
>@@ -169,6 +169,9 @@ static struct strlist *__probe_file__get_namelist(int fd, bool include_group)
> 	struct probe_trace_event tev;
> 	int ret = 0;
>
>+	if (fd < 0)
>+		return NULL;
>+
> 	memset(&tev, 0, sizeof(tev));
> 	rawlist = probe_file__get_rawlist(fd);
> 	if (!rawlist)
>
>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/builtin-probe.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
>> index 132afc9..861aa89 100644
>> --- a/tools/perf/builtin-probe.c
>> +++ b/tools/perf/builtin-probe.c
>> @@ -384,7 +384,11 @@ static int perf_del_probe_events(struct strfilter *filter)
>>  		goto out;
>>  	}
>>
>> -	ret = probe_file__get_events(kfd, filter, klist);
>> +	if (kfd < 0)
>> +		ret = -ENOENT;
>> +	else
>> +		ret = probe_file__get_events(kfd, filter, klist);
>> +
>>  	if (ret == 0) {
>>  		strlist__for_each(ent, klist)
>>  			pr_info("Removed event: %s\n", ent->s);
>> @@ -394,7 +398,11 @@ static int perf_del_probe_events(struct strfilter *filter)
>>  			goto error;
>>  	}
>>
>> -	ret2 = probe_file__get_events(ufd, filter, ulist);
>> +	if (ufd < 0)
>> +		ret2 = -ENOENT;
>> +	else
>> +		ret2 = probe_file__get_events(ufd, filter, ulist);
>> +
>>  	if (ret2 == 0) {
>>  		strlist__for_each(ent, ulist)
>>  			pr_info("Removed event: %s\n", ent->s);
>> --
>> 1.8.3.4

Re: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid

['Arnaldo Carvalho de Melo']

Em Thu, Nov 05, 2015 at 03:07:23PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> >Em Thu, Nov 05, 2015 at 01:19:24PM +0000, Wang Nan escreveu:
> >> In system with kprobe enabled but uprobe turned off, 'perf probe -d'
> >> causes segfault because it calls probe_file__get_events() with a
> >> negative fd (when deleting uprobe events).
> >>
> >> This patch validates fds before calling probe_file__get_events().
> >
> >Wouldn't this shorter patch be more robust by deferring the validation
> >to just before using the 'fd' value?
> >
> >The end result is that probe_file__get_events() will return -ENOENT in
> >both calls, so ret and ret2 will both be set to -ENOENT, as in your
> >patch.
> >
> >Masami?
> 
> Yes, I've suggested so :)

Kinda, you suggested to check at probe_file__get_events() , while I am
suggesting to check even deeper, just before the actual use of fd, in
probe_file__get_rawlist().

- Arnaldo

> https://lkml.org/lkml/2015/11/5/353
> 
> Thanks!
> 
> 
> 
> >
> >- Arnaldo
> >
> >diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> >index 89dbeb92c68e..f04a8318a1a7 100644
> >--- a/tools/perf/util/probe-file.c
> >+++ b/tools/perf/util/probe-file.c
> >@@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
> > 	char *p;
> > 	struct strlist *sl;
> >
> >+	if (fd < 0)
> >+		return NULL;
> >+
> > 	sl = strlist__new(NULL, NULL);
> >
> > 	fp = fdopen(dup(fd), "r");
> >
> >
> >diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> >index 89dbeb92c68e..e5dc8e62f0f1 100644
> >--- a/tools/perf/util/probe-file.c
> >+++ b/tools/perf/util/probe-file.c
> >@@ -169,6 +169,9 @@ static struct strlist *__probe_file__get_namelist(int fd, bool include_group)
> > 	struct probe_trace_event tev;
> > 	int ret = 0;
> >
> >+	if (fd < 0)
> >+		return NULL;
> >+
> > 	memset(&tev, 0, sizeof(tev));
> > 	rawlist = probe_file__get_rawlist(fd);
> > 	if (!rawlist)
> >
> >
> >> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> >> Cc: Jiri Olsa <jolsa@kernel.org>
> >> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >> Cc: Namhyung Kim <namhyung@kernel.org>
> >> ---
> >>  tools/perf/builtin-probe.c | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> >> index 132afc9..861aa89 100644
> >> --- a/tools/perf/builtin-probe.c
> >> +++ b/tools/perf/builtin-probe.c
> >> @@ -384,7 +384,11 @@ static int perf_del_probe_events(struct strfilter *filter)
> >>  		goto out;
> >>  	}
> >>
> >> -	ret = probe_file__get_events(kfd, filter, klist);
> >> +	if (kfd < 0)
> >> +		ret = -ENOENT;
> >> +	else
> >> +		ret = probe_file__get_events(kfd, filter, klist);
> >> +
> >>  	if (ret == 0) {
> >>  		strlist__for_each(ent, klist)
> >>  			pr_info("Removed event: %s\n", ent->s);
> >> @@ -394,7 +398,11 @@ static int perf_del_probe_events(struct strfilter *filter)
> >>  			goto error;
> >>  	}
> >>
> >> -	ret2 = probe_file__get_events(ufd, filter, ulist);
> >> +	if (ufd < 0)
> >> +		ret2 = -ENOENT;
> >> +	else
> >> +		ret2 = probe_file__get_events(ufd, filter, ulist);
> >> +
> >>  	if (ret2 == 0) {
> >>  		strlist__for_each(ent, ulist)
> >>  			pr_info("Removed event: %s\n", ent->s);
> >> --
> >> 1.8.3.4

Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[acme]

Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> From: Wang Nan [mailto:wangnan0@huawei.com]
> >
> >It is possible that find_perf_probe_point_from_map() fails to find
> >symbol but still returns 0 because of an small error when coding:
> >find_perf_probe_point_from_map() set 'ret' to error code at first,
> >but also use it to hold return value of
> >kernel_get_symbol_address_by_name().
> 
> OK, I didn't expect that there is a symbol which can be found by 
> kernel_get_symbol_address_by_name() but not by __find_kernel_function()...

> Would you have any example of the error?
> 
> >
> >This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
> >success, so if !sym, the whole function returns error correctly.
> 
> Hmm, that sounds tricky. I'd rather like to add *psym to kernel_get_symbol_address_by_name() 
> to save symbol and don't use __find_kernel_function() instead.

Tricky? I don't think so, suboptimal? possibly, but it fixes an error,
so should be processed quickly, right? I'm applying his patch and then
whatever improvement can be done on top.

- Arnaldo

RE: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1412 bytes --]

From: acme@kernel.org [mailto:acme@kernel.org]
>
>Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
>> From: Wang Nan [mailto:wangnan0@huawei.com]
>> >
>> >It is possible that find_perf_probe_point_from_map() fails to find
>> >symbol but still returns 0 because of an small error when coding:
>> >find_perf_probe_point_from_map() set 'ret' to error code at first,
>> >but also use it to hold return value of
>> >kernel_get_symbol_address_by_name().
>>
>> OK, I didn't expect that there is a symbol which can be found by
>> kernel_get_symbol_address_by_name() but not by __find_kernel_function()...
>
>> Would you have any example of the error?
>>
>> >
>> >This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
>> >success, so if !sym, the whole function returns error correctly.
>>
>> Hmm, that sounds tricky. I'd rather like to add *psym to kernel_get_symbol_address_by_name()
>> to save symbol and don't use __find_kernel_function() instead.
>
>Tricky? I don't think so, suboptimal? possibly, but it fixes an error,
>so should be processed quickly, right? I'm applying his patch and then
>whatever improvement can be done on top.

OK, then I'll send an improvement patch.

Thanks,

>
>- Arnaldo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2393 bytes --]

From: acme@kernel.org [mailto:acme@kernel.org]
>>
>>Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
>>> From: Wang Nan [mailto:wangnan0@huawei.com]
>>> >
>>> >It is possible that find_perf_probe_point_from_map() fails to find
>>> >symbol but still returns 0 because of an small error when coding:
>>> >find_perf_probe_point_from_map() set 'ret' to error code at first,
>>> >but also use it to hold return value of
>>> >kernel_get_symbol_address_by_name().
>>>
>>> OK, I didn't expect that there is a symbol which can be found by
>>> kernel_get_symbol_address_by_name() but not by __find_kernel_function()...
>>
>>> Would you have any example of the error?
>>>
>>> >
>>> >This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
>>> >success, so if !sym, the whole function returns error correctly.
>>>
>>> Hmm, that sounds tricky. I'd rather like to add *psym to kernel_get_symbol_address_by_name()
>>> to save symbol and don't use __find_kernel_function() instead.
>>
>>Tricky? I don't think so, suboptimal? possibly, but it fixes an error,
>>so should be processed quickly, right? I'm applying his patch and then
>>whatever improvement can be done on top.
>
>OK, then I'll send an improvement patch.

Ah, finally I got what happened. I guess the problem may happen when we put
a probe on the kernel somewhere outside of any functions and run "perf probe -l".
I think it should not be allowed to put the probe outside any symbol.

The background is here, at first "perf-probe -a somewhere" defines a probe in
the kernel but its address is relative from "_text". (thus, vfs_read becomes "_text+2348080"
 for example). Since it is not readable by human, perf probe -l tries to get an appropriate
symbol from the "_text+OFFSET".
For the purpose, the first kernel_get_symbol_address_by_name() is for translating _text to
an address, and the second  __find_kernel_function() is for finding a symbol from the
address+OFFSET.
Then, if the address+OFFSET is out of the symbol map, the second one can fail.
This means the first symbol and the second symbol is not same.

So, the direction of Wang solution is good :). Just a cleanup is required.

Thank you!

>
>Thanks,
>
>>
>>- Arnaldo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[Wangnan (F)]

On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:
> From: acme@kernel.org [mailto:acme@kernel.org]
>>> Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
>>>> From: Wang Nan [mailto:wangnan0@huawei.com]
>>>>> It is possible that find_perf_probe_point_from_map() fails to find
>>>>> symbol but still returns 0 because of an small error when coding:
>>>>> find_perf_probe_point_from_map() set 'ret' to error code at first,
>>>>> but also use it to hold return value of
>>>>> kernel_get_symbol_address_by_name().
>>>> OK, I didn't expect that there is a symbol which can be found by
>>>> kernel_get_symbol_address_by_name() but not by __find_kernel_function()...
>>>> Would you have any example of the error?
>>>>
>>>>> This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
>>>>> success, so if !sym, the whole function returns error correctly.
>>>> Hmm, that sounds tricky. I'd rather like to add *psym to kernel_get_symbol_address_by_name()
>>>> to save symbol and don't use __find_kernel_function() instead.
>>> Tricky? I don't think so, suboptimal? possibly, but it fixes an error,
>>> so should be processed quickly, right? I'm applying his patch and then
>>> whatever improvement can be done on top.
>> OK, then I'll send an improvement patch.
> Ah, finally I got what happened. I guess the problem may happen when we put
> a probe on the kernel somewhere outside of any functions and run "perf probe -l".
> I think it should not be allowed to put the probe outside any symbol.
>
> The background is here, at first "perf-probe -a somewhere" defines a probe in
> the kernel but its address is relative from "_text". (thus, vfs_read becomes "_text+2348080"
>   for example). Since it is not readable by human, perf probe -l tries to get an appropriate
> symbol from the "_text+OFFSET".
> For the purpose, the first kernel_get_symbol_address_by_name() is for translating _text to
> an address, and the second  __find_kernel_function() is for finding a symbol from the
> address+OFFSET.
> Then, if the address+OFFSET is out of the symbol map, the second one can fail.
> This means the first symbol and the second symbol is not same.
>
> So, the direction of Wang solution is good :). Just a cleanup is required.
>
> Thank you!

I also tried to finger out the problem for all day and made some 
progress. It is another
problem. It happeneds when probing an address reside in a module on 
aarch64 system.

On my aarch64 system I use kcore. Different from x86, on aarch64, 
modules address is lower
than normal kernel. For example:

On x86_64:

# readelf -a /proc/kcore

   Type           Offset             VirtAddr           PhysAddr
                  FileSiz            MemSiz              Flags  Align
   ...
   LOAD           0x00007fff81003000 0xffffffff81000000 
0x0000000000000000   <-- kernel
                  0x0000000001026000 0x0000000001026000  RWE    1000
   LOAD           0x00007fffa0003000 0xffffffffa0000000 
0x0000000000000000   <-- module
                  0x000000005f000000 0x000000005f000000  RWE    1000

On aarch64:

   Type           Offset             VirtAddr           PhysAddr
                  FileSiz            MemSiz              Flags  Align
   ...
   LOAD           0x0000000000002000 0xffffffc000000000 
0x0000000000000000    <-- kernel
                  0x000000007fc00000 0x000000007fc00000  RWE    1000
   LOAD           0xfffffffffc002000 0xffffffbffc000000 
0x0000000000000000    <-- module
                  0x0000000004000000 0x0000000004000000  RWE    1000

See? On aarch64, Offset field of module address area is negative.

Which causes a problem in dso__split_kallsyms_for_kcore(): when it 
adjusting symbols
using "pos->start -= curr_map->start - curr_map->pgoff", the relative 
order between
module functions and normal kernel function is changed.

For example:

funca at 0xffffffc00021b428 is a normal kernel function.
funcb at 0xffffffbffc000000 is a function in kernel.

During parsing /proc/kallsyms, address of funca > address of funcb.

However, after the adjusting:

funca becomes:

0xffffffc00021b428 - (0xffffffc000000000 - 0x2000) = 0x21d428

funcb becomes:

0xffffffbffc000000 - (0xffffffbffc000000 - 0xfffffffffc002000) = 
0xfffffffffc002000

address of funca < address of funcb.

Unfortunately, the rbtree is not adjusted in this case.

I hacked symbols__find:

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index b4cc766..8463b0c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -332,12 +332,14 @@ static struct symbol *symbols__find(struct rb_root 
*symbols, u64 ip)
         while (n) {
                 struct symbol *s = rb_entry(n, struct symbol, rb_node);

-               if (ip < s->start)
+               if ((s64)ip < (s64)s->start)
                         n = n->rb_left;
-               else if (ip >= s->end)
+               else if ((s64)ip >= (s64)s->end)
                         n = n->rb_right;
-               else
+               else {
+                       pr_debug("found %p\n", (void *)ip);
                         return s;
+               }
         }

         return NULL;

and get correct result:

try to find information at 3ffc000000 in kernel_module
Failed to find module kernel_module.
Failed to find the path for kernel_module: [kernel_module]
Failed to find corresponding probes from debuginfo.
found 0xfffffffffc002000

However, what we really need is adjusting rbtree in this case.

Could you please give me some hint for fixing this problem? I'm not 
familiar with
this part of code.

Thank you.

Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[Wangnan (F)]

On 2015/11/6 16:30, Wangnan (F) wrote:
>
>
> On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:
>> From: acme@kernel.org [mailto:acme@kernel.org]
>>>> Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / 
>>>> HIRAMATU，MASAMI escreveu:
>>>>> From: Wang Nan [mailto:wangnan0@huawei.com]

[SNIP]
>> Ah, finally I got what happened. I guess the problem may happen when 
>> we put
>> a probe on the kernel somewhere outside of any functions and run 
>> "perf probe -l".
>> I think it should not be allowed to put the probe outside any symbol.
>>
>> The background is here, at first "perf-probe -a somewhere" defines a 
>> probe in
>> the kernel but its address is relative from "_text". (thus, vfs_read 
>> becomes "_text+2348080"
>>   for example). Since it is not readable by human, perf probe -l 
>> tries to get an appropriate
>> symbol from the "_text+OFFSET".
>> For the purpose, the first kernel_get_symbol_address_by_name() is for 
>> translating _text to
>> an address, and the second  __find_kernel_function() is for finding a 
>> symbol from the
>> address+OFFSET.
>> Then, if the address+OFFSET is out of the symbol map, the second one 
>> can fail.
>> This means the first symbol and the second symbol is not same.
>>
>> So, the direction of Wang solution is good :). Just a cleanup is 
>> required.
>>
>> Thank you!
>
> I also tried to finger out the problem for all day and made some 
> progress. It is another
> problem. It happeneds when probing an address reside in a module on 
> aarch64 system.
>
> On my aarch64 system I use kcore. Different from x86, on aarch64, 
> modules address is lower
> than normal kernel. For example:
>
> On x86_64:
>
> # readelf -a /proc/kcore
>
>   Type           Offset             VirtAddr           PhysAddr
>                  FileSiz            MemSiz              Flags Align
>   ...
>   LOAD           0x00007fff81003000 0xffffffff81000000 
> 0x0000000000000000   <-- kernel
>                  0x0000000001026000 0x0000000001026000  RWE 1000
>   LOAD           0x00007fffa0003000 0xffffffffa0000000 
> 0x0000000000000000   <-- module
>                  0x000000005f000000 0x000000005f000000  RWE 1000
>
> On aarch64:
>
>   Type           Offset             VirtAddr           PhysAddr
>                  FileSiz            MemSiz              Flags Align
>   ...
>   LOAD           0x0000000000002000 0xffffffc000000000 
> 0x0000000000000000    <-- kernel
>                  0x000000007fc00000 0x000000007fc00000  RWE 1000
>   LOAD           0xfffffffffc002000 0xffffffbffc000000 
> 0x0000000000000000    <-- module
>                  0x0000000004000000 0x0000000004000000  RWE 1000
>
> See? On aarch64, Offset field of module address area is negative.
>

One thing should be noticed that, even if normal kernel code and modules 
use different
'struct map', they share a same dso. Please see dso__load_kcore, notice 
how it initialize
parameters (md) before calling file__read_maps().

> Which causes a problem in dso__split_kallsyms_for_kcore(): when it 
> adjusting symbols
> using "pos->start -= curr_map->start - curr_map->pgoff", the relative 
> order between
> module functions and normal kernel function is changed.
>
> For example:
>
> funca at 0xffffffc00021b428 is a normal kernel function.
> funcb at 0xffffffbffc000000 is a function in kernel.
>
> During parsing /proc/kallsyms, address of funca > address of funcb.
>
> However, after the adjusting:
>
> funca becomes:
>
> 0xffffffc00021b428 - (0xffffffc000000000 - 0x2000) = 0x21d428
>
> funcb becomes:
>
> 0xffffffbffc000000 - (0xffffffbffc000000 - 0xfffffffffc002000) = 
> 0xfffffffffc002000
>
> address of funca < address of funcb.
>
> Unfortunately, the rbtree is not adjusted in this case.
>

Even if they are in different maps, they share a same dso here, so a 
same rbtree.

Thank you.

[PATCH v2] perf probe: Verify parameters for two functions

[Wang Nan]

On kernel with only one of CONFIG_KPROBE_EVENTS and
CONFIG_UPROBE_EVENTS enabled, 'perf probe -d' causes segfault because
perf_del_probe_events() calls probe_file__get_events() with a negative
fd.

This patch fixes it by add parameter validation at the entry of
probe_file__get_events() and probe_file__get_rawlist(). Since they are
both non-static public functions (in .h file), parameter verifying
is required.

v1 -> v2: Verify fd at the head of probe_file__get_rawlist() instead of
          checking at call site (suggested by Masami and Arnaldo at [1,2]).

[1] http://lkml.kernel.org/r/50399556C9727B4D88A595C8584AAB37526048E3@GSjpTKYDCembx32.service.hitachi.net
[2] http://lkml.kernel.org/r/20151105155830.GV13236@kernel.org

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 89dbeb9..e3b3b92 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
 	char *p;
 	struct strlist *sl;
 
+	if (fd < 0)
+		return NULL;
+
 	sl = strlist__new(NULL, NULL);
 
 	fp = fdopen(dup(fd), "r");
@@ -271,6 +274,9 @@ int probe_file__get_events(int fd, struct strfilter *filter,
 	const char *p;
 	int ret = -ENOENT;
 
+	if (!plist)
+		return -EINVAL;
+
 	namelist = __probe_file__get_namelist(fd, true);
 	if (!namelist)
 		return -ENOENT;
-- 
1.8.3.4

RE: [PATCH v2] perf probe: Verify parameters for two functions

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2011 bytes --]

From: Wang Nan [mailto:wangnan0@huawei.com]
>
>On kernel with only one of CONFIG_KPROBE_EVENTS and
>CONFIG_UPROBE_EVENTS enabled, 'perf probe -d' causes segfault because
>perf_del_probe_events() calls probe_file__get_events() with a negative
>fd.
>
>This patch fixes it by add parameter validation at the entry of
>probe_file__get_events() and probe_file__get_rawlist(). Since they are
>both non-static public functions (in .h file), parameter verifying
>is required.

Looks good to me ! :)

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

Thank you!

>
>v1 -> v2: Verify fd at the head of probe_file__get_rawlist() instead of
>          checking at call site (suggested by Masami and Arnaldo at [1,2]).
>
>[1] http://lkml.kernel.org/r/50399556C9727B4D88A595C8584AAB37526048E3@GSjpTKYDCembx32.service.hitachi.net
>[2] http://lkml.kernel.org/r/20151105155830.GV13236@kernel.org
>
>Signed-off-by: Wang Nan <wangnan0@huawei.com>
>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>Cc: Jiri Olsa <jolsa@kernel.org>
>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>Cc: Namhyung Kim <namhyung@kernel.org>
>---
> tools/perf/util/probe-file.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>index 89dbeb9..e3b3b92 100644
>--- a/tools/perf/util/probe-file.c
>+++ b/tools/perf/util/probe-file.c
>@@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
> 	char *p;
> 	struct strlist *sl;
>
>+	if (fd < 0)
>+		return NULL;
>+
> 	sl = strlist__new(NULL, NULL);
>
> 	fp = fdopen(dup(fd), "r");
>@@ -271,6 +274,9 @@ int probe_file__get_events(int fd, struct strfilter *filter,
> 	const char *p;
> 	int ret = -ENOENT;
>
>+	if (!plist)
>+		return -EINVAL;
>+
> 	namelist = __probe_file__get_namelist(fd, true);
> 	if (!namelist)
> 		return -ENOENT;
>--
>1.8.3.4

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

[Arnaldo Carvalho de Melo]

Em Fri, Nov 06, 2015 at 05:27:06PM +0800, Wangnan (F) escreveu:
> On 2015/11/6 16:30, Wangnan (F) wrote:
> >On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:
> >>From: acme@kernel.org [mailto:acme@kernel.org]
> >>>>Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> >>>>>From: Wang Nan [mailto:wangnan0@huawei.com]
 
> [SNIP]
> >>Ah, finally I got what happened. I guess the problem may happen when
> >>we put a probe on the kernel somewhere outside of any functions and
> >>run "perf probe -l".

> >>I think it should not be allowed to put the probe outside any symbol.

> >>The background is here, at first "perf-probe -a somewhere" defines a
> >>probe in the kernel but its address is relative from "_text". (thus,
> >>vfs_read becomes "_text+2348080"

> >>  for example). Since it is not readable by human, perf probe -l tries
> >>to get an appropriate
> >>symbol from the "_text+OFFSET".
> >>For the purpose, the first kernel_get_symbol_address_by_name() is for
> >>translating _text to
> >>an address, and the second  __find_kernel_function() is for finding a
> >>symbol from the
> >>address+OFFSET.

> >>Then, if the address+OFFSET is out of the symbol map, the second one can
> >>fail.

> >>This means the first symbol and the second symbol is not same.

> >>So, the direction of Wang solution is good :). Just a cleanup is
> >>required.

> >I also tried to finger out the problem for all day and made some progress.
> >It is another
> >problem. It happeneds when probing an address reside in a module on
> >aarch64 system.
> >
> >On my aarch64 system I use kcore. Different from x86, on aarch64, modules
> >address is lower
> >than normal kernel. For example:
> >
> >On x86_64:
> >
> ># readelf -a /proc/kcore
> >
> >  Type           Offset             VirtAddr           PhysAddr
> >                 FileSiz            MemSiz              Flags Align
> >  ...
> >  LOAD           0x00007fff81003000 0xffffffff81000000 0x0000000000000000
> ><-- kernel
> >                 0x0000000001026000 0x0000000001026000  RWE 1000
> >  LOAD           0x00007fffa0003000 0xffffffffa0000000 0x0000000000000000
> ><-- module
> >                 0x000000005f000000 0x000000005f000000  RWE 1000
> >
> >On aarch64:
> >
> >  Type           Offset             VirtAddr           PhysAddr
> >                 FileSiz            MemSiz              Flags Align
> >  ...
> >  LOAD           0x0000000000002000 0xffffffc000000000 0x0000000000000000
> ><-- kernel
> >                 0x000000007fc00000 0x000000007fc00000  RWE 1000
> >  LOAD           0xfffffffffc002000 0xffffffbffc000000 0x0000000000000000
> ><-- module
> >                 0x0000000004000000 0x0000000004000000  RWE 1000
> >
> >See? On aarch64, Offset field of module address area is negative.
> >
> 
> One thing should be noticed that, even if normal kernel code and modules use
> different
> 'struct map', they share a same dso. Please see dso__load_kcore, notice how
> it initialize
> parameters (md) before calling file__read_maps().
> 
> >Which causes a problem in dso__split_kallsyms_for_kcore(): when it
> >adjusting symbols
> >using "pos->start -= curr_map->start - curr_map->pgoff", the relative
> >order between
> >module functions and normal kernel function is changed.
> >
> >For example:
> >
> >funca at 0xffffffc00021b428 is a normal kernel function.
> >funcb at 0xffffffbffc000000 is a function in kernel.
> >
> >During parsing /proc/kallsyms, address of funca > address of funcb.

> >However, after the adjusting:

> >funca becomes:

> >0xffffffc00021b428 - (0xffffffc000000000 - 0x2000) = 0x21d428

> >funcb becomes:

> >0xffffffbffc000000 - (0xffffffbffc000000 - 0xfffffffffc002000) =
> >0xfffffffffc002000

> >address of funca < address of funcb.

> >Unfortunately, the rbtree is not adjusted in this case.
 
> Even if they are in different maps, they share a same dso here, so a same
> rbtree.

Yeah, see the answer to the patch you sent, we can't change the symbols
in a DSO, as it may be shared by multiple maps (think about glibc and
prelink, even without prelink) the same applies for kernel modules, that
we represent in the same way, and in at least one case, i.e. split
kallsyms for modules, core kernel, etc we share the same dso by multiple
maps, so any adjustment that needs to be done should be done to the map
members, not to the dso ones.

CCing Adrian, that originally wrote the kcore code, but IIRC there are
other places that touch sym-> (thus dso internal state) instead of
adjusting map members :-\

- Arnaldo

[tip:perf/urgent] perf tools: Fix find_perf_probe_point_from_map( ) which incorrectly returns success

[tip-bot for Wang Nan]

Commit-ID:  98d3b258ede2cdac31a2728543f652964e597e79
Gitweb:     http://git.kernel.org/tip/98d3b258ede2cdac31a2728543f652964e597e79
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Thu, 5 Nov 2015 13:19:25 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 5 Nov 2015 12:47:52 -0300

perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

It is possible that find_perf_probe_point_from_map() fails to find a
symbol but still returns 0 because of an small error when coding:
find_perf_probe_point_from_map() set 'ret' to error code at first, but
also use it to hold return value of kernel_get_symbol_address_by_name().

This patch resets 'ret' to error even kernel_get_symbol_address_by_name()
success, so if !sym, the whole function returns error correctly.

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

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b51a8bf..e659c4f 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1905,6 +1905,9 @@ static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
 			sym = __find_kernel_function(addr, &map);
 		}
 	}
+
+	/* ret may has be overwritten so reset it */
+	ret = -ENOENT;
 	if (!sym)
 		goto out;
 

Re: [PATCH v2] perf probe: Verify parameters for two functions

[Wangnan (F)]

Hi Arnaldo,

Could you please collect this patch to your tree? It fixes a segfault
when only one of kprobe and uprobe is enabled.

Thank you.

On 2015/11/6 17:50, Wang Nan wrote:
> On kernel with only one of CONFIG_KPROBE_EVENTS and
> CONFIG_UPROBE_EVENTS enabled, 'perf probe -d' causes segfault because
> perf_del_probe_events() calls probe_file__get_events() with a negative
> fd.
>
> This patch fixes it by add parameter validation at the entry of
> probe_file__get_events() and probe_file__get_rawlist(). Since they are
> both non-static public functions (in .h file), parameter verifying
> is required.
>
> v1 -> v2: Verify fd at the head of probe_file__get_rawlist() instead of
>            checking at call site (suggested by Masami and Arnaldo at [1,2]).
>
> [1] http://lkml.kernel.org/r/50399556C9727B4D88A595C8584AAB37526048E3@GSjpTKYDCembx32.service.hitachi.net
> [2] http://lkml.kernel.org/r/20151105155830.GV13236@kernel.org
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> ---
>   tools/perf/util/probe-file.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 89dbeb9..e3b3b92 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
>   	char *p;
>   	struct strlist *sl;
>   
> +	if (fd < 0)
> +		return NULL;
> +
>   	sl = strlist__new(NULL, NULL);
>   
>   	fp = fdopen(dup(fd), "r");
> @@ -271,6 +274,9 @@ int probe_file__get_events(int fd, struct strfilter *filter,
>   	const char *p;
>   	int ret = -ENOENT;
>   
> +	if (!plist)
> +		return -EINVAL;
> +
>   	namelist = __probe_file__get_namelist(fd, true);
>   	if (!namelist)
>   		return -ENOENT;

[tip:perf/urgent] perf probe: Verify parameters in two functions

[tip-bot for Wang Nan]

Commit-ID:  421fd0845eaeecce6b3806f7f0c0d67d1f9ad108
Gitweb:     http://git.kernel.org/tip/421fd0845eaeecce6b3806f7f0c0d67d1f9ad108
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Fri, 6 Nov 2015 09:50:15 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 11 Nov 2015 18:41:32 -0300

perf probe: Verify parameters in two functions

On kernel with only one out of CONFIG_KPROBE_EVENTS and
CONFIG_UPROBE_EVENTS enabled, 'perf probe -d' causes a segfault because
perf_del_probe_events() calls probe_file__get_events() with a negative
fd.

This patch fixes it by adding parameter validation at the entry of
probe_file__get_events() and probe_file__get_rawlist(). Since they are
both non-static public functions (in .h file), parameter verifying is
required.

v1 -> v2: Verify fd at the head of probe_file__get_rawlist() instead of
          checking at call site (suggested by Masami and Arnaldo at [1,2]).

[1] http://lkml.kernel.org/r/50399556C9727B4D88A595C8584AAB37526048E3@GSjpTKYDCembx32.service.hitachi.net
[2] http://lkml.kernel.org/r/20151105155830.GV13236@kernel.org

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

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 89dbeb9..e3b3b92 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -138,6 +138,9 @@ struct strlist *probe_file__get_rawlist(int fd)
 	char *p;
 	struct strlist *sl;
 
+	if (fd < 0)
+		return NULL;
+
 	sl = strlist__new(NULL, NULL);
 
 	fp = fdopen(dup(fd), "r");
@@ -271,6 +274,9 @@ int probe_file__get_events(int fd, struct strfilter *filter,
 	const char *p;
 	int ret = -ENOENT;
 
+	if (!plist)
+		return -EINVAL;
+
 	namelist = __probe_file__get_namelist(fd, true);
 	if (!namelist)
 		return -ENOENT;