Re: [PATCH v8 bpf-next 06/10] btf: support kernel parsing of BTF with kind layout

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2025-12-16 at 14:09 -0800, Andrii Nakryiko wrote:
> On Tue, Dec 16, 2025 at 1:25 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Tue, 2025-12-16 at 13:21 -0800, Andrii Nakryiko wrote:
> > > On Mon, Dec 15, 2025 at 10:51 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > > 
> > > > On Mon, 2025-12-15 at 09:17 +0000, Alan Maguire wrote:
> > > > 
> > > > If strict kind layout checks are the goal, would it make sense to
> > > > check sizes declared in kind_layout for known types?
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -5215,23 +5216,36 @@ static s32 btf_check_meta(struct btf_verifier_env *env,
> > > > >               return -EINVAL;
> > > > >       }
> > > > > 
> > > > > -     if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX ||
> > > > > -         BTF_INFO_KIND(t->info) == BTF_KIND_UNKN) {
> > > > > +     if (!btf_name_offset_valid(env->btf, t->name_off)) {
> > > > > +             btf_verifier_log(env, "[%u] Invalid name_offset:%u",
> > > > > +                              env->log_type_id, t->name_off);
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     if (BTF_INFO_KIND(t->info) == BTF_KIND_UNKN) {
> > > > >               btf_verifier_log(env, "[%u] Invalid kind:%u",
> > > > >                                env->log_type_id, BTF_INFO_KIND(t->info));
> > > > >               return -EINVAL;
> > > > >       }
> > > > > 
> > > > > -     if (!btf_name_offset_valid(env->btf, t->name_off)) {
> > > > > -             btf_verifier_log(env, "[%u] Invalid name_offset:%u",
> > > > > -                              env->log_type_id, t->name_off);
> > > > > +     if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX && env->btf->kind_layout &&
> > > > > +         ((BTF_INFO_KIND(t->info) + 1) * sizeof(struct btf_kind_layout)) <
> > > > > +          env->btf->hdr.kind_layout_len) {
> > > > > +             btf_verifier_log(env, "[%u] unknown but required kind %u",
> > > > > +                              env->log_type_id,
> > > > > +                              BTF_INFO_KIND(t->info));
> > > > >               return -EINVAL;
> > > > > +     } else {
> > > > > +             if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX) {
> > > > > +                     btf_verifier_log(env, "[%u] Invalid kind:%u",
> > > > > +                                      env->log_type_id, BTF_INFO_KIND(t->info));
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > > +             var_meta_size = btf_type_ops(t)->check_meta(env, t, meta_left);
> > > > > +             if (var_meta_size < 0)
> > > > > +                     return var_meta_size;
> > > > >       }
> > > > > 
> > > > > -     var_meta_size = btf_type_ops(t)->check_meta(env, t, meta_left);
> > > > > -     if (var_meta_size < 0)
> > > > > -             return var_meta_size;
> > > > > -
> > > > >       meta_left -= var_meta_size;
> > > > 
> > > > It appears that a smaller change here would achieve same results:
> > > > 
> > > >     -        if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX ||
> > > >     +        u32 layout_kind_max = env->btf->hdr.kind_layout_len / sizeof(struct btf_kind_layout);
> > > >     +        if (BTF_INFO_KIND(t->info) > layout_kind_max ||
> > > >                  BTF_INFO_KIND(t->info) == BTF_KIND_UNKN) {
> > > >                      btf_verifier_log(env, "[%u] Invalid kind:%u",
> > > >                                       env->log_type_id, BTF_INFO_KIND(t->info));
> > > >                      return -EINVAL;
> > > >              }
> > > > 
> > > >     +        if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX) {
> > > >     +                btf_verifier_log(env, "[%u] unknown but required kind %u",
> > > >     +                                 env->log_type_id,
> > > >     +                                 BTF_INFO_KIND(t->info));
> 
> hm... did you mean to have return -EINVAL here? Then it wouldn't let
> through anything that's not known to verifier.

Oops, yeap, I meant to return error here.
As Alan's original patch does.
Sorry for confusion.

[...]