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

From: Google
Date: Sun Nov 10 2024 - 23:47:20 EST


On Sat, 9 Nov 2024 02:19:08 +0800
Li Huafei <lihuafei1@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>