Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa <olsajiri@gmail.com>]

On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote:
> 
> 
> On 5/12/23 7:59 PM, Yonghong Song wrote:
> > 
> > 
> > On 5/12/23 2:54 PM, Jiri Olsa wrote:
> > > On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
> > > > On Fri, May 12, 2023 at 9:04 AM Alan Maguire
> > > > <alan.maguire@oracle.com> wrote:
> > > > > 
> > > > > On 12/05/2023 03:51, Yafang Shao wrote:
> > > > > > On Wed, May 10, 2023 at 9:03 PM Alan Maguire
> > > > > > <alan.maguire@oracle.com> wrote:
> > > > > > > 
> > > > > > > v1.25 of pahole supports filtering out functions
> > > > > > > with multiple inconsistent
> > > > > > > function prototypes or optimized-out parameters from
> > > > > > > the BTF representation.
> > > > > > > These present problems because there is no
> > > > > > > additional info in BTF saying which
> > > > > > > inconsistent prototype matches which function
> > > > > > > instance to help guide attachment,
> > > > > > > and functions with optimized-out parameters can lead
> > > > > > > to incorrect assumptions
> > > > > > > about register contents.
> > > > > > > 
> > > > > > > So for now, filter out such functions while adding
> > > > > > > BTF representations for
> > > > > > > functions that have "."-suffixes (foo.isra.0) but
> > > > > > > not optimized-out parameters.
> > > > > > > This patch assumes that below linked changes land in
> > > > > > > pahole for v1.25.
> > > > > > > 
> > > > > > > Issues with pahole filtering being too aggressive in
> > > > > > > removing functions
> > > > > > > appear to be resolved now, but CI and further testing will confirm.
> > > > > > > 
> > > > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > > > > > ---
> > > > > > >   scripts/pahole-flags.sh | 3 +++
> > > > > > >   1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > > > > > > index 1f1f1d397c39..728d55190d97 100755
> > > > > > > --- a/scripts/pahole-flags.sh
> > > > > > > +++ b/scripts/pahole-flags.sh
> > > > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > > > > > >          # see PAHOLE_HAS_LANG_EXCLUDE
> > > > > > >          extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > > > > > >   fi
> > > > > > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > > > > > +       extra_paholeopt="${extra_paholeopt}
> > > > > > > --skip_encoding_btf_inconsistent_proto
> > > > > > > --btf_gen_optimized"
> > > > > > > +fi
> > > > > > > 
> > > > > > >   echo ${extra_paholeopt}
> > > > > > > -- 
> > > > > > > 2.31.1
> > > > > > > 
> > > > > > 
> > > > > > That change looks like a workaround to me.
> > > > > > There may be multiple functions that have the same proto, e.g.:
> > > > > > 
> > > > > >    $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
> > > > > > kernel/bpf/ net/core/
> > > > > >    kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
> > > > > > bpf_iter_aux_info *aux)
> > > > > >    net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
> > > > > > bpf_iter_aux_info *aux)
> > > > > > 
> > > > > >    $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
> > > > > > bpf_iter_detach_map
> > > > > >    [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
> > > > > >    'aux' type_id=2638
> > > > > >    [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
> > > > > > 
> > > > > > We don't know which one it is in the BTF.
> > > > > > However, I'm not against this change, as it can avoid some issues.
> > > > > > 
> > > > > 
> > > > > In the above case, the BTF representation is consistent though.
> > > > > That is, if I attach fentry progs to either of these functions
> > > > > based on that BTF representation, nothing will crash.
> > > > > 
> > > > > That's ultimately what those changes are about; ensuring
> > > > > consistency in BTF representation, so when a function is in
> > > > > BTF we can know the signature of the function can be safely
> > > > > used by fentry for example.
> > > > > 
> > > > > The question of being able to identify functions (as opposed
> > > > > to having a consistent representation) is the next step.
> > > > > Finding a way to link between kallsyms and BTF would allow us to
> > > > > have multiple inconsistent functions in BTF, since we could map
> > > > > from BTF -> kallsyms safely. So two functions called "foo"
> > > > > with different function signatures would be okay, because
> > > > > we'd know which was which in kallsyms and could attach
> > > > > safely. Something like a BTF tag for the function that could
> > > > > clarify that mapping - but just for cases where it would
> > > > > otherwise be ambiguous - is probably the way forward
> > > > > longer term.
> > > > > 
> > > > > Jiri's talking about this topic at LSF/MM/BPF this week I believe.
> > > > 
> > > > Jiri presented a few ideas during LSFMMBPF.
> > > > 
> > > > I feel the best approach is to add a set of addr-s to BTF
> > > > via a special decl_tag.
> > > > We can also consider extending KIND_FUNC.
> > > > The advantage that every BTF func will have one or more addrs
> > > > associated with it and bpf prog loading logic wouldn't need to do
> > > > fragile name comparison between btf and kallsyms.
> > > > pahole can take addrs from dwarf and optionally double check
> > > > with kallsyms.
> > > 
> > > Yonghong summed it up in another email discussion, pasting it in here:
> > > 
> > >    So overall we have three options as kallsyms representation now:
> > >      (a) "addr module:foo:dir_a/dir_b/core.c"
> > >      (b) "addr module:foo"
> > >      (c) "addr module:foo:btf_id"
> > > 
> > >    option (a):
> > >      'dir_a/dir_b/core.c' needs to be encoded in BTF.
> > >      user space either check file path or func signature
> > >      to find attach_btf_id and pass to the kernel.
> > >      kernel can find file path in BTF and then lookup
> > >      kallsyms to find addr.
> > > 
> > >    option (b):
> > >      "addr" needs to be encoded in BTF.
> > >      user space checks func signature to find
> > >      attach_btf_id and pass to the kernel.
> > >      kernel can find addr in BTF and use it.
> > > 
> > >    option (c):
> > >      if user can decide which function to attach, e.g.,
> > >      through func signature, then no BTF encoding
> > >      is necessary. attach_btf_id is passed to the
> > >      kernel and search kallsyms to find the matching
> > >      btf_id and 'addr' will be available then.
> > > 
> > >    For option (b) and (c), user space needs to check
> > >    func signature to find which btf_id to use. If
> > >    same-name static functions having the identical
> > >    signatures, then user space would have a hard time
> > >    to differentiate. I think it should be very
> > >    rare same-name static functions in the kernel will have
> > >    identical signatures. But if we want 100% correctness,
> > >    we may need file path in which case option (a)
> > >    is preferable.
> > 
> > As Alexei mentioned in previous email, for such a extreme case,
> > if user is willing to go through extra step to check dwarf
> > to find and match file path, then (b) and (c) should work
> > perfectly as well.
> 
> Okay, it looks like this is more complex if the function signature is
> the same. In such cases, current BTF dedup will merge these
> functions as a single BTF func. In such cases, we could have:
> 
>    decl_tag_1   ----> dedup'ed static_func
>                          ^
>                          |
>    decl_tag_2   ---------
> 
> For such cases, just passing btf_id of static func to kernel
> won't work since the kernel won't be able to know which
> decl_tag to be associated with.
> 
> (I did a simple test with vmlinux, it looks we have
>  issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
>  as well since only one of decl_tag survives.
>  But this is a different issue.
> )
> 
> So if we intend to add decl tag (addr or file_path), we
> should not dedup static functions or generally any functions.

I did not think functions would be dedup-ed, they are ;-) with the
declaration tags in place we could perhaps switch it off, right?

or perhaps I can't think of all the cases we need functions dedup for,
so maybe the dedup code could check also the associated decl tag when
comparing functions

jirka