Message-ID: <50399556C9727B4D88A595C8584AAB3752618DA1@GSjpTKYDCembx32.service.hitachi.net>
Date:	Sat, 14 Nov 2015 04:47:43 +0000
From:	平松雅巳 / HIRAMATU，MASAMI 
	<masami.hiramatsu.pt@...achi.com>
To:	"'Wangnan (F)'" <wangnan0@...wei.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Zefan Li <lizefan@...wei.com>,
	"pi3orama@....com" <pi3orama@....com>
Subject: RE: Re: [PATCH perf/core ] [BUGFIX] perf probe: Fix memory
 leaking on faiulre by clearing all probe_trace_events

From: Wangnan (F) [mailto:wangnan0@...wei.com]
>
>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@...ux-hydrogen/fs/file.c with oldfd)
>   probe:SyS_dup3       (on SyS_dup3@...ux-hydrogen/fs/file.c with oldfd)
>   probe:SyS_dup3_1     (on SYSC_dup2:12@...ux-hydrogen/fs/file.c)
>   probe:SyS_dup3_2     (on SyS_dup3@...ux-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@...ux-hydrogen/fs/file.c with oldfd)
>   probe:SyS_dup3       (on SyS_dup3@...ux-hydrogen/fs/file.c with oldfd)
>   probe:SyS_dup3_1     (on SyS_dup3@...ux-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.

OK, this may allow user to probe only where the variables can be accessed.
I also think we'd better offer another option to fail all probe if one of
them cannot fulfill the required arguments so that they can decide not to
trace it or change probe point.

And at least this error message 
>Failed to find 'oldfd' in this function.
should be fixed ...

Thanks!

>
>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@huawei.com
>[2]
>http://lkml.kernel.org/g/1447237183-106610-1-git-send-email-wangnan0@huawei.com
>
>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@...achi.com>
>> Reported-by: Wang Nan <wangnan0@...wei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
>> Cc: Zefan Li <lizefan@...wei.com>
>> Cc: pi3orama@....com
>> ---
>>   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;
>>   	}
>>
>

Powered by blists - more mailing lists