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@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?
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@xxxxxxxxxx/
https://lore.kernel.org/bpf/20220916171234.841556-1-yakoyoku@xxxxxxxxx/
I did not do 5.10 fix as explained in here:
https://lore.kernel.org/bpf/YyeYUEHyR%2FnHM8NT@krava/
jirka
