Re: [PATCH RFC bpf-next 4/7] libbpf: Support BPF_PROG_TYPE_STRUCT_OPS in bpf_gen__prog_load()

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4075 bytes --]

> libbpf: Support BPF_PROG_TYPE_STRUCT_OPS in bpf_gen__prog_load()

TL;DR: In light skeletons, a kernel-module BTF fd for struct_ops may leak
(stay open in the loader's fd table) if any struct_ops program load fails,
since no error-path close is emitted.

> 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

[ ... ]

> @@ -1151,6 +1151,20 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
>  		emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_7,
>  				      offsetof(union bpf_attr, attach_btf_obj_fd)));
>  	}
> +	if (prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> +		move_blob2blob(gen, attr_field(prog_load_attr, expected_attach_type), 4,
> +			       load_attr->expected_attach_type);
> +		emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX_VALUE,
> +						 0, 0, 0, load_attr->attach_btf_id));
> +		emit(gen, BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, 0));
> +		emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_0, BPF_PSEUDO_MAP_IDX_VALUE,
> +						 0, 0, 0, prog_load_attr));
> +		emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_7,
> +				      offsetof(union bpf_attr, attach_btf_id)));
> +		emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32));
> +		emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_7,
> +				      offsetof(union bpf_attr, attach_btf_obj_fd)));
> +	}
>  	emit_relos(gen, insns_off);
>  	/* emit PROG_LOAD command */
>  	emit_sys_bpf(gen, BPF_PROG_LOAD, prog_load_attr, attr_size);

For a struct_ops type defined in a kernel module, can the module BTF fd
leak into the process running the light skeleton when a struct_ops
program fails to load?

The block above copies the upper 32 bits of the u64 at
load_attr->attach_btf_id into prog_load_attr.attach_btf_obj_fd.  That
value was produced at runtime by bpf_btf_find_by_name_kind() in
bpf_gen__init_kern_struct_ops().  For a module type it creates a new fd
with __btf_new_fd() and returns:

    ret | ((u64)btf_obj_fd << 32)

so this is an open fd owned by the loader.

The existing code closes module BTF fds whether the load succeeds or not:

    /* successful or not, close btf module FDs used in extern ksyms and attach_btf_obj_fd */
    cleanup_relos(gen, insns_off);
    if (gen->attach_kind) {
            emit_sys_close_blob(gen,
                                attr_field(prog_load_attr, attach_btf_obj_fd));
            gen->attach_kind = 0;
    }
    emit_check_err(gen);

bpf_gen__map_create() does the same for value_type_btf_obj_fd, but the
struct_ops path here does not.  The fd cannot be closed here because all
programs of the map share it.  The only close is in
bpf_gen__struct_ops_update_elem(), added later in the series, which is
emitted from bpf_object_prepare_struct_ops() after all programs are
loaded and so only runs on success.

If BPF_PROG_LOAD fails for any struct_ops program (for example a verifier
rejection), emit_check_err() jumps to gen->cleanup_label.  The cleanup
code emitted by bpf_gen__init() only closes the prog_fd[] stack slots and
the map fds in the blob fd_array:

    for (i = 0; i < nr_progs_sz; i += 4) { ... BPF_FUNC_sys_close ... }
    for (i = 0; i < nr_maps; i++)
            emit_sys_close_blob(gen, blob_fd_array_off(gen, i));

Nothing there knows about st_ops->type_id_off, so the module BTF fd of
every struct_ops map initialised so far stays open in the caller's fd
table.  The same applies to any later failure before
bpf_gen__struct_ops_update_elem() runs, such as another program's load or
an emit_check_err() in emit_relos().  vmlinux struct_ops types are not
affected, since the upper 32 bits are 0 and __emit_sys_close() skips
fd <= 0.

No later commit in the series adds an error-path close.  Should these fds
be added to the cleanup_label path, or closed in an error branch around
the prog-load emit_check_err()?


---
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/42289585064