On Tue, Oct 4, 2022 at 6:39 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Oct 4, 2022 at 5:33 PM <sdf@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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?
I guess I'm still trying to understand whether we care about
old/stable kernels + recent llvm combination.
