Messages in this thread | | | Date | Fri, 13 Jan 2023 00:00:55 -0800 | Subject | Re: [PATCH] libbpf: resolve kernel function name optimization for kprobe | From | Yonghong Song <> |
| |
On 1/12/23 1:07 PM, Alexei Starovoitov wrote: > On Thu, Jan 12, 2023 at 2:20 AM Alan Maguire <alan.maguire@oracle.com> 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@meta.com> wrote: >>>>> >>>>> >>>>> >>>>> On 1/9/23 1:42 AM, menglong8.dong@gmail.com wrote: >>>>>> From: Menglong Dong <imagedong@tencent.com> >>>>>> >>>>>> 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? > > Sounds like a great idea to me. > Addresses this issue in a clean way.
Yes, the second approach seems a reasonable approach. If the number of parameters for the *actual* functions equals to the number of parameters for the defined function (abstract_origin), we can roughly assume the actual function signature matches the prototype. Although it is theoretically possible that compiler might change parameter types, e.g., from a struct pointer (struct foo *p) to a int value (p->field1). But this should be extremely rare and we need compiler emitting additional dwarf data (might through btf_decl_tag) to discover such cases.
| |