Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 23:56:11 +0900
From:   Masami Hiramatsu <mhiramat@kernel.org>
To:     Jiri Olsa <jolsa@kernel.org>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, lkml <linux-kernel@vger.kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCHv4 bpf-next 2/5] ftrace: Add ftrace_lookup_symbols
 function
Message-Id: <20220429235611.4d14d87efc0b6189dadfe3b1@kernel.org>
In-Reply-To: <20220428201207.954552-3-jolsa@kernel.org>
References: <20220428201207.954552-1-jolsa@kernel.org>
        <20220428201207.954552-3-jolsa@kernel.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 28 Apr 2022 22:12:04 +0200
Jiri Olsa <jolsa@kernel.org> wrote:

> Adding ftrace_lookup_symbols function that resolves array of symbols
> with single pass over kallsyms.
> 
> The user provides array of string pointers with count and pointer to
> allocated array for resolved values.
> 
>   int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt,
>                             unsigned long *addrs)
> 
> It iterates all kallsyms symbols and tries to loop up each in provided
> symbols array with bsearch. The symbols array needs to be sorted by
> name for this reason.
> 
> We also check each symbol to pass ftrace_location, because this API
> will be used for fprobe symbols resolving.

This looks good to me.

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

Thanks!

> 
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/ftrace.h |  6 ++++
>  kernel/kallsyms.c      |  1 +
>  kernel/trace/ftrace.c  | 62 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 4816b7e11047..820500430eae 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -303,6 +303,8 @@ int unregister_ftrace_function(struct ftrace_ops *ops);
>  extern void ftrace_stub(unsigned long a0, unsigned long a1,
>  			struct ftrace_ops *op, struct ftrace_regs *fregs);
>  
> +
> +int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
>  #else /* !CONFIG_FUNCTION_TRACER */
>  /*
>   * (un)register_ftrace_function must be a macro since the ops parameter
> @@ -313,6 +315,10 @@ extern void ftrace_stub(unsigned long a0, unsigned long a1,
>  static inline void ftrace_kill(void) { }
>  static inline void ftrace_free_init_mem(void) { }
>  static inline void ftrace_free_mem(struct module *mod, void *start, void *end) { }
> +static inline int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif /* CONFIG_FUNCTION_TRACER */
>  
>  struct ftrace_func_entry {
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fdfd308bebc4..fbdf8d3279ac 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -29,6 +29,7 @@
>  #include <linux/compiler.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/bsearch.h>
>  
>  /*
>   * These will be re-linked against their real values
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4f1d2f5e7263..07d87c7a525d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7964,3 +7964,65 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
>  	mutex_unlock(&ftrace_lock);
>  	return ret;
>  }
> +
> +static int symbols_cmp(const void *a, const void *b)
> +{
> +	const char **str_a = (const char **) a;
> +	const char **str_b = (const char **) b;
> +
> +	return strcmp(*str_a, *str_b);
> +}
> +
> +struct kallsyms_data {
> +	unsigned long *addrs;
> +	const char **syms;
> +	size_t cnt;
> +	size_t found;
> +};
> +
> +static int kallsyms_callback(void *data, const char *name,
> +			     struct module *mod, unsigned long addr)
> +{
> +	struct kallsyms_data *args = data;
> +
> +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> +		return 0;
> +
> +	addr = ftrace_location(addr);
> +	if (!addr)
> +		return 0;
> +
> +	args->addrs[args->found++] = addr;
> +	return args->found == args->cnt ? 1 : 0;
> +}
> +
> +/**
> + * ftrace_lookup_symbols - Lookup addresses for array of symbols
> + *
> + * @sorted_syms: array of symbols pointers symbols to resolve,
> + * must be alphabetically sorted
> + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> + * @addrs: array for storing resulting addresses
> + *
> + * 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.
> + *
> + * This function returns 0 if all provided symbols are found,
> + * -ESRCH otherwise.
> + */
> +int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
> +{
> +	struct kallsyms_data args;
> +	int err;
> +
> +	args.addrs = addrs;
> +	args.syms = sorted_syms;
> +	args.cnt = cnt;
> +	args.found = 0;
> +	err = kallsyms_on_each_symbol(kallsyms_callback, &args);
> +	if (err < 0)
> +		return err;
> +	return args.found == args.cnt ? 0 : -ESRCH;
> +}
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>