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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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?