Re: [BUGFIX PATCH 1/6] perf/probe: Fix wrong address verification

From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [BUGFIX PATCH 1/6] perf/probe: Fix wrong address verification
Date: Fri, 25 Oct 2019 09:14:48 -0300	[thread overview]
Message-ID: <20191025121448.GC23511@kernel.org> (raw)
In-Reply-To: <157199318513.8075.10463906803299647907.stgit@devnote2>

Em Fri, Oct 25, 2019 at 05:46:25PM +0900, Masami Hiramatsu escreveu:
> Since there are some DIE which has only ranges instead of the
> combination of entrypc/highpc, address verification must use
> dwarf_haspc() instead of dwarf_entrypc/dwarf_highpc.
> 
> Also, the ranges only DIE will have a partial code in different
> section (e.g. unlikely code will be in text.unlikely as "FUNC.cold"
> symbol). In that case, we can not use dwarf_entrypc() or
> die_entrypc(), because the offset from original DIE can be
> a minus value.
> 
> Instead, this simply gets the symbol and offset from symtab.
> 
> Without this patch;
>   # tools/perf/perf probe -D clear_tasks_mm_cpumask:1
>   Failed to get entry address of clear_tasks_mm_cpumask
>     Error: Failed to add events.
> 
> And with this patch
>   # tools/perf/perf probe -D clear_tasks_mm_cpumask:1
>   p:probe/clear_tasks_mm_cpumask clear_tasks_mm_cpumask+0
>   p:probe/clear_tasks_mm_cpumask_1 clear_tasks_mm_cpumask+5
>   p:probe/clear_tasks_mm_cpumask_2 clear_tasks_mm_cpumask+8
>   p:probe/clear_tasks_mm_cpumask_3 clear_tasks_mm_cpumask+16
>   p:probe/clear_tasks_mm_cpumask_4 clear_tasks_mm_cpumask+82

Ok, so this just asks for the definition, but doesn't try to actually
_use_ it, which I did and it fails:

[root@quaco tracebuffer]# perf probe -D clear_tasks_mm_cpumask:1
p:probe/clear_tasks_mm_cpumask _text+919968
p:probe/clear_tasks_mm_cpumask_1 _text+919973
p:probe/clear_tasks_mm_cpumask_2 _text+919976
[root@quaco tracebuffer]#
[root@quaco tracebuffer]# perf probe clear_tasks_mm_cpumask
Probe point 'clear_tasks_mm_cpumask' not found.
  Error: Failed to add events.
[root@quaco tracebuffer]#

So I'll tentatively continue to apply the other patches in this series,
maybe one of them will fix this.

- Arnaldo

> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Fixes: 576b523721b7 ("perf probe: Fix probing symbols with optimization suffix")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/probe-finder.c |   32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index cd9f95e5044e..2b6513e5725c 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -604,38 +604,26 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
>  				  const char *function,
>  				  struct probe_trace_point *tp)
>  {
> -	Dwarf_Addr eaddr, highaddr;
> +	Dwarf_Addr eaddr;
>  	GElf_Sym sym;
>  	const char *symbol;
>  
>  	/* Verify the address is correct */
> -	if (dwarf_entrypc(sp_die, &eaddr) != 0) {
> -		pr_warning("Failed to get entry address of %s\n",
> -			   dwarf_diename(sp_die));
> -		return -ENOENT;
> -	}
> -	if (dwarf_highpc(sp_die, &highaddr) != 0) {
> -		pr_warning("Failed to get end address of %s\n",
> -			   dwarf_diename(sp_die));
> -		return -ENOENT;
> -	}
> -	if (paddr > highaddr) {
> -		pr_warning("Offset specified is greater than size of %s\n",
> +	if (!dwarf_haspc(sp_die, paddr)) {
> +		pr_warning("Specified offset is out of %s\n",
>  			   dwarf_diename(sp_die));
>  		return -EINVAL;
>  	}
>  
> -	symbol = dwarf_diename(sp_die);
> +	/* Try to get actual symbol name from symtab */
> +	symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
>  	if (!symbol) {
> -		/* Try to get the symbol name from symtab */
> -		symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> -		if (!symbol) {
> -			pr_warning("Failed to find symbol at 0x%lx\n",
> -				   (unsigned long)paddr);
> -			return -ENOENT;
> -		}
> -		eaddr = sym.st_value;
> +		pr_warning("Failed to find symbol at 0x%lx\n",
> +			   (unsigned long)paddr);
> +		return -ENOENT;
>  	}
> +	eaddr = sym.st_value;
> +
>  	tp->offset = (unsigned long)(paddr - eaddr);
>  	tp->address = (unsigned long)paddr;
>  	tp->symbol = strdup(symbol);

-- 

- Arnaldo