linux-kernel - Re: [RFC PATCH v7 1/7] libbpf: Add BTF permutation support for type reordering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAErzpmv1W69HTB=WjETaB0qz4WJsUE4zSMd7dJ2v7PLmr3mzhg@mail.gmail.com>
Date: Fri, 21 Nov 2025 22:15:54 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>, ast@...nel.org, zhangxiaoqin@...omi.com, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org, 
	Donglin Peng <pengdonglin@...omi.com>, Alan Maguire <alan.maguire@...cle.com>, 
	Song Liu <song@...nel.org>
Subject: Re: [RFC PATCH v7 1/7] libbpf: Add BTF permutation support for type reordering

On Fri, Nov 21, 2025 at 7:21 AM Eduard Zingerman <eddyz87@...il.com> wrote:
>
> On Wed, 2025-11-19 at 10:21 -0800, Andrii Nakryiko wrote:
> > On Tue, Nov 18, 2025 at 7:21 PM Donglin Peng <dolinux.peng@...il.com> wrote:
>
> [...]
>
> > > +       nt = new_types;
> > > +       for (i = 0; i < new_nr_types; i++) {
> > > +               struct btf_field_iter it;
> > > +               const struct btf_type *t;
> > > +               __u32 *type_id;
> > > +               int type_size;
> > > +
> > > +               id = order_map[i];
> > > +               /* must be a valid type ID */
> >
> > redundant comment, please drop
> >
> > > +               t = btf__type_by_id(btf, id);
> > > +               if (!t) {
> >
> > no need to check this, we already validated that all types are valid earlier
> >
> > > +                       err = -EINVAL;
> > > +                       goto done;
> > > +               }
> > > +               type_size = btf_type_size(t);
> > > +               memcpy(nt, t, type_size);
> > > +
> > > +               /* Fix up referenced IDs for BTF */
> > > +               err = btf_field_iter_init(&it, nt, BTF_FIELD_ITER_IDS);
> > > +               if (err)
> > > +                       goto done;
> > > +               while ((type_id = btf_field_iter_next(&it))) {
> > > +                       err = btf_permute_remap_type_id(type_id, &p);
> > > +                       if (err)
> > > +                               goto done;
> > > +               }
> > > +
> > > +               nt += type_size;
> > > +       }
> > > +
> > > +       /* Fix up referenced IDs for btf_ext */
> > > +       btf_ext = OPTS_GET(opts, btf_ext, NULL);
> > > +       if (btf_ext) {
> > > +               err = btf_ext_visit_type_ids(btf_ext, btf_permute_remap_type_id, &p);
> > > +               if (err)
> > > +                       goto done;
> > > +       }
> > > +
> > > +       new_type_len = nt - new_types;
> >
> >
> > new_type_len has to be exactly the same as the old size, this is redundant
> >
> > > +       next_type = new_types;
> > > +       end_type = next_type + new_type_len;
> > > +       i = 0;
> > > +       while (next_type + sizeof(struct btf_type) <= end_type) {
> >
> > while (next_type < end_type)?
> >
> > Reference to struct btf_type is confusing, as generally type is bigger
> > than just sizeof(struct btf_type). But there is no need for this, with
> > correct code next_type < end_type is sufficient check
> >
> > But really, this can also be written cleanly as a simple for loop
> >
> > for (i = 0; i < nr_types; i++) {
> >     btf->type_offs[i] = next_type - new_types;
> >     next_type += btf_type_size(next_type);
> > }
> >
>
> Adding to what Andrii says, the whole group of assignments is
> reducible:
>
>   +       new_type_len = nt - new_types;
>   +       next_type = new_types;
>   +       end_type = next_type + new_type_len;
>
> => end_type = new_types + new_type_len; // subst next_type -> new_types
> => end_type = new_types + nt - new_types; // subst new_types -> nt - new_types
> => end_type = nt
>
> Why recomputing it in such a convoluted way?

Sorry, I referred to the implementation of btf_parse_type_sec and could have
thought more about how to refactor it.

>
> > > +               btf->type_offs[i++] = next_type - new_types;
> > > +               next_type += btf_type_size(next_type);
> > > +       }
> > > +
>
> [...]