Re: [PATCH] libbpf: resolve kernel function name optimization for kprobe

[Menglong Dong]

On Thu, Jan 12, 2023 at 6:20 PM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 12/01/2023 07:23, Yonghong Song wrote:
> >
> >
> > On 1/9/23 7:11 PM, Menglong Dong wrote:
> >> On Tue, Jan 10, 2023 at 4:29 AM Yonghong Song <yhs@xxxxxxxx> wrote:
> >>>
> >>>
> >>>
> >>> On 1/9/23 1:42 AM, menglong8.dong@xxxxxxxxx wrote:
> >>>> From: Menglong Dong <imagedong@xxxxxxxxxxx>
> >>>>
> >>>> The function name in kernel may be changed by the compiler. For example,
> >>>> the function 'ip_rcv_core' can be compiled to 'ip_rcv_core.isra.0'.
> >>>>
> >>>> This kind optimization can happen in any kernel function. Therefor, we
> >>>> should conside this case.
> >>>>
> >>>> If we failed to attach kprobe with a '-ENOENT', then we can lookup the
> >>>> kallsyms and check if there is a similar function end with '.xxx', and
> >>>> retry.
> >>>
> >>> This might produce incorrect result, so this approach won't work
> >>> for all .isra.0 cases. When a function name is changed from
> >>> <func> to <func>.isra.<num>, it is possible that compiler may have
> >>> make some changes to the arguments, e.g., removing one argument,
> >>> chaning a semantics of argument, etc. if bpf program still
> >>> uses the original function signature, the bpf program may
> >>> produce unexpected result.
> >>
> >> Oops, I wasn't aware of this part. Can we make this function disabled
> >> by default and offer an option to users to enable it? Such as:
> >>
> >>      bpf_object_adapt_sym(struct bpf_object *obj)
> >>
> >> In my case, kernel function rename is common, and I have to
> >> check all functions and do such adaptation before attaching
> >> my kprobe programs, which makes me can't use auto-attach.
> >>
> >> What's more, I haven't seen the arguments change so far, and
> >> maybe it's not a common case?
> >
> > I don't have statistics, but it happens. In general, if you
> > want to attach to a function like <foo>, but it has a variant
> > <foo>.isra.<num>, you probably should check assembly code
> > to ensure the parameter semantics not changed, and then
> > you can attach to kprobe function <foo>.isra.<num>, which
> > I assume current libbpf infrastructure should support it.
> > After you investigate all these <foo>.isra.<num> functions
> > and confirm their argument semantics won't change, you
> > could use kprobe multi to do attachment.
> >
>
> I crunched some numbers on this, and discovered out of ~1600
> .isra/.constprop functions, 76 had a missing argument. The patch series
> at [1] is a rough attempt to get pahole to spot these, and add
> BTF entries for each, where the BTF representation reflects
> reality by skipping optimized-out arguments. So for a function
> like
>
> static int ip6_nh_lookup_table(struct net *net, struct fib6_config *cfg,
>                                const struct in6_addr *gw_addr, u32 tbid,
>                                int flags, struct fib6_result *res);
>
> Examining the BTF representation using pahole from [1], we see
>
> int ip6_nh_lookup_table.isra.0(struct net *net, struct fib6_config *cfg, struct in6_addr *gw_addr, u32 tbid, int flags);
>
> Comparing to the definition, we see the last parameter is missing,
> i.e. the "struct fib6_result *" argument is missing. The calling pattern -
> where the callers have a struct fib6_result on the stack and pass a pointer -
> is reflected in late DWARF info which shows the argument is not actually
> passed as a register, but can be expressed as an offset relative to the current
> function stack (DW_OP_fbreg).
>
> This approach howvever introduces the problem that currently the kernel
> doesn't  allow a "." in a function name. We can fix that, but any BTF encoding
> that introduced optimized functions containing a  "." would have to be opt-in
> via a pahole option, so we do not generate invalid vmlinux BTF for kernels
> without that change.
>
> An alternative approach would be to simply encode .isra functions
> in BTF without the .isra suffix (i.e. using "function_name" not
> "function_name.isra"), only doing the BTF encoding if no arguments were
> optimized out - i.e. if the function signature matches expectations.
> The 76 functions with optimized-out parameters could simply be skipped.
> To me that feels like the simpler approach - it avoids issues
> with function name BTF encoding, and with that sort of model a
> loose-matching kallsyms approach - like that described here - could be used
> for kprobes and fentry/fexit. It also fits with the DWARF representation -
> the .isra suffixes are not present in DWARF representations of the function,
> only in the symbol table and kallsyms, so perhaps BTF should follow suit
> and not add the suffixes. What do you think?

This idea sounds great to me too, which makes the kernel function
consistent to users. As for the functions with optimized-out parameters,
I think that skipping them is acceptable, as they might produce incorrect
results to users.

Thanks!
Menglong Dong

>
> Alan
>
> [1] https://github.com/acmel/dwarves/compare/master...alan-maguire:dwarves:optimized
> >>
> >> (Please just ignore this reply if it doesn't work :/ )
> >>
> >> Thanks!
> >> Menglong Dong
> >>>
> >>>>
> >>>> Signed-off-by: Menglong Dong <imagedong@xxxxxxxxxxx>
> >>>> ---
> >>>>    tools/lib/bpf/libbpf.c | 37 ++++++++++++++++++++++++++++++++++++-
> >>>>    1 file changed, 36 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >>>> index a5c67a3c93c5..fdfb1ca34ced 100644
> >>>> --- a/tools/lib/bpf/libbpf.c
> >>>> +++ b/tools/lib/bpf/libbpf.c
> >>>> @@ -10375,12 +10375,30 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >>>>        return libbpf_err_ptr(err);
> >>>>    }
> >>>>
> >>>> +struct kprobe_resolve {
> >>>> +     char pattern[128];
> >>>> +     char name[128];
> >>>> +};
> >>>> +
> >>>> +static int kprobe_kallsyms_cb(unsigned long long sym_addr, char sym_type,
> >>>> +                           const char *sym_name, void *ctx)
> >>>> +{
> >>>> +     struct kprobe_resolve *res = ctx;
> >>>> +
> >>>> +     if (!glob_match(sym_name, res->pattern))
> >>>> +             return 0;
> >>>> +     strcpy(res->name, sym_name);
> >>>> +     return 1;
> >>>> +}
> >>>> +
> >>>>    static int attach_kprobe(const struct bpf_program *prog, long cookie, struct bpf_link **link)
> >>>>    {
> >>>>        DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
> >>>> +     struct kprobe_resolve res = {};
> >>>>        unsigned long offset = 0;
> >>>>        const char *func_name;
> >>>>        char *func;
> >>>> +     int err;
> >>>>        int n;
> >>>>
> >>>>        *link = NULL;
> >>>> @@ -10408,8 +10426,25 @@ static int attach_kprobe(const struct bpf_program *prog, long cookie, struct bpf
> >>>>
> >>>>        opts.offset = offset;
> >>>>        *link = bpf_program__attach_kprobe_opts(prog, func, &opts);
> >>>> +     err = libbpf_get_error(*link);
> >>>> +
> >>>> +     if (!err || err != -ENOENT)
> >>>> +             goto out;
> >>>> +
> >>>> +     sprintf(res.pattern, "%s.*", func);
> >>>> +     if (!libbpf_kallsyms_parse(kprobe_kallsyms_cb, &res))
> >>>> +             goto out;
> >>>> +
> >>>> +     pr_warn("prog '%s': trying to create %s '%s+0x%zx' perf event instead\n",
> >>>> +             prog->name, opts.retprobe ? "kretprobe" : "kprobe",
> >>>> +             res.name, offset);
> >>>> +
> >>>> +     *link = bpf_program__attach_kprobe_opts(prog, res.name, &opts);
> >>>> +     err = libbpf_get_error(*link);
> >>>> +
> >>>> +out:
> >>>>        free(func);
> >>>> -     return libbpf_get_error(*link);
> >>>> +     return err;
> >>>>    }
> >>>>
> >>>>    static int attach_ksyscall(const struct bpf_program *prog, long cookie, struct bpf_link **link)