| Message ID | 20250122025308.2717553-5-ihor.solodrai@pm.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | BTF: arbitrary __attribute__ encoding | expand |
On 22/01/2025 02:53, Ihor Solodrai wrote: > BTF type tags and decl tags now may have info->kflag set to 1, > changing the semantics of the tag. > > Change BTF verification to permit BTF that makes use of this feature: > * remove kflag check in btf_decl_tag_check_meta(), as both values > are valid > * allow kflag to be set for BTF_KIND_TYPE_TAG type in > btf_ref_type_check_meta() > > Modify a selftest checking for kflag in decl_tag accordingly. > > Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me> Looks good, but I have a few questions here. Today in btf_struct_walk() we check pointer type tags like this: /* check type tag */ t = btf_type_by_id(btf, mtype->type); if (btf_type_is_type_tag(t)) { tag_value = __btf_name_by_offset(btf, t->name_off); /* check __user tag */ if (strcmp(tag_value, "user") == 0) tmp_flag = MEM_USER; /* check __percpu tag */ if (strcmp(tag_value, "percpu") == 0) tmp_flag = MEM_PERCPU; /* check __rcu tag */ if (strcmp(tag_value, "rcu") == 0) tmp_flag = MEM_RCU; } Do we need to add in a check for kind_flag == 0 here to ensure it's a BTF type tag attribute? Similar question for type tag kptr checking in btf_find_kptr() (we could perhaps add a btf_type_is_btf_type_tag() or something to include the kind_flag == 0 check). And I presume the btf_check_type_tags() logic still applies for generic attributes - i.e. that they always precede the modifiers in the chain? > --- > kernel/bpf/btf.c | 7 +------ > tools/testing/selftests/bpf/prog_tests/btf.c | 4 +--- > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 8396ce1d0fba..becdec583e00 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -2575,7 +2575,7 @@ static int btf_ref_type_check_meta(struct btf_verifier_env *env, > return -EINVAL; > } > > - if (btf_type_kflag(t)) { > + if (btf_type_kflag(t) && BTF_INFO_KIND(t->info) != BTF_KIND_TYPE_TAG) { > btf_verifier_log_type(env, t, "Invalid btf_info kind_flag"); > return -EINVAL; > } > @@ -4944,11 +4944,6 @@ static s32 btf_decl_tag_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; > - } > - > component_idx = btf_type_decl_tag(t)->component_idx; > if (component_idx < -1) { > btf_verifier_log_type(env, t, "Invalid component_idx"); > diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c > index e63d74ce046f..aab9ad88c845 100644 > --- a/tools/testing/selftests/bpf/prog_tests/btf.c > +++ b/tools/testing/selftests/bpf/prog_tests/btf.c > @@ -3866,7 +3866,7 @@ static struct btf_raw_test raw_tests[] = { > .err_str = "vlen != 0", > }, > { > - .descr = "decl_tag test #8, invalid kflag", > + .descr = "decl_tag test #8, tag with kflag", > .raw_types = { > BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ > BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */ > @@ -3881,8 +3881,6 @@ static struct btf_raw_test raw_tests[] = { > .key_type_id = 1, > .value_type_id = 1, > .max_entries = 1, > - .btf_load_err = true, > - .err_str = "Invalid btf_info kind_flag", > }, > { > .descr = "decl_tag test #9, var, invalid component_idx",
On Wednesday, January 22nd, 2025 at 3:09 AM, Alan Maguire <alan.maguire@oracle.com> wrote: > > > On 22/01/2025 02:53, Ihor Solodrai wrote: > > > BTF type tags and decl tags now may have info->kflag set to 1, > > changing the semantics of the tag. > > > > Change BTF verification to permit BTF that makes use of this feature: > > * remove kflag check in btf_decl_tag_check_meta(), as both values > > are valid > > * allow kflag to be set for BTF_KIND_TYPE_TAG type in > > btf_ref_type_check_meta() > > > > Modify a selftest checking for kflag in decl_tag accordingly. > > > > Signed-off-by: Ihor Solodrai ihor.solodrai@pm.me > > > Looks good, but I have a few questions here. Today in btf_struct_walk() > we check pointer type tags like this: > > /* check type tag */ > t = btf_type_by_id(btf, mtype->type); > > if (btf_type_is_type_tag(t)) { > tag_value = __btf_name_by_offset(btf, > t->name_off); > > /* check __user tag / > if (strcmp(tag_value, "user") == 0) > tmp_flag = MEM_USER; > / check __percpu tag / > if (strcmp(tag_value, "percpu") == 0) > tmp_flag = MEM_PERCPU; > / check __rcu tag */ > if (strcmp(tag_value, "rcu") == 0) > tmp_flag = MEM_RCU; > } > > > Do we need to add in a check for kind_flag == 0 here to ensure it's a > BTF type tag attribute? Similar question for type tag kptr checking in > btf_find_kptr() (we could perhaps add a btf_type_is_btf_type_tag() or > something to include the kind_flag == 0 check). Yes, we should check for kflag here. Thanks for pointing this out. __attribute__((user)) vs __attribute__((bpf_type_tag("user"))) are very different things. > > And I presume the btf_check_type_tags() logic still applies for generic > attributes - i.e. that they always precede the modifiers in the chain? Yes, that is my understanding too. > > [...]
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 8396ce1d0fba..becdec583e00 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -2575,7 +2575,7 @@ static int btf_ref_type_check_meta(struct btf_verifier_env *env, return -EINVAL; } - if (btf_type_kflag(t)) { + if (btf_type_kflag(t) && BTF_INFO_KIND(t->info) != BTF_KIND_TYPE_TAG) { btf_verifier_log_type(env, t, "Invalid btf_info kind_flag"); return -EINVAL; } @@ -4944,11 +4944,6 @@ static s32 btf_decl_tag_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; - } - component_idx = btf_type_decl_tag(t)->component_idx; if (component_idx < -1) { btf_verifier_log_type(env, t, "Invalid component_idx"); diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c index e63d74ce046f..aab9ad88c845 100644 --- a/tools/testing/selftests/bpf/prog_tests/btf.c +++ b/tools/testing/selftests/bpf/prog_tests/btf.c @@ -3866,7 +3866,7 @@ static struct btf_raw_test raw_tests[] = { .err_str = "vlen != 0", }, { - .descr = "decl_tag test #8, invalid kflag", + .descr = "decl_tag test #8, tag with kflag", .raw_types = { BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */ @@ -3881,8 +3881,6 @@ static struct btf_raw_test raw_tests[] = { .key_type_id = 1, .value_type_id = 1, .max_entries = 1, - .btf_load_err = true, - .err_str = "Invalid btf_info kind_flag", }, { .descr = "decl_tag test #9, var, invalid component_idx",
BTF type tags and decl tags now may have info->kflag set to 1, changing the semantics of the tag. Change BTF verification to permit BTF that makes use of this feature: * remove kflag check in btf_decl_tag_check_meta(), as both values are valid * allow kflag to be set for BTF_KIND_TYPE_TAG type in btf_ref_type_check_meta() Modify a selftest checking for kflag in decl_tag accordingly. Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me> --- kernel/bpf/btf.c | 7 +------ tools/testing/selftests/bpf/prog_tests/btf.c | 4 +--- 2 files changed, 2 insertions(+), 9 deletions(-)