[PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently

[PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently

[Stanislav Fomichev]

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?)

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
-- 
2.38.0.rc1.362.ged0d419d3c-goog

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

[sdf]

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
> --
> 2.38.0.rc1.362.ged0d419d3c-goog

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

[Alexei Starovoitov]

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?

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

[Stanislav Fomichev]

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?

I guess I'm still trying to understand whether we care about
old/stable kernels + recent llvm combination.

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

[Andrii Nakryiko]

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?

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

[Jiri Olsa]

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/

jirka

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

[Stanislav Fomichev]

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