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!
> 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>
