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
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 <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
From: Wang Nan [mailto:[email protected]]
>
>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 <[email protected]>
>Cc: Arnaldo Carvalho de Melo <[email protected]>
>Cc: Jiri Olsa <[email protected]>
>Cc: Masami Hiramatsu <[email protected]>
>Cc: Namhyung Kim <[email protected]>
>---
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
From: Wang Nan [mailto:[email protected]]
>
>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 <[email protected]>
>Cc: Arnaldo Carvalho de Melo <[email protected]>
>Cc: Jiri Olsa <[email protected]>
>Cc: Masami Hiramatsu <[email protected]>
>Cc: Namhyung Kim <[email protected]>
>---
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> ---
> 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
From: Arnaldo Carvalho de Melo [mailto:[email protected]]
>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 <[email protected]>
>> Cc: Arnaldo Carvalho de Melo <[email protected]>
>> Cc: Jiri Olsa <[email protected]>
>> Cc: Masami Hiramatsu <[email protected]>
>> Cc: Namhyung Kim <[email protected]>
>> ---
>> 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
Em Thu, Nov 05, 2015 at 03:07:23PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> From: Arnaldo Carvalho de Melo [mailto:[email protected]]
> >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 <[email protected]>
> >> Cc: Arnaldo Carvalho de Melo <[email protected]>
> >> Cc: Jiri Olsa <[email protected]>
> >> Cc: Masami Hiramatsu <[email protected]>
> >> Cc: Namhyung Kim <[email protected]>
> >> ---
> >> 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
Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> From: Wang Nan [mailto:[email protected]]
> >
> >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
From: [email protected] [mailto:[email protected]]
>
>Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
>> From: Wang Nan [mailto:[email protected]]
>> >
>> >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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
From: [email protected] [mailto:[email protected]]
>>
>>Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
>>> From: Wang Nan [mailto:[email protected]]
>>> >
>>> >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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:
> From: [email protected] [mailto:[email protected]]
>>> Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
>>>> From: Wang Nan [mailto:[email protected]]
>>>>> 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.
On 2015/11/6 16:30, Wangnan (F) wrote:
>
>
> On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:
>> From: [email protected] [mailto:[email protected]]
>>>> Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 /
>>>> HIRAMATU，MASAMI escreveu:
>>>>> From: Wang Nan [mailto:[email protected]]
[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.
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/[email protected]
Signed-off-by: Wang Nan <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
From: Wang Nan [mailto:[email protected]]
>
>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 <[email protected]>
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/[email protected]
>
>Signed-off-by: Wang Nan <[email protected]>
>Cc: Arnaldo Carvalho de Melo <[email protected]>
>Cc: Jiri Olsa <[email protected]>
>Cc: Masami Hiramatsu <[email protected]>
>Cc: Namhyung Kim <[email protected]>
>---
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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: [email protected] [mailto:[email protected]]
> >>>>Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> >>>>>From: Wang Nan [mailto:[email protected]]
> [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
Commit-ID: 98d3b258ede2cdac31a2728543f652964e597e79
Gitweb: http://git.kernel.org/tip/98d3b258ede2cdac31a2728543f652964e597e79
Author: Wang Nan <[email protected]>
AuthorDate: Thu, 5 Nov 2015 13:19:25 +0000
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Zefan Li <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;
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/[email protected]
>
> Signed-off-by: Wang Nan <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> ---
> 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;
Commit-ID: 421fd0845eaeecce6b3806f7f0c0d67d1f9ad108
Gitweb: http://git.kernel.org/tip/421fd0845eaeecce6b3806f7f0c0d67d1f9ad108
Author: Wang Nan <[email protected]>
AuthorDate: Fri, 6 Nov 2015 09:50:15 +0000
Committer: Arnaldo Carvalho de Melo <[email protected]>
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/[email protected]
Signed-off-by: Wang Nan <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Zefan Li <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;