diff mbox series

[bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently

Message ID 20221004222725.2813510-1-sdf@google.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf
netdev/apply fail Patch does not apply to bpf
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-1 success Logs for build for s390x with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-17 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-8 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-10 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-6 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-9 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-12 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for test_verifier on s390x with gcc

Commit Message

Stanislav Fomichev Oct. 4, 2022, 10:27 p.m. UTC 
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(+)

Comments

Stanislav Fomichev Oct. 5, 2022, 12:33 a.m. UTC | #1 
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
Alexei Starovoitov Oct. 5, 2022, 1:39 a.m. UTC | #2 
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?
Stanislav Fomichev Oct. 5, 2022, 2:04 a.m. UTC | #3 
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.
Andrii Nakryiko Oct. 5, 2022, 11:28 p.m. UTC | #4 
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?
Jiri Olsa Oct. 6, 2022, 7:21 a.m. UTC | #5 
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
Stanislav Fomichev Oct. 6, 2022, 4:20 p.m. UTC | #6 
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
diff mbox series

Patch

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