Message-ID: <1957a60b-6c45-42a7-b525-a6e335a735ff@linux.dev>
Date: Fri, 5 Dec 2025 17:37:20 -0800
From: Ihor Solodrai <ihor.solodrai@...ux.dev>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
 <eddyz87@...il.com>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>,
 Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nsc@...nel.org>,
 Tejun Heo <tj@...nel.org>, David Vernet <void@...ifault.com>,
 Andrea Righi <arighi@...dia.com>, Changwoo Min <changwoo@...lia.com>,
 Shuah Khan <shuah@...nel.org>,
 Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
 Alan Maguire <alan.maguire@...cle.com>, Bill Wendling <morbo@...gle.com>,
 Justin Stitt <justinstitt@...gle.com>, Donglin Peng
 <dolinux.peng@...il.com>, bpf@...r.kernel.org, dwarves@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 6/6] resolve_btfids: change in-place update
 with raw binary output

On 12/5/25 5:13 PM, Andrii Nakryiko wrote:
> On Fri, Dec 5, 2025 at 2:36 PM Ihor Solodrai <ihor.solodrai@...ux.dev> wrote:
>>
>> [...]
>>
> 
> Overall it looks good, but I'd like another pair of eyes on this :)

I don't think you need to worry about another pair of eyes :)

> See some more minore nits below as well.
> 
> pw-bot: cr
> 
> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e36689cd7cc7..fe6141c69708 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4673,6 +4673,7 @@ F:        net/sched/act_bpf.c
>>  F:     net/sched/cls_bpf.c
>>  F:     samples/bpf/
>>  F:     scripts/bpf_doc.py
>> +F:     scripts/gen-btf.sh
>>  F:     scripts/Makefile.btf
>>  F:     scripts/pahole-version.sh
>>  F:     tools/bpf/
>> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
>> index 7c1cd6c2ff75..d067e91049cb 100644
>> --- a/scripts/Makefile.btf
>> +++ b/scripts/Makefile.btf
>> @@ -1,5 +1,10 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>
>> +gen-btf-y                              =
>> +gen-btf-$(CONFIG_DEBUG_INFO_BTF)       = $(srctree)/scripts/gen-btf.sh
>> +
>> +export GEN_BTF := $(gen-btf-y)
>> +
> 
> What's the point of GEN_BTF? It's just so that you don't have to have
> $(srctree)/scripts/gen-btf.sh specified in three places? Between
> obscure $(GEN_BTF) (and having to understand where it is set and how
> it's exported) and explicit $(srctree)/scripts/gen-btf.sh in a few
> places, I'd prefer the latter, as it is way more greppable and it's
> not like we are going to rename or move this script frequently

Yeah... It seems fine for me either way, tbh.

I think a slight difference is that in link-vmlinux.sh checking for a
config value is a grep on include/config/auto.conf, which is not
necessary if we have a dedicated env variable.

> 
> 
>>  pahole-ver := $(CONFIG_PAHOLE_VERSION)
>>  pahole-flags-y :=
>>
> 
> [...]
> 
>> @@ -371,7 +348,7 @@ static int elf_collect(struct object *obj)
>>
>>         elf_version(EV_CURRENT);
>>
>> -       elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL);
>> +       elf = elf_begin(fd, ELF_C_READ_MMAP_PRIVATE, NULL);
>>         if (!elf) {
>>                 close(fd);
>>                 pr_err("FAILED cannot create ELF descriptor: %s\n",
>> @@ -434,21 +411,20 @@ static int elf_collect(struct object *obj)
>>                         obj->efile.symbols_shndx = idx;
>>                         obj->efile.strtabidx     = sh.sh_link;
>>                 } else if (!strcmp(name, BTF_IDS_SECTION)) {
>> +                       /*
>> +                        * If target endianness differs from host, we need to bswap32
>> +                        * the .BTF_ids section data on load, because .BTF_ids has
>> +                        * Elf_Type = ELF_T_BYTE, and so libelf returns data buffer in
>> +                        * the target endiannes. We repeat this on dump.
> 
> gmail screams at me for "endianness"

That's how you know I am not AI.

> 
>> +                        */
>> +                       if (obj->efile.encoding != ELFDATANATIVE) {
>> +                               pr_debug("bswap_32 .BTF_ids data from target to host endianness\n");
>> +                               bswap_32_data(data->d_buf, data->d_size);
> 
> this looks like a violation of ELF_C_READ_MMAP_PRIVATE promise, no?...
> would it be too create a copy here? for simplicity we can just always
> malloc() a copy, regardless of bswap(), it can never be a huge amount
> of data

No, it's not a violation. From libelf.h:

  ELF_C_READ_MMAP_PRIVATE,	/* Read, but memory is writable, results are
				   not written to the file.  */

I haven't checked how it works under the hood, but from testing so far
the comment seems to be telling the truth.


> 
>> +                       }
>>                         obj->efile.idlist       = data;
>>                         obj->efile.idlist_shndx = idx;
>>                         obj->efile.idlist_addr  = sh.sh_addr;
>> -               } else if (!strcmp(name, BTF_BASE_ELF_SEC)) {
>> -                       /* If a .BTF.base section is found, do not resolve
>> -                        * BTF ids relative to vmlinux; resolve relative
>> -                        * to the .BTF.base section instead.  btf__parse_split()
>> -                        * will take care of this once the base BTF it is
>> -                        * passed is NULL.
>> -                        */
>> -                       obj->base_btf_path = NULL;
>>                 }
>> -
>> -               if (compressed_section_fix(elf, scn, &sh))
>> -                       return -1;
>>         }
>>
>>         return 0;
>> @@ -552,6 +528,13 @@ static int symbols_collect(struct object *obj)
>>         return 0;
>>  }
>>
>> +static inline bool is_envvar_set(const char *var_name)
>> +{
>> +       const char *value = getenv(var_name);
>> +
>> +       return value && value[0] != '\0';
>> +}
>> +
> 
> leftovers?

Yes. I think I should add "fail build on warnings" for resolve_btfids.

> 
>>  static int load_btf(struct object *obj)
>>  {
>>         struct btf *base_btf = NULL, *btf = NULL;
>> @@ -578,6 +561,19 @@ static int load_btf(struct object *obj)
>>         obj->base_btf = base_btf;
>>         obj->btf = btf;
>>
>> +       if (obj->base_btf && obj->distill_base) {
>> +               err = btf__distill_base(obj->btf, &base_btf, &btf);
>> +               if (err) {
>> +                       pr_err("FAILED to distill base BTF: %s\n", strerror(errno));
>> +                       goto out_err;
>> +               }
>> +
>> +               btf__free(obj->btf);
>> +               btf__free(obj->base_btf);
>> +               obj->btf = btf;
>> +               obj->base_btf = base_btf;
>> +       }
>> +
>>         return 0;
>>
>>  out_err:
> 
> [...]
> 
>> +static int dump_raw_btf_ids(struct object *obj, const char *out_path)
>> +{
>> +       Elf_Data *data = obj->efile.idlist;
>> +       int fd, err;
>> +
>> +       if (!data || !data->d_buf) {
>> +               pr_debug("%s has no BTF_ids data to dump\n", obj->path);
>> +               return 0;
>> +       }
>> +
>> +       /*
>> +        * If target endianness differs from host, we need to bswap32 the
>> +        * .BTF_ids section data before dumping so that the output is in
>> +        * target endianness.
>> +        */
>> +       if (obj->efile.encoding != ELFDATANATIVE) {
>> +               pr_debug("bswap_32 .BTF_ids data from host to target endianness\n");
>> +               bswap_32_data(data->d_buf, data->d_size);
> 
> same about modifying ELF data in-place for what is supposed to be read-only use

See above.

> 
>> +       }
>> +
>> +       err = dump_raw_data(out_path, data->d_buf, data->d_size);
>> +       if (err)
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> +
>> +static int dump_raw_btf(struct btf *btf, const char *out_path)
>> +{
>> +       const void *raw_btf_data;
>> +       u32 raw_btf_size;
>> +       int fd, err;
>> +
>> +       raw_btf_data = btf__raw_data(btf, &raw_btf_size);
>> +       if (!raw_btf_data) {
>> +               pr_err("btf__raw_data() failed\n");
>> +               return -1;
>> +       }
> 
> did you check that libbpf does proper byte swap as well?

libbpf is tracking BTF endianness internally, we don't have to worry
about it here. If it wasn't, s390x selftests would have almost
certainly failed (and they did when I messed up).

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/btf.c?h=v6.18#n227

> 
>> +
>> +       err = dump_raw_data(out_path, raw_btf_data, raw_btf_size);
>> +       if (err)
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> +
>> +static inline int make_out_path(char *buf, const char *in_path, const char *suffix)
>> +{
>> +       int len = snprintf(buf, PATH_MAX, "%s%s", in_path, suffix);
> 
> nit: normally you pass buffer and its size as input arguments instead
> of assuming and hard-coding common PATH_MAX constant in two separate
> places

acked

> 
>> +
>> +       if (len < 0 || len >= PATH_MAX) {
>> +               pr_err("Output path is too long: %s%s\n", in_path, suffix);
>> +               return -E2BIG;
>>         }
>>
>> -       pr_debug("update %s for %s\n",
>> -                err >= 0 ? "ok" : "failed", obj->path);
>> -       return err < 0 ? -1 : 0;
>> +       return 0;
>>  }
>>
>>  static const char * const resolve_btfids_usage[] = {
> 
> [...]

Powered by blists - more mailing lists