* [PATCH 0/2] perf tools: Two bugfixs related to perf probe @ 2015-11-05 13:19 Wang Nan 2015-11-05 13:19 ` [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid Wang Nan 2015-11-05 13:19 ` [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success Wang Nan 0 siblings, 2 replies; 19+ messages in thread From: Wang Nan @ 2015-11-05 13:19 UTC (permalink / raw) To: acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, masami.hiramatsu.pt, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid 2015-11-05 13:19 [PATCH 0/2] perf tools: Two bugfixs related to perf probe Wang Nan @ 2015-11-05 13:19 ` Wang Nan 2015-11-05 14:23 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 14:57 ` Arnaldo Carvalho de Melo 2015-11-05 13:19 ` [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success Wang Nan 1 sibling, 2 replies; 19+ messages in thread From: Wang Nan @ 2015-11-05 13:19 UTC (permalink / raw) To: acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, masami.hiramatsu.pt, Wang Nan, Arnaldo Carvalho de Melo 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid 2015-11-05 13:19 ` [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid Wang Nan @ 2015-11-05 14:23 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 14:57 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 19+ messages in thread From: 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-05 14:23 UTC (permalink / raw) To: 'Wang Nan', acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, Arnaldo Carvalho de Melo [-- 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¥ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid 2015-11-05 13:19 ` [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid Wang Nan 2015-11-05 14:23 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-05 14:57 ` Arnaldo Carvalho de Melo 2015-11-05 15:07 ` 平松雅巳 / HIRAMATU,MASAMI 1 sibling, 1 reply; 19+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-11-05 14:57 UTC (permalink / raw) To: Wang Nan Cc: namhyung, lizefan, pi3orama, linux-kernel, jolsa, masami.hiramatsu.pt 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid 2015-11-05 14:57 ` Arnaldo Carvalho de Melo @ 2015-11-05 15:07 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 15:58 ` 'Arnaldo Carvalho de Melo' 0 siblings, 1 reply; 19+ messages in thread From: 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-05 15:07 UTC (permalink / raw) To: 'Arnaldo Carvalho de Melo', Wang Nan Cc: namhyung, lizefan, pi3orama, linux-kernel, jolsa 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid 2015-11-05 15:07 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-05 15:58 ` 'Arnaldo Carvalho de Melo' 2015-11-06 9:50 ` [PATCH v2] perf probe: Verify parameters for two functions Wang Nan 0 siblings, 1 reply; 19+ messages in thread From: 'Arnaldo Carvalho de Melo' @ 2015-11-05 15:58 UTC (permalink / raw) To: 平松雅巳 / HIRAMATU,MASAMI Cc: Wang Nan, namhyung, lizefan, pi3orama, linux-kernel, jolsa 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] perf probe: Verify parameters for two functions 2015-11-05 15:58 ` 'Arnaldo Carvalho de Melo' @ 2015-11-06 9:50 ` Wang Nan 2015-11-06 10:03 ` 平松雅巳 / HIRAMATU,MASAMI ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Wang Nan @ 2015-11-06 9:50 UTC (permalink / raw) To: acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, masami.hiramatsu.pt, Wang Nan, Arnaldo Carvalho de Melo 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH v2] perf probe: Verify parameters for two functions 2015-11-06 9:50 ` [PATCH v2] perf probe: Verify parameters for two functions Wang Nan @ 2015-11-06 10:03 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-11 7:00 ` Wangnan (F) 2015-11-12 6:43 ` [tip:perf/urgent] perf probe: Verify parameters in " tip-bot for Wang Nan 2 siblings, 0 replies; 19+ messages in thread From: 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-06 10:03 UTC (permalink / raw) To: 'Wang Nan', acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, Arnaldo Carvalho de Melo [-- 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¥ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] perf probe: Verify parameters for two functions 2015-11-06 9:50 ` [PATCH v2] perf probe: Verify parameters for two functions Wang Nan 2015-11-06 10:03 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-11 7:00 ` Wangnan (F) 2015-11-12 6:43 ` [tip:perf/urgent] perf probe: Verify parameters in " tip-bot for Wang Nan 2 siblings, 0 replies; 19+ messages in thread From: Wangnan (F) @ 2015-11-11 7:00 UTC (permalink / raw) To: acme Cc: namhyung, lizefan, pi3orama, linux-kernel, jolsa, masami.hiramatsu.pt, Arnaldo Carvalho de Melo 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; ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:perf/urgent] perf probe: Verify parameters in two functions 2015-11-06 9:50 ` [PATCH v2] perf probe: Verify parameters for two functions Wang Nan 2015-11-06 10:03 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-11 7:00 ` Wangnan (F) @ 2015-11-12 6:43 ` tip-bot for Wang Nan 2 siblings, 0 replies; 19+ messages in thread From: tip-bot for Wang Nan @ 2015-11-12 6:43 UTC (permalink / raw) To: linux-tip-commits Cc: namhyung, lizefan, hpa, masami.hiramatsu.pt, wangnan0, linux-kernel, acme, tglx, jolsa, mingo 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; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-05 13:19 [PATCH 0/2] perf tools: Two bugfixs related to perf probe Wang Nan 2015-11-05 13:19 ` [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid Wang Nan @ 2015-11-05 13:19 ` Wang Nan 2015-11-05 14:08 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-08 7:31 ` [tip:perf/urgent] perf tools: Fix find_perf_probe_point_from_map( ) " tip-bot for Wang Nan 1 sibling, 2 replies; 19+ messages in thread From: Wang Nan @ 2015-11-05 13:19 UTC (permalink / raw) To: acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, masami.hiramatsu.pt, Wang Nan, Arnaldo Carvalho de Melo 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-05 13:19 ` [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success Wang Nan @ 2015-11-05 14:08 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 16:00 ` acme 2015-11-08 7:31 ` [tip:perf/urgent] perf tools: Fix find_perf_probe_point_from_map( ) " tip-bot for Wang Nan 1 sibling, 1 reply; 19+ messages in thread From: 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-05 14:08 UTC (permalink / raw) To: 'Wang Nan', acme, namhyung Cc: lizefan, pi3orama, linux-kernel, jolsa, Arnaldo Carvalho de Melo [-- 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¥ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-05 14:08 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-05 16:00 ` acme 2015-11-06 6:28 ` 平松雅巳 / HIRAMATU,MASAMI 0 siblings, 1 reply; 19+ messages in thread From: acme @ 2015-11-05 16:00 UTC (permalink / raw) To: 平松雅巳 / HIRAMATU,MASAMI Cc: 'Wang Nan', namhyung, lizefan, pi3orama, linux-kernel, jolsa 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-05 16:00 ` acme @ 2015-11-06 6:28 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-06 7:12 ` 平松雅巳 / HIRAMATU,MASAMI 0 siblings, 1 reply; 19+ messages in thread From: 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-06 6:28 UTC (permalink / raw) To: 'acme@kernel.org' Cc: 'Wang Nan', namhyung, lizefan, pi3orama, linux-kernel, jolsa [-- 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¥ ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-06 6:28 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-06 7:12 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-06 8:30 ` Wangnan (F) 0 siblings, 1 reply; 19+ messages in thread From: 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-06 7:12 UTC (permalink / raw) To: 'ltc-kernel@ml.yrl.intra.hitachi.co.jp', 'acme@kernel.org' Cc: 'Wang Nan', namhyung, lizefan, pi3orama, linux-kernel, jolsa [-- 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¥ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-06 7:12 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-06 8:30 ` Wangnan (F) 2015-11-06 9:27 ` Wangnan (F) 0 siblings, 1 reply; 19+ messages in thread From: Wangnan (F) @ 2015-11-06 8:30 UTC (permalink / raw) To: 平松雅巳 / HIRAMATU,MASAMI, 'ltc-kernel@ml.yrl.intra.hitachi.co.jp', 'acme@kernel.org' Cc: namhyung, lizefan, pi3orama, linux-kernel, jolsa 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. ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-06 8:30 ` Wangnan (F) @ 2015-11-06 9:27 ` Wangnan (F) 2015-11-06 13:43 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 19+ messages in thread From: Wangnan (F) @ 2015-11-06 9:27 UTC (permalink / raw) To: 平松雅巳 / HIRAMATU,MASAMI, 'ltc-kernel@ml.yrl.intra.hitachi.co.jp', 'acme@kernel.org' Cc: namhyung, lizefan, pi3orama, linux-kernel, jolsa 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success 2015-11-06 9:27 ` Wangnan (F) @ 2015-11-06 13:43 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 19+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-11-06 13:43 UTC (permalink / raw) To: Wangnan (F) Cc: Adrian Hunter, 平松雅巳 / HIRAMATU,MASAMI, 'ltc-kernel@ml.yrl.intra.hitachi.co.jp', namhyung, lizefan, pi3orama, linux-kernel, jolsa 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:perf/urgent] perf tools: Fix find_perf_probe_point_from_map( ) which incorrectly returns success 2015-11-05 13:19 ` [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success Wang Nan 2015-11-05 14:08 ` 平松雅巳 / HIRAMATU,MASAMI @ 2015-11-08 7:31 ` tip-bot for Wang Nan 1 sibling, 0 replies; 19+ messages in thread From: tip-bot for Wang Nan @ 2015-11-08 7:31 UTC (permalink / raw) To: linux-tip-commits Cc: wangnan0, acme, linux-kernel, lizefan, namhyung, hpa, tglx, jolsa, masami.hiramatsu.pt, mingo 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; ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-11-12 6:44 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-05 13:19 [PATCH 0/2] perf tools: Two bugfixs related to perf probe Wang Nan 2015-11-05 13:19 ` [PATCH 1/2] perf probe: Only call probe_file__get_events() when fd is valid Wang Nan 2015-11-05 14:23 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 14:57 ` Arnaldo Carvalho de Melo 2015-11-05 15:07 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 15:58 ` 'Arnaldo Carvalho de Melo' 2015-11-06 9:50 ` [PATCH v2] perf probe: Verify parameters for two functions Wang Nan 2015-11-06 10:03 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-11 7:00 ` Wangnan (F) 2015-11-12 6:43 ` [tip:perf/urgent] perf probe: Verify parameters in " tip-bot for Wang Nan 2015-11-05 13:19 ` [PATCH 2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success Wang Nan 2015-11-05 14:08 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-05 16:00 ` acme 2015-11-06 6:28 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-06 7:12 ` 平松雅巳 / HIRAMATU,MASAMI 2015-11-06 8:30 ` Wangnan (F) 2015-11-06 9:27 ` Wangnan (F) 2015-11-06 13:43 ` Arnaldo Carvalho de Melo 2015-11-08 7:31 ` [tip:perf/urgent] perf tools: Fix find_perf_probe_point_from_map( ) " tip-bot for Wang Nan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.