Re: [PATCH V2] tools/perf: Pick the correct dwarf die while adding probe point for a function

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

On Tue, 25 Feb 2025 18:00:42 +0530
Athira Rajeev <atrajeev@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) <mhiramat@kernel.org>

Thank you!

> Signed-off-by: Athira Rajeev <atrajeev@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) <mhiramat@kernel.org>