Re: [PATCHv2 bpf-next 2/6] ftrace: Keep the resolved addr in kallsyms_callback

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

On Thu, 11 Aug 2022 11:15:22 +0200
Jiri Olsa <jolsa@kernel.org> wrote:

> Keeping the resolved 'addr' in kallsyms_callback, instead of taking
> ftrace_location value, because we depend on symbol address in the
> cookie related code.
> 
> With CONFIG_X86_KERNEL_IBT option the ftrace_location value differs
> from symbol address, which screwes the symbol address cookies matching.
> 
> There are 2 users of this function:
> - bpf_kprobe_multi_link_attach
>     for which this fix is for
> 
> - get_ftrace_locations
>     which is used by register_fprobe_syms
> 
>     this function needs to get symbols resolved to addresses,
>     but does not need 'ftrace location addresses' at this point
>     there's another ftrace location translation in the path done
>     by ftrace_set_filter_ips call:
> 
>      register_fprobe_syms
>        addrs = get_ftrace_locations
> 
>        register_fprobe_ips(addrs)
>          ...
>          ftrace_set_filter_ips
>            ...
>              __ftrace_match_addr
>                ip = ftrace_location(ip);
>                ...
> 

Yes, this looks OK for fprobe. I confirmed above.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

One concern was that the caller might expect that the address
must be ftrace_location(), but as far as I can read the function
document, there is no such description.

 * ftrace_lookup_symbols - Lookup addresses for array of symbols
...
 * This function looks up addresses for array of symbols provided in
 * @syms array (must be alphabetically sorted) and stores them in
 * @addrs array, which needs to be big enough to store at least @cnt
 * addresses.

So this change is OK.

Thank you,

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/ftrace.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index bc921a3f7ea8..8a8c90d1a387 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -8268,8 +8268,7 @@ static int kallsyms_callback(void *data, const char *name,
>  	if (args->addrs[idx])
>  		return 0;
>  
> -	addr = ftrace_location(addr);
> -	if (!addr)
> +	if (!ftrace_location(addr))
>  		return 0;
>  
>  	args->addrs[idx] = addr;
> -- 
> 2.37.1
> 


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