> On 26 Feb 2025, at 6:35 PM, Masami Hiramatsu (Google) <mhira...@kernel.org>
> wrote:
>
> On Tue, 25 Feb 2025 18:00:42 +0530
> Athira Rajeev <atraj...@linux.ibm.com> wrote:
>
>> Perf probe on vfs_fstatat fails as below on a powerpc system
>>
>> ./perf probe -nf --max-probes=512 -a 'vfs_fstatat $params'
>> Segmentation fault (core dumped)
>>
>> This is observed while running perftool-testsuite_probe testcase.
>>
>> While running with verbose, its observed that segfault happens
>> at:
>>
>> synthesize_probe_trace_arg ()
>> synthesize_probe_trace_command ()
>> probe_file.add_event ()
>> apply_perf_probe_events ()
>> __cmd_probe ()
>> cmd_probe ()
>> run_builtin ()
>> handle_internal_command ()
>> main ()
>>
>> Code in synthesize_probe_trace_arg() access a null value and results in
>> segfault. Data structure which is null:
>> struct probe_trace_arg arg->value
>>
>> We are hitting a case where arg->value is null in probe point:
>> "vfs_fstatat $params". This is happening since 'commit e896474fe485
>> ("getname_maybe_null() - the third variant of pathname copy-in")'
>> Before the commit, probe point for vfs_fstatat was getting added only
>> for one location:
>>
>> Writing event: p:probe/vfs_fstatat _text+6345404 dfd=%gpr3:s32
>> filename=%gpr4:x64 stat=%gpr5:x64 flags=%gpr6:s32
>>
>> With this change, vfs_fstatat code is inlined for other locations in the
>> code:
>> Probe point found: __do_sys_lstat64+48
>> Probe point found: __do_sys_stat64+48
>> Probe point found: __do_sys_newlstat+48
>> Probe point found: __do_sys_newstat+48
>> Probe point found: vfs_fstatat+0
>>
>> When trying to find matching dwarf information entry (DIE)
>> from the debuginfo, the code incorrectly picks DIE which is
>> not referring to vfs_fstatat. Snippet from dwarf entry in vmlinux
>> debuginfo file.
>>
>> The main abstract die is:
>> <1><4214883>: Abbrev Number: 147 (DW_TAG_subprogram)
>> <4214885> DW_AT_external : 1
>> <4214885> DW_AT_name : (indirect string, offset: 0x17b9f3):
>> vfs_fstatat
>>
>> With formal parameters:
>> <2><4214896>: Abbrev Number: 51 (DW_TAG_formal_parameter)
>> <4214897> DW_AT_name : dfd
>> <2><42148a3>: Abbrev Number: 23 (DW_TAG_formal_parameter)
>> <42148a4> DW_AT_name : (indirect string, offset: 0x8fda9):
>> filename
>> <2><42148b0>: Abbrev Number: 23 (DW_TAG_formal_parameter)
>> <42148b1> DW_AT_name : (indirect string, offset: 0x16bd9c): stat
>> <2><42148bd>: Abbrev Number: 23 (DW_TAG_formal_parameter)
>> <42148be> DW_AT_name : (indirect string, offset: 0x39832b): flags
>>
>> While collecting variables/parameters for a probe point, the function
>> copy_variables_cb() also looks at dwarf debug entries based on the
>> instruction address. Snippet
>>
>> if (dwarf_haspc(die_mem, vf->pf->addr))
>> return DIE_FIND_CB_CONTINUE;
>> else
>> return DIE_FIND_CB_SIBLING;
>>
>> But incase of inlined function instance for vfs_fstatat, there are two
>> entries which has the instruction address entry point as same.
>>
>> Instance 1: which is for vfs_fstatat and DW_AT_abstract_origin points to
>> 0x4214883 (reference above for main abstract die)
>>
>> <3><42131fa>: Abbrev Number: 59 (DW_TAG_inlined_subroutine)
>> <42131fb> DW_AT_abstract_origin: <0x4214883>
>> <42131ff> DW_AT_entry_pc : 0xc00000000062b1e0
>>
>> Instance 2: which is not for vfs_fstatat but for getname
>>
>> <5><4213270>: Abbrev Number: 39 (DW_TAG_inlined_subroutine)
>> <4213271> DW_AT_abstract_origin: <0x4215b6b>
>> <4213275> DW_AT_entry_pc : 0xc00000000062b1e0
>>
>> But the copy_variables_cb() continues to add parameters from second
>> instance also based on the dwarf_haspc() check. This results in
>> formal parameters for getname also appended to params. But while
>> filling in the args->value for these parameters, since these args
>> are not part of dwarf with offset "42131fa". Hence value will be
>> null. This incorrect args results in segfault when value field is
>> accessed.
>>
>> Save the dwarf dieoffset of the actual DW_TAG_subprogram as part of
>> "struct probe_finder". In copy_variables_cb(), include check to make
>> sure the DW_AT_abstract_origin points to the correct entry if the
>> dwarf_haspc() matches the instruction address.
>>
>
> Good catch! and this looks good to me :-)
>
> Acked-by: Masami Hiramatsu (Google) <mhira...@kernel.org>
>
> Thank you!
Hi Masami
Thanks for checking the patch.
Namhyung,
Thanks for having this patch in tmp.perf-tools-next.
James had sent reviewed-by for the first version
https://lore.kernel.org/linux-perf-users/964f60e0-fe88-48c8-87c3-a6ea2f3ca...@linaro.org/
Since I added a change in V2 to directly use the die offset, I didn’t carry his
reviewed-by in V2 to make sure he is fine with V2.
Please add reviewed-by from James if he is ok with V2 :)
Thanks
Athira
>
>> Signed-off-by: Athira Rajeev <atraj...@linux.ibm.com>
>> ---
>> Changelog:
>> v1 -> v2:
>> Instead of saving the Dwarf_Die as part of "struct probe_finder",
>> save the dwarf dieoffset. We only need offset while comparing to see
>> if DW_AT_abstract_origin points to correct entry
>> Suggested in review by Namhyung.
>>
>> tools/perf/util/probe-finder.c | 21 ++++++++++++++++++---
>> tools/perf/util/probe-finder.h | 1 +
>> 2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
>> index 1e769b68da37..3cc7c40f5097 100644
>> --- a/tools/perf/util/probe-finder.c
>> +++ b/tools/perf/util/probe-finder.c
>> @@ -973,6 +973,7 @@ static int probe_point_search_cb(Dwarf_Die *sp_die, void
>> *data)
>> pr_debug("Matched function: %s [%lx]\n", dwarf_diename(sp_die),
>> (unsigned long)dwarf_dieoffset(sp_die));
>> pf->fname = fname;
>> + pf->abstrace_dieoffset = dwarf_dieoffset(sp_die);
>> if (pp->line) { /* Function relative line */
>> dwarf_decl_line(sp_die, &pf->lno);
>> pf->lno += pp->line;
>> @@ -1179,6 +1180,8 @@ static int copy_variables_cb(Dwarf_Die *die_mem, void
>> *data)
>> struct local_vars_finder *vf = data;
>> struct probe_finder *pf = vf->pf;
>> int tag;
>> + Dwarf_Attribute attr;
>> + Dwarf_Die var_die;
>>
>> tag = dwarf_tag(die_mem);
>> if (tag == DW_TAG_formal_parameter ||
>> @@ -1196,10 +1199,22 @@ static int copy_variables_cb(Dwarf_Die *die_mem,
>> void *data)
>> }
>> }
>>
>> - if (dwarf_haspc(die_mem, vf->pf->addr))
>> + if (dwarf_haspc(die_mem, vf->pf->addr)) {
>> + /*
>> + * when DW_AT_entry_pc contains instruction address,
>> + * also check if the DW_AT_abstract_origin of die_mem
>> + * points to correct die.
>> + */
>> + if (dwarf_attr(die_mem, DW_AT_abstract_origin, &attr)) {
>> + dwarf_formref_die(&attr, &var_die);
>> + if (pf->abstrace_dieoffset != dwarf_dieoffset(&var_die))
>> + goto out;
>> + }
>> return DIE_FIND_CB_CONTINUE;
>> - else
>> - return DIE_FIND_CB_SIBLING;
>> + }
>> +
>> +out:
>> + return DIE_FIND_CB_SIBLING;
>> }
>>
>> static int expand_probe_args(Dwarf_Die *sc_die, struct probe_finder *pf,
>> diff --git a/tools/perf/util/probe-finder.h b/tools/perf/util/probe-finder.h
>> index dcf6cc1e1cbe..ecd6d937c592 100644
>> --- a/tools/perf/util/probe-finder.h
>> +++ b/tools/perf/util/probe-finder.h
>> @@ -63,6 +63,7 @@ struct probe_finder {
>> const char *fname; /* Real file name */
>> Dwarf_Die cu_die; /* Current CU */
>> Dwarf_Die sp_die;
>> + Dwarf_Off abstrace_dieoffset;
>> struct intlist *lcache; /* Line cache for lazy match */
>>
>> /* For variable searching */
>> --
>> 2.43.5
>>
>
>
> --
> Masami Hiramatsu (Google) <mhira...@kernel.org>
