Message-ID: <CAFhGd8ob_qet6ODduHz2=sjGXkHaFMzrtu1FFkN0eUWQvpyPrQ@mail.gmail.com>
Date:   Fri, 25 Aug 2023 11:36:25 -0700
From:   Justin Stitt <justinstitt@...gle.com>
To:     Eduard Zingerman <eddyz87@...il.com>
Cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-kselftest@...r.kernel.org, bpf@...r.kernel.org,
        linux-input@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Kees Cook <keescook@...gle.com>
Subject: Re: selftests: hid: trouble building with clang due to missing header

On Fri, Aug 25, 2023 at 6:01 AM Eduard Zingerman <eddyz87@...il.com> wrote:
>
> On Fri, 2023-08-25 at 10:08 +0200, Benjamin Tissoires wrote:
> >
> > On Tue, Aug 22, 2023 at 11:42 PM Justin Stitt <justinstitt@...gle.com> wrote:
> > > > > > > Which kernel are you trying to test?
> > > > > > > I tested your 2 commands on v6.5-rc7 and it just works.
> > > > > >
> > > > > > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> > > > > >
> > > > > > I ran these exact commands:
> > > > > > >    $ make mrproper
> > > > > > >    $ make LLVM=1 ARCH=x86_64 headers
> > > > > > >    $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > > > > > TARGETS=hid &> out
> > > > > >
> > > > > > and here's the contents of `out` (still warnings/errors):
> > > > > > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> > > > > >
> > > > > > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
> > > > >
> > > > > Sigh... there is a high chance my Makefile is not correct and uses the
> > > > > installed headers (I was running the exact same commands, but on a
> > > > > v6.4-rc7+ kernel).
> > > > >
> > > > > But sorry, it will have to wait for tomorrow if you want me to have a
> > > > > look at it. It's 11:35 PM here, and I need to go to bed
> > > > Take it easy. Thanks for the prompt responses here! I'd like to get
> > > > the entire kselftest make target building with Clang so that we can
> > > > close [1].
> >
> > Sorry I got urgent matters to tackle yesterday.
> >
> > It took me a while to understand what was going on, and I finally found
> > it.
> >
> > struct hid_bpf_ctx is internal to the kernel, and so is declared in
> > vmlinux.h, that we generate through BTF. But to generate the vmlinux.h
> > with the correct symbols, these need to be present in the running
> > kernel.
> > And that's where we had a fundamental difference: I was running my
> > compilations on a kernel v6.3+ (6.4.11) with that symbol available, and
> > you are probably not.
> >
> > The bpf folks are using a clever trick to force the compilation[2]. And
> > I think the following patch would work for you:
>
> Hi Benjamin, Justin,
>
> You might want to take a look at these two links:
> [1] https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes
> [2] https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html#dealing-with-kernel-version-and-configuration-differences
>
> The short version is: CO-RE relocation handling logic in libbpf
> ignores suffixes of form '___something' for type and field names.
>
> So, the following should accomplish the same as the trick with
> #define/#undef:
>
>     #include "vmlinux.h"
>     ...
>     struct hid_bpf_ctx___local {
>         __u32 index;
>         const struct hid_device *hid;
>         __u32 allocated_size;
>         enum hid_report_type report_type;
>         union {
>             __s32 retval;
>             __s32 size;
>         };
>
>     };
>     ...
>     extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
>                                   unsigned int offset, ...)
>
> However, if the kernel does not have `hid_bpf_ctx` definition would
> the test `progs/hid.c` still make sense?
>
> When I tried to build hid tests locally I run into similar errors:
>
>     ...
>       CLNG-BPF hid.bpf.o
>     In file included from progs/hid.c:6:
>     progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct hid_bpf_ctx' \
>            will not be visible outside of this function [-Werror,-Wvisibility]
>     extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
>     ...
>
> And there is indeed no `hid_bpf_ctx` in my vmlinux.h.
> However, after enabling CONFIG_HID_BPF in kernel config the
> `hid_bpf_ctx` appears in vmlinux.h, and I can compile HID selftests
> w/o issues.

Even with enabling this configuration option I was unable to get clean
builds of the HID selftests. I proposed a 4th patch on top of
Benjamin's n=3 patch series here [1] using the #def/#undef pattern.

>
> >
> > ---
> >  From bb9eccb7a896ba4b3a35ed12a248e6d6cfed2df6 Mon Sep 17 00:00:00 2001
> > From: Benjamin Tissoires <bentiss@...nel.org>
> > Date: Fri, 25 Aug 2023 10:02:32 +0200
> > Subject: [PATCH] selftests/hid: ensure we can compile the tests on kernels
> >   pre-6.3
> >
> > For the hid-bpf tests to compile, we need to have the definition of
> > struct hid_bpf_ctx. This definition is an internal one from the kernel
> > and it is supposed to be defined in the generated vmlinux.h.
> >
> > This vmlinux.h header is generated based on the currently running kernel
> > or if the kernel was already compiled in the tree. If you just compile
> > the selftests without compiling the kernel beforehand and you are running
> > on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> > definition.
> >
> > Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> > to force the definition of that symbol in case we don't find it in the
> > BTF.
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@...nel.org>
> > ---
> >   tools/testing/selftests/hid/progs/hid.c       |  3 ---
> >   .../selftests/hid/progs/hid_bpf_helpers.h     | 20 +++++++++++++++++++
> >   2 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> > index 88c593f753b5..1e558826b809 100644
> > --- a/tools/testing/selftests/hid/progs/hid.c
> > +++ b/tools/testing/selftests/hid/progs/hid.c
> > @@ -1,8 +1,5 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   /* Copyright (c) 2022 Red hat */
> > -#include "vmlinux.h"
> > -#include <bpf/bpf_helpers.h>
> > -#include <bpf/bpf_tracing.h>
> >   #include "hid_bpf_helpers.h"
> >
> >   char _license[] SEC("license") = "GPL";
> > diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > index 4fff31dbe0e7..749097f8f4d9 100644
> > --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > @@ -5,6 +5,26 @@
> >   #ifndef __HID_BPF_HELPERS_H
> >   #define __HID_BPF_HELPERS_H
> >
> > +/* "undefine" structs in vmlinux.h, because we "override" them below */
> > +#define hid_bpf_ctx hid_bpf_ctx___not_used
> > +#include "vmlinux.h"
> > +#undef hid_bpf_ctx
> > +
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +
> > +struct hid_bpf_ctx {
> > +     __u32 index;
> > +     const struct hid_device *hid;
> > +     __u32 allocated_size;
> > +     enum hid_report_type report_type;
> > +     union {
> > +             __s32 retval;
> > +             __s32 size;
> > +     };
> > +};
> > +
> >   /* following are kfuncs exported by HID for HID-BPF */
> >   extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> >                             unsigned int offset,
>

[1]: https://lore.kernel.org/all/20230825182316.m2ksjoxe4s7dsapn@google.com/

Thanks
Justin

Powered by blists - more mailing lists