Re: [PATCH RFC bpf-next 5/7] libbpf: Implement bpf_gen__struct_ops_update_elem() for light skeletons

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] libbpf: wrong prog_fd[] stack index in
  bpf_gen__struct_ops_update_elem()
- [Low] libbpf: module BTF FD leak on error path in
  bpf_gen__struct_ops_update_elem()
--

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

libbpf: Implement bpf_gen__struct_ops_update_elem() for light skeletons

This commit adds bpf_gen__struct_ops_update_elem() and calls it from
bpf_object_prepare_struct_ops() to close attach_btf_obj_fd, copy callback
program file descriptors into kern_vdata, and emit the map update for
struct_ops maps in light skeletons.

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index aaa491463521a..6dcba83dcc098 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -1573,3 +1573,61 @@ void bpf_gen__init_kern_struct_ops(struct bpf_gen *gen, struct btf *btf,
>  		}
>  	}
>  }
> +
> +void bpf_gen__struct_ops_update_elem(struct bpf_gen *gen, int map_idx,
> +				     struct btf *btf,
> +				     struct bpf_struct_ops *st_ops)
> +{
> +	int attr_size = offsetofend(union bpf_attr, flags);
> +	const struct btf_type *type;
> +	union bpf_attr attr;
> +	int key, zero = 0;
> +	__u32 i;
> +
> +	/* 1. Close attach_btf_obj_fd (upper 32 bits of st_ops->type_id_off) */
> +	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_0, BPF_PSEUDO_MAP_IDX_VALUE,
> +					 0, 0, 0, st_ops->type_id_off));
> +	emit(gen, BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 0));
> +	emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_1, 32));
> +	__emit_sys_close(gen);

[Severity: Low]
Does this leak attach_btf_obj_fd if an earlier runtime check fails and jumps
to cleanup_label?

When the target struct_ops type is in a kernel module,
bpf_gen__init_kern_struct_ops() stores the module btf file descriptor in the
upper 32 bits of st_ops->type_id_off before map creation and program loading
are emitted.  Because gen->cleanup_label only closes stack file descriptors
and fd_array map file descriptors, any emit_check_err() failure before
reaching bpf_gen__struct_ops_update_elem() leaves attach_btf_obj_fd open.

> +
> +	/* 2. Populate prog_fd into kern_vdata for all callback members */
> +	type = btf__type_by_id(btf, st_ops->type_id);
> +	for (i = 0; i < btf_vlen(type); i++) {
> +		struct bpf_program *prog = st_ops->progs[i];
> +		int prog_idx, off_slot;
> +
> +		if (!prog)
> +			continue;
> +
> +		prog_idx = bpf_program_idx(prog);
> +		off_slot = st_ops->kern_func_off[i];
> +
> +		/* R2 = dynamic kernel member offset from off_slot */
> +		emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX_VALUE,
> +						 0, 0, 0, off_slot));
> +		emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, 0));
> +		emit(gen, BPF_JMP_IMM(BPF_JLE, BPF_REG_2,
> +				      KERN_VDATA_MAX_SIZE - sizeof(__u64), 2));
> +		emit(gen, BPF_MOV64_IMM(BPF_REG_7, -E2BIG));
> +		emit_check_err(gen);
> +
> +		/* R1 = kern_vdata_off + offset */
> +		emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX_VALUE,
> +						 0, 0, 0, st_ops->kern_vdata_off));
> +		emit(gen, BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2));
> +
> +		/* R0 = prog_fd from stack (stack_off(prog_fd[prog_idx])) */
> +		emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_10,
> +				      stack_off(prog_fd[prog_idx])));

[Severity: Medium]
Can stack_off(prog_fd[prog_idx]) read the wrong stack slot or go out of
bounds when obj->programs[] contains subprograms or non-autoloaded programs?

bpf_program_idx() returns prog - prog->obj->programs, whereas
bpf_object__load_progs() skips subprograms (such as functions in .text) and
non-autoloaded programs, and bpf_gen__prog_load() stores each loaded
program's file descriptor sequentially at stack_off(prog_fd[gen->nr_progs]):

tools/lib/bpf/gen_loader.c:bpf_gen__prog_load() {
	...
	/* remember prog_fd in the stack, if successful */
	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_7,
			      stack_off(prog_fd[gen->nr_progs])));
	gen->nr_progs++;
}

When any skipped program precedes prog in obj->programs[], prog_idx does not
match the program's slot in prog_fd[], so the generated loader either writes
the wrong file descriptor into kern_vdata or emits an out-of-bounds stack
read that the verifier rejects if prog_idx exceeds the prog_fd[] array size.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-lskel-struct-ops-v1-0-a4486ffc79cb@google.com?part=5