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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Sat, 9 Nov 2024 02:19:08 +0800
Li Huafei <lihuafei1@huawei.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@kernel/sched/core.c)
>     probe:schedule_1     (on schedule+8@kernel/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@kernel/sched/core.c)
>     probe:schedule_1     (on schedule:-6751@kernel/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.

Thank you,

> Signed-off-by: Li Huafei <lihuafei1@huawei.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
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>