Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently

[Stanislav Fomichev <sdf@google.com>]

On Thu, Oct 6, 2022 at 12:21 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Oct 05, 2022 at 04:28:49PM -0700, Andrii Nakryiko wrote:
> > On Tue, Oct 4, 2022 at 7:04 PM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > On Tue, Oct 4, 2022 at 6:39 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 4, 2022 at 5:33 PM <sdf@google.com> wrote:
> > > > >
> > > > > On 10/04, Stanislav Fomichev wrote:
> > > > > > We're having an issue where we have a recent clang that seems
> > > > > > to generate kind_flag for enums (aka, adding signed/unsigned).
> > > > > > Trying to install a module on a kernel that doesn't have commit
> > > > > > 6089fb325cf7 ("bpf: Add btf enum64 support") returns the following:
> > > > >
> > > > > > [    3.176954] BPF:Invalid btf_info kind_flag
> > > > >
> > > > > > The enum that it complains about doesn't seem to have anything special
> > > > > > except having a sign:
> > > > >
> > > > > > [1721] ENUM 'perf_event_state' encoding=SIGNED size=4 vlen=6
> > > > > >          'PERF_EVENT_STATE_DEAD' val=-4
> > > > > >          'PERF_EVENT_STATE_EXIT' val=-3
> > > > > >          'PERF_EVENT_STATE_ERROR' val=-2
> > > > > >          'PERF_EVENT_STATE_OFF' val=-1
> > > > > >          'PERF_EVENT_STATE_INACTIVE' val=0
> > > > > >          'PERF_EVENT_STATE_ACTIVE' val=1
> > > > >
> > > > > > We are not currently using CONFIG_DEBUG_INFO_BTF_MODULES and
> > > > > > don't plan to use module BTF, so it's preferable to be able
> > > > > > to explicits disable it in the kernel config. Unfortunately,
> > > > > > because that kconfig option doesn't have a name, it's not
> > > > > > possible to flip it independently from CONFIG_DEBUG_INFO_BTF.
> > > > > > Let's add a name to make sure module BTF is user-controllable.
> > > > >
> > > > > [..]
> > > > >
> > > > > > (Not sure, but maybe the right fix is to also have a stable patch
> > > > > >   to relax that "Invalid btf_info kind_flag" check?)
> > > > >
> > > > > Answering to myself, looks like we do need the following for
> > > > > non-enum64-compatible older/stable kernels:
> > > > >
> > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > > index 3cfba41a0829..928f4955090a 100644
> > > > > --- a/kernel/bpf/btf.c
> > > > > +++ b/kernel/bpf/btf.c
> > > > > @@ -3301,11 +3301,6 @@ static s32 btf_enum_check_meta(struct
> > > > > btf_verifier_env *env,
> > > > >                 return -EINVAL;
> > > > >         }
> > > > >
> > > > > -       if (btf_type_kflag(t)) {
> > > > > -               btf_verifier_log_type(env, t, "Invalid btf_info kind_flag");
> > > > > -               return -EINVAL;
> > > > > -       }
> > > > > -
> > > > >         if (t->size > 8 || !is_power_of_2(t->size)) {
> > > > >                 btf_verifier_log_type(env, t, "Unexpected size");
> > > > >                 return -EINVAL;
> > > > >
> > > > > Anything I'm missing? Feels like any pre-6089fb325cf7 ("bpf: Add btf
> > > > > enum64 support") kernel will have an issue with a recent clang
> > > > > that puts sign into kflag?
> > > > >
> > > > >
> > > > > > Fixes: 5f9ae91f7c0d ("kbuild: Build kernel module BTFs if BTF is enabled
> > > > > > and pahole supports it")
> > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > > > ---
> > > > > >   lib/Kconfig.debug | 1 +
> > > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > > > > index c77fe36bb3d8..6336a697c9f5 100644
> > > > > > --- a/lib/Kconfig.debug
> > > > > > +++ b/lib/Kconfig.debug
> > > > > > @@ -326,6 +326,7 @@ config PAHOLE_HAS_SPLIT_BTF
> > > > > >       def_bool $(success, test `$(PAHOLE) --version | sed
> > > > > > -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119")
> > > > >
> > > > > >   config DEBUG_INFO_BTF_MODULES
> > > > > > +     bool "Generate BTF module typeinfo"
> > > > > >       def_bool y
> > > > > >       depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF
> > > > > >       help
> > > >
> > > > Not quite following.
> > > > Are you saying instead of backporting enum64 support
> > > > to older kernels you'd prefer to add this patch to upstream
> > > > and backport this smaller patch?
> > >
> > > Yeah, sorry, it took me a while to build the context, I might still be
> > > missing something.
> > >
> > > So far it seems that disabling DEBUG_INFO_BTF_MODULES is not enough.
> > > It looks like as long as the older kernels still have that
> > > btf_type_kflag enum check, compiling them with a fairly recent clang
> > > won't work?
> > > Or do we expect those users to use pahole's --skip_encoding_btf_enum64
> > > which seems to fallback to the old way?
> >
> > Clang doesn't generate kernel or kernel module's BTF, so it has
> > nothing to do with this. It's pahole that needs to be told to not
> > generate kflag bit for enum on older kernels.
> > --skip_encoding_btf_enum64 is not exactly that, seems like we need
> > another flag to disable the sign bit for enum?
> >
> > cc'ed Arnaldo as well.
> >
> > >
> > > I guess I'm still trying to understand whether we care about
> > > old/stable kernels + recent llvm combination.
> >
> > I think it's similar to --skip_encoding_btf_enum64, so I'd say yes?
>
> hi,
> this seems like the issue we fixed recently where BTF with enum64 won't
> pass stable kernel verifier.. we did fix for stable 5.19 and 5.15:
>   https://lore.kernel.org/bpf/20220904131901.13025-1-jolsa@kernel.org/
>   https://lore.kernel.org/bpf/20220916171234.841556-1-yakoyoku@gmail.com/
>
> I did not do 5.10 fix as explained in here:
>   https://lore.kernel.org/bpf/YyeYUEHyR%2FnHM8NT@krava/

Oh, great, thanks for the pointer! That's what I was looking for :-)

> jirka