Re: [PATCH v6 RESEND] libbpf: kprobe.multi: Filter with available_filter_functions

["Jackie Liu" <liu.yun@linux.dev>]

Hi Jiri.


May 29, 2023 9:23 PM, "Jiri Olsa" <olsajiri@gmail.com> 写到:

> On Fri, May 26, 2023 at 11:50:26PM +0800, Jackie Liu wrote:
> 
>> From: Jackie Liu <liuyun01@kylinos.cn>
>> 
>> When using regular expression matching with "kprobe multi", it scans all
>> the functions under "/proc/kallsyms" that can be matched. However, not all
>> of them can be traced by kprobe.multi. If any one of the functions fails
>> to be traced, it will result in the failure of all functions. The best
>> approach is to filter out the functions that cannot be traced to ensure
>> proper tracking of the functions.
>> 
>> Use available_filter_functions check first, if failed, fallback to
>> kallsyms.
>> 
>> Here is the test eBPF program [1].
>> [1] https://github.com/JackieLiu1/ketones/commit/a9e76d1ba57390e533b8b3eadde97f7a4535e867
>> 
>> Suggested-by: Jiri Olsa <olsajiri@gmail.com>
>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>> ---
>> tools/lib/bpf/libbpf.c | 100 ++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 93 insertions(+), 7 deletions(-)
>> 
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index ad1ec893b41b..0914b7e98e30 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -10106,6 +10106,12 @@ static const char *tracefs_uprobe_events(void)
>> return use_debugfs() ? DEBUGFS"/uprobe_events" : TRACEFS"/uprobe_events";
>> }
>> 
>> +static const char *tracefs_available_filter_functions(void)
>> +{
>> + return use_debugfs() ? DEBUGFS"/available_filter_functions" :
>> + TRACEFS"/available_filter_functions";
>> +}
>> +
>> static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
>> const char *kfunc_name, size_t offset)
>> {
>> @@ -10417,13 +10423,14 @@ static bool glob_match(const char *str, const char *pat)
>> struct kprobe_multi_resolve {
>> const char *pattern;
>> unsigned long *addrs;
>> + const char **syms;
>> size_t cap;
>> size_t cnt;
>> };
>> 
>> static int
>> -resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>> - const char *sym_name, void *ctx)
>> +kallsyms_resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>> + const char *sym_name, void *ctx)
>> {
>> struct kprobe_multi_resolve *res = ctx;
>> int err;
>> @@ -10440,6 +10447,77 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>> return 0;
>> }
>> 
>> +static int ftrace_resolve_kprobe_multi_cb(const char *sym_name, void *ctx)
>> +{
>> + struct kprobe_multi_resolve *res = ctx;
>> + int err;
>> + char *name;
>> +
>> + if (!glob_match(sym_name, res->pattern))
>> + return 0;
>> +
>> + err = libbpf_ensure_mem((void **) &res->syms, &res->cap,
>> + sizeof(const char *), res->cnt + 1);
>> + if (err)
>> + return err;
>> +
>> + name = strdup(sym_name);
>> + if (!name)
>> + return -errno;
>> +
>> + res->syms[res->cnt++] = name;
>> + return 0;
>> +}
>> +
>> +typedef int (*available_kprobe_cb_t)(const char *sym_name, void *ctx);
>> +
>> +static int
>> +libbpf_available_kprobes_parse(available_kprobe_cb_t cb, void *ctx)
>> +{
>> + char sym_name[256];
>> + FILE *f;
>> + int ret, err = 0;
>> + const char *available_path = tracefs_available_filter_functions();
>> +
>> + f = fopen(available_path, "r");
>> + if (!f) {
>> + err = -errno;
>> + pr_warn("failed to open %s, fallback to /proc/kallsyms.\n",
>> + available_path);
>> + return err;
>> + }
>> +
>> + while (true) {
>> + ret = fscanf(f, "%255s%*[^\n]\n", sym_name);
>> + if (ret == EOF && feof(f))
>> + break;
>> + if (ret != 1) {
>> + pr_warn("failed to read available kprobe entry: %d\n",
>> + ret);
>> + err = -EINVAL;
>> + break;
>> + }
>> +
>> + err = cb(sym_name, ctx);
>> + if (err)
>> + break;
>> + }
>> +
>> + fclose(f);
>> + return err;
>> +}
>> +
>> +static void kprobe_multi_resolve_free(struct kprobe_multi_resolve *res)
>> +{
>> + while (res->syms && res->cnt)
>> + free((char *)res->syms[--res->cnt]);
>> +
>> + free(res->syms);
>> + free(res->addrs);
> 
> I think we also need to zero the res->syms pointer, so the final
> kprobe_multi_resolve_free won't try to release it again
> perhaps use zfree for both syms and addrs
> 
> other than this it looks ok to me:
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thank you for your patient guidance and best wishes to you.

You are right, I finally decided to initialize all related variables to 0. We may indeed guarantee 
that res->cnt will become 0 now, but we will change the logic in the future to miss this.

Thanks again.

-- 
BR, Jackie Liu

> 
> thanks,
> jirka
> 
>> + /* reset cap to zero, when fallback */
>> + res->cap = 0;
>> +}
>> +
>> struct bpf_link *
>> bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>> const char *pattern,
>> @@ -10476,13 +10554,21 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>> return libbpf_err_ptr(-EINVAL);
>> 
>> if (pattern) {
>> - err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
>> - if (err)
>> - goto error;
>> + err = libbpf_available_kprobes_parse(ftrace_resolve_kprobe_multi_cb,
>> + &res);
>> + if (err) {
>> + /* fallback to kallsyms */
>> + kprobe_multi_resolve_free(&res);
>> + err = libbpf_kallsyms_parse(kallsyms_resolve_kprobe_multi_cb,
>> + &res);
>> + if (err)
>> + goto error;
>> + }
>> if (!res.cnt) {
>> err = -ENOENT;
>> goto error;
>> }
>> + syms = res.syms;
>> addrs = res.addrs;
>> cnt = res.cnt;
>> }
>> @@ -10511,12 +10597,12 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>> goto error;
>> }
>> link->fd = link_fd;
>> - free(res.addrs);
>> + kprobe_multi_resolve_free(&res);
>> return link;
>> 
>> error:
>> free(link);
>> - free(res.addrs);
>> + kprobe_multi_resolve_free(&res);
>> return libbpf_err_ptr(err);
>> }
>> 
>> --
>> 2.25.1