linux-kernel - Re: [PATCH 1/2] perf probe: Reset old content before processing the next event string

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <24c76d15-43d1-3612-be61-44d34fb3ac9e@huawei.com>
Date: Tue, 12 Nov 2024 09:47:09 +0800
From: Li Huafei <lihuafei1@...wei.com>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
CC: <acme@...nel.org>, <peterz@...radead.org>, <mingo@...hat.com>,
	<namhyung@...nel.org>, <mark.rutland@....com>,
	<alexander.shishkin@...ux.intel.com>, <jolsa@...nel.org>,
	<irogers@...gle.com>, <adrian.hunter@...el.com>, <kan.liang@...ux.intel.com>,
	<dima@...retsauce.net>, <aleksander.lobakin@...el.com>,
	<linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] perf probe: Reset old content before processing the
 next event string



On 2024/11/11 12:47, Masami Hiramatsu (Google) wrote:
> On Sat, 9 Nov 2024 02:19:08 +0800
> Li Huafei <lihuafei1@...wei.com> wrote:
> 
>> I added two probe events:
>>
>>   # perf probe -f -a schedule+8
>>   Added new event:
>>     probe:schedule       (on schedule+8)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:schedule -aR sleep 1
>>
>>   # perf probe -f -a schedule+20
>>   Added new event:
>>     probe:schedule_1     (on schedule+20)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:schedule_1 -aR sleep 1
>>
>> However, 'perf probe -l' shows the same offset:
>>
>>   # perf probe -l
>>     probe:schedule       (on schedule+8@...nel/sched/core.c)
>>     probe:schedule_1     (on schedule+8@...nel/sched/core.c)
>>
>> __show_perf_probe_events() does not clean up the 'pev' content when
>> parsing the rawlist. If the 'pev->offset' is not set while processing
>> the next probe event string, the offset value of the previous event will
>> be used.  After adding debug information, it was found that indeed there
>> was line number information when processing 'probe:schedule_1', so the
>> offset was not set and used the offset from 'probe:schedule'.
>>
>> To fix this, in the loop that parses the rawlist, reset the contents of
>> 'tev' and 'pev' to ensure it does not affect the next parsing.
>>
>> After the modification,  'perf probe -l' shows the following:
>>
>>   # perf probe -l
>>     probe:schedule       (on schedule+8@...nel/sched/core.c)
>>     probe:schedule_1     (on schedule:-6751@...nel/sched/core.c)
>>
>> Note that 'probe:schedule_1' is displayed with line number, but the line
>> number seem to be incorrect. This issue is independent of the problem
>> fixed by the current patch and will be addressed in the next patch.
>>
> 
> Good catch! But we should do the cleanup in clear_perf_probe_event()
> and clear_probe_trace_event().
> 
>> Fixes: d8f9da240495 ("perf tools: Use zfree() where applicable")
> 
> What we need is to revert this change for above 2 functions, because
> without that, it "clear"s the structure correctly. Current code
> releases allocated fields, but not clear all fields. This can lead
> another bug.


Right, I will make this change in the next version.

Thanks，
Huafei

> 
> Thank you,
> 
>> Signed-off-by: Li Huafei <lihuafei1@...wei.com>
>> ---
>>  tools/perf/util/probe-event.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index a17c9b8a7a79..ec0b11f8d881 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -2695,6 +2695,8 @@ static int __show_perf_probe_events(int fd, bool is_kprobe,
>>  next:
>>  		clear_perf_probe_event(&pev);
>>  		clear_probe_trace_event(&tev);
>> +		memset(&tev, 0, sizeof(tev));
>> +		memset(&pev, 0, sizeof(pev));
>>  		if (ret < 0)
>>  			break;
>>  	}
>> -- 
>> 2.25.1
>>
>>
> 
>