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

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Sep 08, 2022 at 09:35:51PM +0900, Masami Hiramatsu wrote:
> 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,

thanks for review, I'll rebase and new version

jirka

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