On Tue, Oct 4, 2022 at 7:04 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> 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?
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?
