Re: [PATCH v4 3/3] libbpf: plumb btf_vmlinux_value_type_id and btf_fd in gen_loader

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The patch truncates the bpf_attr payload, dropping `value_type_btf_obj_fd`, which breaks module-backed STRUCT_OPS maps in gen_loader.

Pre-existing issues:
- [Critical] Double-close of `inner_map_fd` in the gen_loader cleanup path can hijack and close an unrelated file descriptor.
- [High] The condition to populate `btf_fd` incorrectly omits `btf_key_type_id`, causing map creation to fail if only a key BTF type is defined.
- [High] Memory leak of dynamic arrays `gen->relos`, `gen->ksyms`, and `gen->core_relos` in the error path.
--

commit b58090bc2b448f8a5c71dbba37463b5a9e73fdd5
Author: Siddharth Nayyar <sidnayyar@google.com>

libbpf: plumb btf_vmlinux_value_type_id and btf_fd in gen_loader

This commit updates the BPF map creation generator to plumb the kernel-side
structure value type ID (btf_vmlinux_value_type_id) into the map creation
system call attributes. It also modifies the condition for copying the
userspace BTF file descriptor so it populates btf_fd when either
btf_value_type_id or btf_vmlinux_value_type_id is set.

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -525,13 +525,14 @@ void bpf_gen__map_create(struct bpf_gen *gen,
>  	attr.max_entries = tgt_endian(max_entries);
>  	attr.btf_key_type_id = tgt_endian(map_attr->btf_key_type_id);
>  	attr.btf_value_type_id = tgt_endian(map_attr->btf_value_type_id);
> +	attr.btf_vmlinux_value_type_id = tgt_endian(map_attr->btf_vmlinux_value_type_id);

[Severity: High]
Does this code truncate the bpf_attr payload and drop value_type_btf_obj_fd?

At the top of bpf_gen__map_create(), attr_size is calculated as
offsetofend(union bpf_attr, map_extra). However, value_type_btf_obj_fd was
introduced after map_extra in the union. By plumbing btf_vmlinux_value_type_id
here without expanding attr_size, the loader may send a truncated payload to
the kernel.

For module-backed struct_ops maps, libbpf sets the BPF_F_VTYPE_BTF_OBJ_FD flag,
but the kernel would receive value_type_btf_obj_fd = 0 (stdin). Could this
cause btf_get_by_fd(0) to fail and break map creation?

>  
>  	map_create_attr = add_data(gen, &attr, attr_size);
>  	pr_debug("gen: map_create: %s idx %d type %d value_type_id %d, attr: off %d size %d\n",
>  		 map_name, map_idx, map_type, map_attr->btf_value_type_id,
>  		 map_create_attr, attr_size);
>  
> -	if (map_attr->btf_value_type_id)
> +	if (map_attr->btf_value_type_id || map_attr->btf_vmlinux_value_type_id)

[Severity: High]
This is a pre-existing issue, but does this condition incorrectly omit
btf_key_type_id?

If a map defines only a BTF key type but no value type, btf_key_type_id will be
non-zero while btf_value_type_id remains zero.

This means gen_loader would skip populating btf_fd in this case, sending
btf_fd = 0 alongside the non-zero btf_key_type_id. Since the kernel strictly
validates that btf_fd must be valid if any BTF type ID is set, will the kernel
reject this with -EINVAL?

>  		/* populate union bpf_attr with btf_fd saved in the stack earlier */
>  		move_stack2blob(gen, attr_field(map_create_attr, btf_fd), 4,
>  				stack_off(btf_fd));

[Severity: Critical]
This is a pre-existing issue, but can the inner map handling later in this
function lead to a double-close regression in the cleanup path?

When emit_sys_close_stack() is called for inner_map_fd, it issues the sys_close
BPF helper but doesn't zero out the memory slot containing the file descriptor
on the loader stack.

If a subsequent command in the loader program fails, the cleanup routine loops
over the loader stack and calls sys_close on any file descriptor greater than
zero. Because the inner_map_fd slot was never cleared, it gets closed a second
time. If another thread opened a new file or socket in the interim, could this
hijack and close the unrelated resource?

[Severity: High]
This is a pre-existing issue, but is there a memory leak in the error path for
the dynamic arrays used during generation?

The arrays gen->relos, gen->ksyms, and gen->core_relos are dynamically allocated
using libbpf_reallocarray(). They are expected to be freed by cleanup_relos()
at the end of bpf_gen__prog_load().

If the loader encounters an error earlier in the process, the program aborts
and bpf_gen__free() is called. However, bpf_gen__free() appears to fail to free
these arrays or call cleanup_relos(), causing a permanent memory leak. For
long-running daemons parsing untrusted objects, could this be exploited to
cause an out-of-memory crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-libbpf-load-vmlinux-btf-in-gen_loader-mode-v4-0-27daa15487a9@google.com?part=3