* [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently
@ 2022-10-04 22:27 Stanislav Fomichev
2022-10-05 0:33 ` sdf
0 siblings, 1 reply; 7+ messages in thread
From: Stanislav Fomichev @ 2022-10-04 22:27 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently 2022-10-04 22:27 [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently Stanislav Fomichev @ 2022-10-05 0:33 ` sdf 2022-10-05 1:39 ` Alexei Starovoitov 0 siblings, 1 reply; 7+ messages in thread From: sdf @ 2022-10-05 0:33 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend, kpsingh, haoluo, jolsa 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently 2022-10-05 0:33 ` sdf @ 2022-10-05 1:39 ` Alexei Starovoitov 2022-10-05 2:04 ` Stanislav Fomichev 0 siblings, 1 reply; 7+ messages in thread From: Alexei Starovoitov @ 2022-10-05 1:39 UTC (permalink / raw) To: Stanislav Fomichev Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Hao Luo, Jiri Olsa 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently 2022-10-05 1:39 ` Alexei Starovoitov @ 2022-10-05 2:04 ` Stanislav Fomichev 2022-10-05 23:28 ` Andrii Nakryiko 0 siblings, 1 reply; 7+ messages in thread From: Stanislav Fomichev @ 2022-10-05 2:04 UTC (permalink / raw) To: Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Hao Luo, Jiri Olsa 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently 2022-10-05 2:04 ` Stanislav Fomichev @ 2022-10-05 23:28 ` Andrii Nakryiko 2022-10-06 7:21 ` Jiri Olsa 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2022-10-05 23:28 UTC (permalink / raw) To: Stanislav Fomichev, Arnaldo Carvalho de Melo Cc: Alexei Starovoitov, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Hao Luo, Jiri Olsa 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently 2022-10-05 23:28 ` Andrii Nakryiko @ 2022-10-06 7:21 ` Jiri Olsa 2022-10-06 16:20 ` Stanislav Fomichev 0 siblings, 1 reply; 7+ messages in thread From: Jiri Olsa @ 2022-10-06 7:21 UTC (permalink / raw) To: Andrii Nakryiko Cc: Stanislav Fomichev, Arnaldo Carvalho de Melo, Alexei Starovoitov, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Hao Luo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently 2022-10-06 7:21 ` Jiri Olsa @ 2022-10-06 16:20 ` Stanislav Fomichev 0 siblings, 0 replies; 7+ messages in thread From: Stanislav Fomichev @ 2022-10-06 16:20 UTC (permalink / raw) To: Jiri Olsa Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, Alexei Starovoitov, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Hao Luo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-06 16:20 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-04 22:27 [PATCH bpf] bpf: make DEBUG_INFO_BTF_MODULES selectable independently Stanislav Fomichev 2022-10-05 0:33 ` sdf 2022-10-05 1:39 ` Alexei Starovoitov 2022-10-05 2:04 ` Stanislav Fomichev 2022-10-05 23:28 ` Andrii Nakryiko 2022-10-06 7:21 ` Jiri Olsa 2022-10-06 16:20 ` Stanislav Fomichev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).