Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932117AbbKMIju (ORCPT ); Fri, 13 Nov 2015 03:39:50 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:1211 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750878AbbKMIjt (ORCPT ); Fri, 13 Nov 2015 03:39:49 -0500 Message-ID: <5645A0F6.1080501@huawei.com> Date: Fri, 13 Nov 2015 16:36:06 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Masami Hiramatsu , CC: Arnaldo Carvalho de Melo , Zefan Li , Subject: Re: [PATCH perf/core ] [BUGFIX] perf probe: Fix memory leaking on faiulre by clearing all probe_trace_events References: <20151111143823.6275.73467.stgit@localhost.localdomain> In-Reply-To: <20151111143823.6275.73467.stgit@localhost.localdomain> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090205.5645A1CF.000F,ss=1,re=0.000,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2011-05-27 18:58:46 X-Mirapoint-Loop-Id: e6e443480088429e63dc6b3830518447 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6701 Lines: 192 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@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 > Reported-by: Wang Nan > Cc: Arnaldo Carvalho de Melo > Cc: Zefan Li > Cc: pi3orama@163.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; > } > -- 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/