Re: [PATCH v2 bpf 02/11] bpftool: define a local bpf_perf_link to fix accessing its fields

[Yonghong Song <yhs@meta.com>]

On 5/4/23 1:18 AM, Michal Suchánek wrote:
> Hello,
> 
> On Thu, May 04, 2023 at 12:43:52AM +0100, Quentin Monnet wrote:
>> On Fri, 21 Apr 2023 at 08:39, Michal Suchánek <msuchanek@suse.de> wrote:
>>>
>>> On Thu, Apr 20, 2023 at 04:07:38PM -0700, Andrii Nakryiko wrote:
>>>> On Fri, Apr 14, 2023 at 9:28 AM Michal Suchánek <msuchanek@suse.de> wrote:
>>>>>
>>>>> On Fri, Apr 14, 2023 at 05:18:27PM +0200, Alexander Lobakin wrote:
>>>>>> From: Michal Suchánek <msuchanek@suse.de>
>>>>>> Date: Fri, 14 Apr 2023 11:54:57 +0200
>>>>>>
>>>>>>> Hello,
>>>>>>
>>>>>> Hey-hey,
>>>>>>
>>>>>>>
>>>>>>> On Thu, Apr 21, 2022 at 12:38:58AM +0000, Alexander Lobakin wrote:
>>>>>>>> When building bpftool with !CONFIG_PERF_EVENTS:
>>>>>>>>
>>>>>>>> skeleton/pid_iter.bpf.c:47:14: error: incomplete definition of type 'struct bpf_perf_link'
>>>>>>>>          perf_link = container_of(link, struct bpf_perf_link, link);
>>>>>>>>                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:74:22: note: expanded from macro 'container_of'
>>>>>>>>                  ((type *)(__mptr - offsetof(type, member)));    \
>>>>>>>>                                     ^~~~~~~~~~~~~~~~~~~~~~
>>>>>>>> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:68:60: note: expanded from macro 'offsetof'
>>>>>>>>   #define offsetof(TYPE, MEMBER)  ((unsigned long)&((TYPE *)0)->MEMBER)
>>>>>>>>                                                    ~~~~~~~~~~~^
>>>>>>>> skeleton/pid_iter.bpf.c:44:9: note: forward declaration of 'struct bpf_perf_link'
>>>>>>>>          struct bpf_perf_link *perf_link;
>>>>>>>>                 ^
>>>>>>>>
>>>>>>>> &bpf_perf_link is being defined and used only under the ifdef.
>>>>>>>> Define struct bpf_perf_link___local with the `preserve_access_index`
>>>>>>>> attribute inside the pid_iter BPF prog to allow compiling on any
>>>>>>>> configs. CO-RE will substitute it with the real struct bpf_perf_link
>>>>>>>> accesses later on.
>>>>>>>> container_of() is not CO-REd, but it is a noop for
>>>>>>>> bpf_perf_link <-> bpf_link and the local copy is a full mirror of
>>>>>>>> the original structure.
>>>>>>>>
>>>>>>>> Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output")
>>>>>>>
>>>>>>> This does not solve the problem completely. Kernels that don't have
>>>>>>> CONFIG_PERF_EVENTS in the first place are also missing the enum value
>>>>>>> BPF_LINK_TYPE_PERF_EVENT which is used as the condition for handling the
>>>>>>> cookie.
>>>>>>
>>>>>> Sorry, I haven't been working with my home/private stuff for more than a
>>>>>> year already. I may get back to it some day when I'm tired of Lua (curse
>>>>>> words, sorry :D), but for now the series is "a bit" abandoned.
>>>>>
>>>>> This part still appllies and works for me with the caveat that
>>>>> BPF_LINK_TYPE_PERF_EVENT also needs to be defined.
>>>>>
>>>>>> I think there was alternative solution proposed there, which promised to
>>>>>> be more flexible. But IIRC it also doesn't touch the enum (was it added
>>>>>> recently? Because it was building just fine a year ago on config without
>>>>>> perf events).
>>>>>
>>>>> It was added in 5.15. Not sure there is a kernel.org LTS kernel usable
>>>>> for CO-RE that does not have it, technically 5.4 would work if it was
>>>>> built monolithic, it does not have module BTF, only kernel IIRC.
>>>>>
>>>>> Nonetheless, the approach to handling features completely missing in the
>>>>> running kernel should be figured out one way or another. I would be
>>>>> surprised if this was the last feature to be added that bpftool needs to
>>>>> know about.
>>>>
>>>> Are we talking about bpftool built from kernel sources or from Github?
>>>> Kernel source version should have access to latest UAPI headers and so
>>>> BPF_LINK_TYPE_PERF_EVENT should be available. Github version, if it
>>>> doesn't do that already, can use UAPI headers distributed (and used
>>>> for building) with libbpf through submodule.
>>>
>>> It does have a copy of the uapi headers but apparently does not use
>>> them. Using them directly might cause conflict with vmlinux.h, though.
>>
>> Indeed, using the UAPI header here conflicts with vmlinux.h.
>>
>> Looking again at some code I started last year but never finalised, I
>> used the following approach, redefining BPF_LINK_TYPE_PERF_EVENT with
>> CO-RE:
>>
>>      enum bpf_link_type___local {
>>          BPF_LINK_TYPE_PERF_EVENT___local = 7,
>>      };
> 
> That's the same as I did except I used simple define instead of this
> fake enum.
> 
> The enum only has value when it is complete and the compiler can check
> that a switch uses only known values, and can confuse things when values
> are missing.

Currently, enum value CORE is done though a llvm builtin function. So
if the enum value is used in switch cases like
   switch(...)
   case BPF_LINK_TYPE_PERF_EVENT:
      ...
CORE relocation will not work in that case since the compiler
expects BPF_LINK_TYPE_PERF_EVENT to be a constant.

> Thanks
> 
> Michal