Re: [PATCH v2] libbpf: kprobe.multi: Filter with available_filter_functions_addrs

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

在 2023/6/27 07:46, Andrii Nakryiko 写道:
> On Sat, Jun 24, 2023 at 6:14 PM Jackie Liu <liu.yun@linux.dev> 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_addrs check first, if failed, fallback to
>> kallsyms.
>>
> 
> This is good, but it also doesn't address the original problem. On
> older kernels that don't have available_filter_functions_addrs, we'll
> still be trying to attach to non-attachable kprobes?
> 
> So I think we'll still need to add available_filter_functions-based
> filtering on top of kallsyms. I guess we'll have to collect all
> symbols+addr from kallsyms, sort them, then go over
> available_filter_functions and do binary search. If element is not
> found, we'll mark it for removal. Then last pass to filter out marked
> entries to keep only addrs to be passed to kernel?

The first patch I submitted was to re-search available_filter_functions
in the case of kallsyms matching. The link is at:

https://lore.kernel.org/all/20230523132547.94384-1-liu.yun@linux.dev/,

but it is very slow and takes about 5s to start. If necessary, I can
continue to add it to the V3 patch. Maybe you have other better
algorithms?

-- 
Jackie Liu

> 
> 
>> Here is the test eBPF program [1].
>> [1] https://github.com/JackieLiu1/ketones/tree/master/src/funccount
>>
>> Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>> ---
>>   tools/lib/bpf/libbpf.c | 81 ++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 75 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index a27f6e9ccce7..fca5d2e412c5 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -10107,6 +10107,12 @@ static const char *tracefs_uprobe_events(void)
>>          return use_debugfs() ? DEBUGFS"/uprobe_events" : TRACEFS"/uprobe_events";
>>   }
>>
>> +static const char *tracefs_available_filter_functions_addrs(void)
>> +{
>> +       return use_debugfs() ? DEBUGFS"/available_filter_functions_addrs" :
>> +                              TRACEFS"/available_filter_functions_addrs";
>> +}
>> +
>>   static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
>>                                           const char *kfunc_name, size_t offset)
>>   {
>> @@ -10422,9 +10428,8 @@ struct kprobe_multi_resolve {
>>          size_t cnt;
>>   };
>>
>> -static int
>> -resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>> -                       const char *sym_name, void *ctx)
>> +static int ftrace_resolve_kprobe_multi_cb(unsigned long long sym_addr,
>> +                                         const char *sym_name, void *ctx)
>>   {
>>          struct kprobe_multi_resolve *res = ctx;
>>          int err;
>> @@ -10441,6 +10446,63 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>>          return 0;
>>   }
>>
>> +static int
>> +kallsyms_resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>> +                                const char *sym_name, void *ctx)
>> +{
>> +       return ftrace_resolve_kprobe_multi_cb(sym_addr, sym_name, ctx);
>> +}
>> +
>> +typedef int (*available_kprobe_cb_t)(unsigned long long sym_addr,
>> +                                    const char *sym_name, void *ctx);
>> +
>> +static int
>> +libbpf_available_kprobes_parse(available_kprobe_cb_t cb, void *ctx)
>> +{
>> +       unsigned long long sym_addr;
>> +       char sym_name[256];
>> +       FILE *f;
>> +       int ret, err = 0;
>> +       const char *available_path = tracefs_available_filter_functions_addrs();
>> +
>> +       f = fopen(available_path, "r");
>> +       if (!f) {
>> +               err = -errno;
>> +               pr_warn("failed to open %s, fallback to /proc/kallsyms.\n",
>> +                       available_path);
> 
> if this is expected to happen, we shouldn't pr_warn() and pollute log output
> 
>> +               return err;
>> +       }
>> +
>> +       while (true) {
>> +               ret = fscanf(f, "%llx %255s%*[^\n]\n", &sym_addr, sym_name);
>> +               if (ret == EOF && feof(f))
>> +                       break;
>> +               if (ret != 2) {
>> +                       pr_warn("failed to read available kprobe entry: %d\n",
>> +                               ret);
>> +                       err = -EINVAL;
>> +                       break;
>> +               }
>> +
>> +               err = cb(sym_addr, sym_name, ctx);
>> +               if (err)
>> +                       break;
>> +       }
>> +
>> +       fclose(f);
>> +       return err;
>> +}
>> +
>> +static void kprobe_multi_resolve_reinit(struct kprobe_multi_resolve *res)
>> +{
>> +       free(res->addrs);
>> +
>> +       /* reset to zero, when fallback */
>> +       res->cap = 0;
>> +       res->cnt = 0;
>> +       res->addrs = NULL;
>> +}
>> +
>>   struct bpf_link *
>>   bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>>                                        const char *pattern,
>> @@ -10477,9 +10539,16 @@ 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_reinit(&res);
> 
> instead of try, fail, try something else approach, can we check that
> tracefs_available_filter_functions_addrs() exists and is readable, and
> if yes -- commit to it. If something fails -- to bad. If it doesn't
> exist, fallback to kallsyms. And we won't need ugly
> kprobe_multi_resolve_reinit() hack.

Sounds good.

> 
>> +                       err = libbpf_kallsyms_parse(kallsyms_resolve_kprobe_multi_cb,
>> +                                                   &res);
>> +                       if (err)
>> +                               goto error;
>> +               }
>>                  if (!res.cnt) {
>>                          err = -ENOENT;
>>                          goto error;
>> --
>> 2.25.1
>>
>>