Re: [PATCH bpf-next 2/4] bpftool: define a local bpf_perf_link to fix accessing its fields

[Yonghong Song <yhs@meta.com>]

On 5/12/23 3:33 AM, Quentin Monnet wrote:
> From: Alexander Lobakin <alobakin@pm.me>
> 
> 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

'container_of() is not CO-REd' is incorrect.

#define container_of(ptr, type, member)                         \
         ({                                                      \
                 void *__mptr = (void *)(ptr);                   \
                 ((type *)(__mptr - offsetof(type, member)));    \
         })


offsetof() will do necessary CO-RE relocation if the field is specified
with preserve_access_index attribute. So container_of will actually
do CO-RE relocation as well.


> 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")
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>   tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> index e2af8e5fb29e..3a4c4f7d83d8 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> @@ -15,6 +15,11 @@ enum bpf_obj_type {
>   	BPF_OBJ_BTF,
>   };
>   
> +struct bpf_perf_link___local {
> +	struct bpf_link link;
> +	struct file *perf_file;
> +} __attribute__((preserve_access_index));
> +
>   struct perf_event___local {
>   	u64 bpf_cookie;
>   } __attribute__((preserve_access_index));
> @@ -45,10 +50,10 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
>   /* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
>   static __u64 get_bpf_cookie(struct bpf_link *link)
>   {
> +	struct bpf_perf_link___local *perf_link;
>   	struct perf_event___local *event;
> -	struct bpf_perf_link *perf_link;
>   
> -	perf_link = container_of(link, struct bpf_perf_link, link);
> +	perf_link = container_of(link, struct bpf_perf_link___local, link);
>   	event = BPF_CORE_READ(perf_link, perf_file, private_data);
>   	return BPF_CORE_READ(event, bpf_cookie);
>   }