[PATCH] perf probe: Fix --line option behavior

[PATCH] perf probe: Fix --line option behavior

[Namhyung Kim]

The commit 5a62257a3ddd1 ("perf probe: Replace line_list with
intlist") replaced line_list to intlist but it has a problem that if a
same line was added again, it'd return -EEXIST rather than 1.

Since line_range_walk_cb() only checks the result being negative, it
resulted in failure or segfault sometimes.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-finder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index df0238654698..3bf0c8cdccb7 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1441,13 +1441,15 @@ static int line_range_walk_cb(const char *fname, int lineno,
 			      void *data)
 {
 	struct line_finder *lf = data;
+	int err;
 
 	if ((strtailcmp(fname, lf->fname) != 0) ||
 	    (lf->lno_s > lineno || lf->lno_e < lineno))
 		return 0;
 
-	if (line_range_add_line(fname, lineno, lf->lr) < 0)
-		return -EINVAL;
+	err = line_range_add_line(fname, lineno, lf->lr);
+	if (err < 0 && err != -EEXIST)
+		return err;
 
 	return 0;
 }
-- 
1.7.11.7

Re: [PATCH] perf probe: Fix --line option behavior

[Masami Hiramatsu]

(2014/04/01 13:47), Namhyung Kim wrote:
> The commit 5a62257a3ddd1 ("perf probe: Replace line_list with
> intlist") replaced line_list to intlist but it has a problem that if a
> same line was added again, it'd return -EEXIST rather than 1.

Ah, right! that's a different behavior.

> Since line_range_walk_cb() only checks the result being negative, it
> resulted in failure or segfault sometimes.

Could you give me an example input of the segfault? I'd like to trace
it down.

> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

Thank you very much!

> ---
>  tools/perf/util/probe-finder.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index df0238654698..3bf0c8cdccb7 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1441,13 +1441,15 @@ static int line_range_walk_cb(const char *fname, int lineno,
>  			      void *data)
>  {
>  	struct line_finder *lf = data;
> +	int err;
>  
>  	if ((strtailcmp(fname, lf->fname) != 0) ||
>  	    (lf->lno_s > lineno || lf->lno_e < lineno))
>  		return 0;
>  
> -	if (line_range_add_line(fname, lineno, lf->lr) < 0)
> -		return -EINVAL;
> +	err = line_range_add_line(fname, lineno, lf->lr);
> +	if (err < 0 && err != -EEXIST)
> +		return err;
>  
>  	return 0;
>  }
> 


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] perf probe: Fix --line option behavior

[Namhyung Kim]

Hi Masami,

(Also adding Jiri in CC and changing email of acme)

On Tue, Apr 1, 2014 at 5:01 AM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (2014/04/01 13:47), Namhyung Kim wrote:
>> The commit 5a62257a3ddd1 ("perf probe: Replace line_list with
>> intlist") replaced line_list to intlist but it has a problem that if a
>> same line was added again, it'd return -EEXIST rather than 1.
>
> Ah, right! that's a different behavior.
>
>> Since line_range_walk_cb() only checks the result being negative, it
>> resulted in failure or segfault sometimes.
>
> Could you give me an example input of the segfault? I'd like to trace
> it down.

Just used current acme/perf/core.

$ git log -1
commit 28b5724a61cc9d84f2cbef6675e8a85ae4b1bc57
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Thu Mar 27 17:36:14 2014 -0300

    MAINTAINERS: Change e-mail to kernel.org one

    Leaving ghostprotocols.net for old networking stuff.

...

$ perf --version
perf version 3.14.rc6.g28b572

$ perf probe -x ./perf -v -L map__load
Open Debuginfo file: /home/namhyung/project/linux/tools/perf/perf
fname: util/map.c, lineno:153
New line range: 153 to 2147483647
path: (null)
Segmentation fault (core dumped)


It seems like find_line_range_by_line() called for multiple times,
line_range_walk_cb returns 0 at first but as line_range_add_line()
returns -EEXIST, it freed lr->path and __show_line_range() ended up
accessing to NULL path.


(gdb) bt
#0  get_real_path (new_path=0x839ff8 <params+9272>,
    comp_dir=0x1f994f0 "/home/namhyung/project/linux/tools/perf",
    raw_path=0x0) at util/probe-event.c:518
#1  __show_line_range (module=<optimized out>, lr=0x839fd8 <params+9240>)
    at util/probe-event.c:641
#2  show_line_range (lr=lr@entry=0x839fd8 <params+9240>,
    module=<optimized out>) at util/probe-event.c:699
#3  0x000000000044667b in __cmd_probe (argc=<optimized out>,
    argv=argv@entry=0x7fff7a770ba0, prefix=<optimized out>)
    at builtin-probe.c:465
#4  0x00000000004469f4 in cmd_probe (argc=<optimized out>,
    argv=0x7fff7a770ba0, prefix=<optimized out>) at builtin-probe.c:520
#5  0x00000000004241b5 in run_builtin (p=p@entry=0x7ebde0 <commands+384>,
    argc=argc@entry=6, argv=argv@entry=0x7fff7a770ba0) at perf.c:319
#6  0x0000000000423a29 in handle_internal_command (argv=0x7fff7a770ba0,
    argc=6) at perf.c:376
#7  run_argv (argv=0x7fff7a7709a0, argcp=0x7fff7a7709ac) at perf.c:420
#8  main (argc=6, argv=0x7fff7a770ba0) at perf.c:529


Thanks,
Namhyung

Re: Re: [PATCH] perf probe: Fix --line option behavior

[Masami Hiramatsu]

(2014/04/01 16:21), Namhyung Kim wrote:
> Hi Masami,
> 
> (Also adding Jiri in CC and changing email of acme)
> 
> On Tue, Apr 1, 2014 at 5:01 AM, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> wrote:
>> (2014/04/01 13:47), Namhyung Kim wrote:
>>> The commit 5a62257a3ddd1 ("perf probe: Replace line_list with
>>> intlist") replaced line_list to intlist but it has a problem that if a
>>> same line was added again, it'd return -EEXIST rather than 1.
>>
>> Ah, right! that's a different behavior.
>>
>>> Since line_range_walk_cb() only checks the result being negative, it
>>> resulted in failure or segfault sometimes.
>>
>> Could you give me an example input of the segfault? I'd like to trace
>> it down.
> 
> Just used current acme/perf/core.
> 
> $ git log -1
> commit 28b5724a61cc9d84f2cbef6675e8a85ae4b1bc57
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Thu Mar 27 17:36:14 2014 -0300
> 
>     MAINTAINERS: Change e-mail to kernel.org one
> 
>     Leaving ghostprotocols.net for old networking stuff.
> 
> ...
> 
> $ perf --version
> perf version 3.14.rc6.g28b572
> 
> $ perf probe -x ./perf -v -L map__load
> Open Debuginfo file: /home/namhyung/project/linux/tools/perf/perf
> fname: util/map.c, lineno:153
> New line range: 153 to 2147483647
> path: (null)
> Segmentation fault (core dumped)
> 
> 
> It seems like find_line_range_by_line() called for multiple times,
> line_range_walk_cb returns 0 at first but as line_range_add_line()
> returns -EEXIST, it freed lr->path and __show_line_range() ended up
> accessing to NULL path.

OK, thanks for this nice report!
Hmm, line_range_inline_cb() must check the result of
find_line_range_by_line() but ignored. That should be fixed too...

Thank you,

> 
> 
> (gdb) bt
> #0  get_real_path (new_path=0x839ff8 <params+9272>,
>     comp_dir=0x1f994f0 "/home/namhyung/project/linux/tools/perf",
>     raw_path=0x0) at util/probe-event.c:518
> #1  __show_line_range (module=<optimized out>, lr=0x839fd8 <params+9240>)
>     at util/probe-event.c:641
> #2  show_line_range (lr=lr@entry=0x839fd8 <params+9240>,
>     module=<optimized out>) at util/probe-event.c:699
> #3  0x000000000044667b in __cmd_probe (argc=<optimized out>,
>     argv=argv@entry=0x7fff7a770ba0, prefix=<optimized out>)
>     at builtin-probe.c:465
> #4  0x00000000004469f4 in cmd_probe (argc=<optimized out>,
>     argv=0x7fff7a770ba0, prefix=<optimized out>) at builtin-probe.c:520
> #5  0x00000000004241b5 in run_builtin (p=p@entry=0x7ebde0 <commands+384>,
>     argc=argc@entry=6, argv=argv@entry=0x7fff7a770ba0) at perf.c:319
> #6  0x0000000000423a29 in handle_internal_command (argv=0x7fff7a770ba0,
>     argc=6) at perf.c:376
> #7  run_argv (argv=0x7fff7a7709a0, argcp=0x7fff7a7709ac) at perf.c:420
> #8  main (argc=6, argv=0x7fff7a770ba0) at perf.c:529
> 
> 
> Thanks,
> Namhyung
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH -tip ] [BUGFIX] perf/probe: Fix to handle errors in line_range searching

[Masami Hiramatsu]

As Namhyung reported(https://lkml.org/lkml/2014/4/1/89),
current perf-probe -L option doesn't handle errors in line-range
searching correctly. It causes a SEGV if an error occured in the
line-range searching.

  ----
  $ perf probe -x ./perf -v -L map__load
  Open Debuginfo file: /home/namhyung/project/linux/tools/perf/perf
  fname: util/map.c, lineno:153
  New line range: 153 to 2147483647
  path: (null)
  Segmentation fault (core dumped)
  ----

This is because line_range_inline_cb() ignores errors
from find_line_range_by_line() which means that lr->path is
already freed on the error path in find_line_range_by_line().
As a result, get_real_path() accesses the lr->path and it
causes a NULL pointer exception.

This fixes line_range_inline_cb() to handle the error correctly,
and report it to the caller.

Anyway, this just fixes a possible SEGV bug, Namhyung's patch
is also required.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reported-by: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/probe-finder.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index df02386..edcd2ff 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1473,14 +1473,15 @@ static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
 
 static int line_range_inline_cb(Dwarf_Die *in_die, void *data)
 {
-	find_line_range_by_line(in_die, data);
+	int ret = find_line_range_by_line(in_die, data);
 
 	/*
 	 * We have to check all instances of inlined function, because
 	 * some execution paths can be optimized out depends on the
-	 * function argument of instances
+	 * function argument of instances. However, if an error occurs,
+	 * it should be handled by the caller.
 	 */
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 /* Search function definition from function name */

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix to handle errors in line_range searching

[Jiri Olsa]

On Wed, Apr 02, 2014 at 02:48:31PM +0900, Masami Hiramatsu wrote:
> As Namhyung reported(https://lkml.org/lkml/2014/4/1/89),
> current perf-probe -L option doesn't handle errors in line-range
> searching correctly. It causes a SEGV if an error occured in the
> line-range searching.
> 
>   ----
>   $ perf probe -x ./perf -v -L map__load
>   Open Debuginfo file: /home/namhyung/project/linux/tools/perf/perf
>   fname: util/map.c, lineno:153
>   New line range: 153 to 2147483647
>   path: (null)
>   Segmentation fault (core dumped)
>   ----
> 
> This is because line_range_inline_cb() ignores errors
> from find_line_range_by_line() which means that lr->path is
> already freed on the error path in find_line_range_by_line().
> As a result, get_real_path() accesses the lr->path and it
> causes a NULL pointer exception.
> 
> This fixes line_range_inline_cb() to handle the error correctly,
> and report it to the caller.
> 
> Anyway, this just fixes a possible SEGV bug, Namhyung's patch
> is also required.

will take both.. Namhyung, ack to Masami's change?

thanks,
jirka

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix to handle errors in line_range searching

[Namhyung Kim]

On Sun, 6 Apr 2014 14:52:48 +0200, Jiri Olsa wrote:
> On Wed, Apr 02, 2014 at 02:48:31PM +0900, Masami Hiramatsu wrote:
>> As Namhyung reported(https://lkml.org/lkml/2014/4/1/89),
>> current perf-probe -L option doesn't handle errors in line-range
>> searching correctly. It causes a SEGV if an error occured in the
>> line-range searching.
>> 
>>   ----
>>   $ perf probe -x ./perf -v -L map__load
>>   Open Debuginfo file: /home/namhyung/project/linux/tools/perf/perf
>>   fname: util/map.c, lineno:153
>>   New line range: 153 to 2147483647
>>   path: (null)
>>   Segmentation fault (core dumped)
>>   ----
>> 
>> This is because line_range_inline_cb() ignores errors
>> from find_line_range_by_line() which means that lr->path is
>> already freed on the error path in find_line_range_by_line().
>> As a result, get_real_path() accesses the lr->path and it
>> causes a NULL pointer exception.
>> 
>> This fixes line_range_inline_cb() to handle the error correctly,
>> and report it to the caller.
>> 
>> Anyway, this just fixes a possible SEGV bug, Namhyung's patch
>> is also required.
>
> will take both.. Namhyung, ack to Masami's change?

Yes,

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

[tip:perf/urgent] perf probe: Fix --line option behavior

[tip-bot for Namhyung Kim]

Commit-ID:  202c7c123c96a1c193149b7fa2718d7fb143efb2
Gitweb:     http://git.kernel.org/tip/202c7c123c96a1c193149b7fa2718d7fb143efb2
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 1 Apr 2014 13:47:57 +0900
Committer:  Jiri Olsa <jolsa@redhat.com>
CommitDate: Mon, 14 Apr 2014 12:55:39 +0200

perf probe: Fix --line option behavior

The commit 5a62257a3ddd1 ("perf probe: Replace line_list with
intlist") replaced line_list to intlist but it has a problem that if a
same line was added again, it'd return -EEXIST rather than 1.

Since line_range_walk_cb() only checks the result being negative, it
resulted in failure or segfault sometimes.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Link: http://lkml.kernel.org/r/1396327677-3657-1-git-send-email-namhyung@kernel.org
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/probe-finder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index df02386..3bf0c8c 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1441,13 +1441,15 @@ static int line_range_walk_cb(const char *fname, int lineno,
 			      void *data)
 {
 	struct line_finder *lf = data;
+	int err;
 
 	if ((strtailcmp(fname, lf->fname) != 0) ||
 	    (lf->lno_s > lineno || lf->lno_e < lineno))
 		return 0;
 
-	if (line_range_add_line(fname, lineno, lf->lr) < 0)
-		return -EINVAL;
+	err = line_range_add_line(fname, lineno, lf->lr);
+	if (err < 0 && err != -EEXIST)
+		return err;
 
 	return 0;
 }

[tip:perf/urgent] perf probe: Fix to handle errors in line_range searching

[tip-bot for Masami Hiramatsu]

Commit-ID:  182c228ebcf1ac67a44e62236d8f7a8a9a3c5699
Gitweb:     http://git.kernel.org/tip/182c228ebcf1ac67a44e62236d8f7a8a9a3c5699
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Wed, 2 Apr 2014 14:48:31 +0900
Committer:  Jiri Olsa <jolsa@redhat.com>
CommitDate: Mon, 14 Apr 2014 12:55:39 +0200

perf probe: Fix to handle errors in line_range searching

As Namhyung reported(https://lkml.org/lkml/2014/4/1/89),
current perf-probe -L option doesn't handle errors in line-range
searching correctly. It causes a SEGV if an error occured in the
line-range searching.

  ----
  $ perf probe -x ./perf -v -L map__load
  Open Debuginfo file: /home/namhyung/project/linux/tools/perf/perf
  fname: util/map.c, lineno:153
  New line range: 153 to 2147483647
  path: (null)
  Segmentation fault (core dumped)
  ----

This is because line_range_inline_cb() ignores errors
from find_line_range_by_line() which means that lr->path is
already freed on the error path in find_line_range_by_line().
As a result, get_real_path() accesses the lr->path and it
causes a NULL pointer exception.

This fixes line_range_inline_cb() to handle the error correctly,
and report it to the caller.

Anyway, this just fixes a possible SEGV bug, Namhyung's patch
is also required.

Reported-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20140402054831.19080.27006.stgit@ltc230.yrl.intra.hitachi.co.jp
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/probe-finder.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 3bf0c8c..fae274e 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1475,14 +1475,15 @@ static int find_line_range_by_line(Dwarf_Die *sp_die, struct line_finder *lf)
 
 static int line_range_inline_cb(Dwarf_Die *in_die, void *data)
 {
-	find_line_range_by_line(in_die, data);
+	int ret = find_line_range_by_line(in_die, data);
 
 	/*
 	 * We have to check all instances of inlined function, because
 	 * some execution paths can be optimized out depends on the
-	 * function argument of instances
+	 * function argument of instances. However, if an error occurs,
+	 * it should be handled by the caller.
 	 */
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 /* Search function definition from function name */