> resolve_btfids: Populate BTF link section
TL;DR: Nits only, no bugs found: patch_btf_link() duplicates much of
patch_btfids() and reopens vmlinux repeatedly, runs a full elf_collect() just
for class/encoding, and could use ensure_mem().
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 15e7be13061f..f737fac60fa2 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
[ ... ]
> @@ -267,6 +282,49 @@ static int __ensure_mem(void **data, u32 *cap, u32 cnt,
> size_t elem_sz)
> #define ensure_mem(arr_ptr, cap_ptr, cnt) \
> __ensure_mem((void **)(arr_ptr), (cap_ptr), (cnt), sizeof(**(arr_ptr)))
>
> +static int parse_btf_link(const struct option *opt, const char *arg, int
> unset)
> +{
> + struct btf_links *links = opt->value;
> + struct btf_link *link;
> + char *separator;
> +
> + if (unset)
> + return -EINVAL;
> + if (__ensure_mem((void **)&links->links, &links->cap, links->cnt + 1,
> + sizeof(*links->links)))
> + return -ENOMEM;
this isn't a bug, but could parse_btf_link() use the ensure_mem() macro
defined right above it, the way the other callers in main.c do?
ensure_mem(&links->links, &links->cap, links->cnt + 1) would expand to the
same __ensure_mem() call.
[ ... ]
> @@ -1030,6 +1089,153 @@ static int dump_raw_btf(struct btf *btf, const char
> *out_path)
> return 0;
> }
>
> +static int patch_btf_link(struct object *obj, const struct btf_link *link)
> +{
> + char section[128];
[ ... ]
> + fd = open(obj->path, O_RDWR);
> + if (fd < 0) {
> + err = -errno;
> + pr_err("FAILED to open %s: %s\n", obj->path, strerror(errno));
> + goto out;
> + }
> + elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL);
> + if (!elf) {
> + err = -EINVAL;
> + pr_err("FAILED to create ELF descriptor for %s: %s\n",
> + obj->path, elf_errmsg(-1));
> + goto out;
> + }
> + elf_flagelf(elf, ELF_C_SET, ELF_F_LAYOUT);
> + if (elf_getshdrstrndx(elf, &shdrstrndx)) {
> + pr_err("FAILED to get section name index from %s: %s\n",
> + obj->path, elf_errmsg(-1));
> + goto out;
> + }
> + while ((scn = elf_nextscn(elf, scn))) {
> + if (gelf_getshdr(scn, &sh) != &sh) {
> + pr_err("FAILED to get section header from %s: %s\n",
> + obj->path, elf_errmsg(-1));
> + goto out;
> + }
> + name = elf_strptr(elf, shdrstrndx, sh.sh_name);
> + if (name && !strcmp(name, section))
> + break;
> + }
> + if (!scn) {
> + pr_err("FAILED: section %s not found in %s\n", section,
> obj->path);
> + goto out;
> + }
> + data = elf_getdata(scn, NULL);
> + if (sh.sh_type != SHT_PROGBITS || !data || !data->d_buf ||
> + data->d_size != module_name_len +
> + BTF_SHA256_DIGEST_LENGTH + sizeof(u32)) {
> + pr_err("FAILED: section %s is not a writable BTF link\n",
> section);
> + goto out;
> + }
this isn't a bug, but most of patch_btf_link() repeats what patch_btfids()
already does: the open(O_RDWR) / elf_begin(ELF_C_RDWR_MMAP) /
elf_getshdrstrndx() setup, the loop that looks up a section by name,
elf_getdata(), and the elf_flagdata(ELF_F_DIRTY) / elf_update(ELF_C_WRITE) /
elf_end() / close() teardown.
This also means the final pass opens and rewrites vmlinux once for
.BTF_ids and then again for each --btf_link.
Could patch_btfids() and patch_btf_link() share a helper that opens the ELF
and finds a section by name, or even use a single RDWR descriptor?
[ ... ]
> @@ -1916,12 +2125,30 @@ int main(int argc, const char **argv)
[ ... ]
> obj.path = argv[0];
>
> - if (btfids_path)
> - return patch_btfids(btfids_path, obj.path);
> + if (btfids_path) {
> + err = patch_btfids(btfids_path, obj.path);
> + if (err || !btf_links.cnt)
> + goto out;
> + if (elf_collect(&obj)) {
> + err = -EINVAL;
> + goto out;
> + }
this isn't a bug, but the --btf_link path runs the whole elf_collect() pass
only to get EI_CLASS and EI_DATA for patch_btf_link().
elf_collect() opens a second descriptor and mmap of the target, walks every
section, and byte-swaps .BTF_ids in its private map. patch_btf_link() then
opens the same file again with its own Elf handle.
Since patch_btf_link() already calls elf_begin(), could it read the class
and data encoding directly with gelf_getehdr() / gelf_getclass(), so that
this elf_collect() call and the new efile.elf_class field aren't needed?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37924786344
