Re: [PATCH perf/core ] [BUGFIX] perf probe: Fix memory leaking on faiulre by clearing all probe_trace_events

[Wangnan (F)]

Hi Masami,

Today I remember the reason why I introduced patch [1]. Although your 
patch is correct,
either [1] or [2] is still required, but they are both need to be fixed.

Here is a bug:

A segfault raises if use glob matching and argument together and one of 
probe point
failed to get that argument:

# ./perf probe -v -n 'SyS_dup? oldfd'
probe-definition(0): SyS_dup? oldfd
symbol:SyS_dup? file:(null) line:0 offset:0 return:0 lazy:(null)
parsing arg: oldfd into oldfd
1 arguments
Looking at the vmlinux_path (7 entries long)
Using /lib/modules/4.3.0-rc4+/build/vmlinux for symbols
Open Debuginfo file: /lib/modules/4.3.0-rc4+/build/vmlinux
Try to find probe point from debuginfo.
Matched function: SyS_dup3
found inline addr: 0xffffffff812095c0
Probe point found: SyS_dup3+0
Searching 'oldfd' variable in context.
Converting variable oldfd into trace event.
oldfd type is long int.
found inline addr: 0xffffffff812096d4
Probe point found: SyS_dup2+36
Searching 'oldfd' variable in context.
Failed to find 'oldfd' in this function.
Matched function: SyS_dup3
Probe point found: SyS_dup3+0
Searching 'oldfd' variable in context.
Converting variable oldfd into trace event.
oldfd type is long int.
Matched function: SyS_dup2
Probe point found: SyS_dup2+0
Searching 'oldfd' variable in context.
Converting variable oldfd into trace event.
oldfd type is long int.
Found 4 probe_trace_events.
Opening /sys/kernel/debug/tracing//kprobe_events write=1
Writing event: p:probe/SyS_dup3 _text+2135488 oldfd=%di:s64
Segmentation fault (core dumped)


Here is how the segfault happen:

In following call stack:

add_probe_trace_event
call_probe_finder
probe_point_search_cb
??
dwarf_getfuncs
find_probe_point_by_func
debuginfo__find_probes
debuginfo__find_trace_events
try_to_find_probe_trace_events
convert_to_probe_trace_events
convert_perf_probe_events
perf_add_probe_events

add_probe_trace_event get called and failed due to failure of 
find_variable(). In this case
tev->args is not freed and tev->nargs are positive (1 in this case). 
Error from find_variable()
will be passed down along the call stack through return values until get to
probe_point_search_cb(). The problem is that, probe_point_search_cb 
doesn't return error
if the function name is a glob:

                /* Inlined function: search instances */
                param->retval = die_walk_instances(sp_die,
                                        probe_point_inline_cb, (void *)pf);
                /* This could be a non-existed inline definition */
                if (param->retval == -ENOENT && strisglob(pp->function))
                        param->retval = 0;


So the error won't be transfer to debuginfo__find_trace_events() from 
debuginfo__find_probes(),
and your patch won't take effect.

With patch [1], we set argument number to 0 and frees tev->args. With 
this fix we can save that
tev. Here's the final result:

# ./perf probe 'SyS_dup? oldfd'
Failed to find 'oldfd' in this function.
Added new events:
  probe:SyS_dup3       (on SyS_dup? with oldfd)
  probe:SyS_dup3_1     (on SyS_dup? with oldfd)
  probe:SyS_dup3_2     (on SyS_dup? with oldfd)
  probe:SyS_dup2       (on SyS_dup? with oldfd)

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

    perf record -e probe:SyS_dup2 -aR sleep 1

# PAGER=cat ./perf probe -l
  probe:SyS_dup2       (on SyS_dup2@linux-hydrogen/fs/file.c with oldfd)
  probe:SyS_dup3       (on SyS_dup3@linux-hydrogen/fs/file.c with oldfd)
  probe:SyS_dup3_1     (on SYSC_dup2:12@linux-hydrogen/fs/file.c)
  probe:SyS_dup3_2     (on SyS_dup3@linux-hydrogen/fs/file.c with oldfd)

Perf can't find oldfd in the context of SyS_dup3_1. With [1]'s fix it still
probe at it but doesn't create argument fetcher.

[2]'s fix totally clear this tev. It is incorrect because it forgets to 
adjust
tf->ntevs. With that appended the final result becomes:

# ./perf probe 'SyS_dup? oldfd'
Failed to find 'oldfd' in this function.
Added new events:
  probe:SyS_dup3       (on SyS_dup? with oldfd)
  probe:SyS_dup3_1     (on SyS_dup? with oldfd)
  probe:SyS_dup2       (on SyS_dup? with oldfd)

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

    perf record -e probe:SyS_dup2 -aR sleep 1

# PAGER=cat ./perf probe -l
  probe:SyS_dup2       (on SyS_dup2@linux-hydrogen/fs/file.c with oldfd)
  probe:SyS_dup3       (on SyS_dup3@linux-hydrogen/fs/file.c with oldfd)
  probe:SyS_dup3_1     (on SyS_dup3@linux-hydrogen/fs/file.c with oldfd)

Here only those probe point we can get 'oldfd' is probed.

I suggest [2]'s fix because I think if user provide argument he or she would
get interested with it, and we have already noticed user the argument is 
unabled
to be found in one probe point.

I'll provide another patch based on [2]. Please have a look.

Thank you.

[1] 
http://lkml.kernel.org/g/1444826502-49291-13-git-send-email-wangnan0@xxxxxxxxxx
[2] 
http://lkml.kernel.org/g/1447237183-106610-1-git-send-email-wangnan0@xxxxxxxxxx

On 2015/11/11 22:38, Masami Hiramatsu wrote:
> Fix memory leaking on failure path in debuginfo__find_trace_events()
> which frees an array of probe_trace_events but doesn't clear all
> allocated sub-structures and strings.
> So, before doing zfree(tevs), clear all the array elements which
> can have allocated resources.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> Reported-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Zefan Li <lizefan@xxxxxxxxxx>
> Cc: pi3orama@xxxxxxx
> ---
>   tools/perf/util/probe-finder.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index bd8f03d..63993d7 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1246,7 +1246,7 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>   	struct trace_event_finder tf = {
>   			.pf = {.pev = pev, .callback = add_probe_trace_event},
>   			.max_tevs = probe_conf.max_probes, .mod = dbg->mod};
> -	int ret;
> +	int ret, i;
>   
>   	/* Allocate result tevs array */
>   	*tevs = zalloc(sizeof(struct probe_trace_event) * tf.max_tevs);
> @@ -1258,6 +1258,8 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>   
>   	ret = debuginfo__find_probes(dbg, &tf.pf);
>   	if (ret < 0) {
> +		for (i = 0; i < tf.ntevs; i++)
> +			clear_probe_trace_event(&tf.tevs[i]);
>   		zfree(tevs);
>   		return ret;
>   	}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/