Re: [PATCH bpf-next v2 15/20] libbpf: Collect .bpf_cleanup records and pass them to the kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: empty cleanup range check in bpf_prog_collect_cleanup_info() only catches offset 0
- [Medium] bpf: bpf_object__init_cleanup_info() rejects valid objects with 0-sized .bpf_cleanup sections
--

commit a645d3bd249aceee4909edcc78e5fe4247fd3432
Author: Yonghong Song <yonghong.song@linux.dev>

libbpf: Collect .bpf_cleanup records and pass them to the kernel

Parse the compiler-emitted .bpf_cleanup section and hand the resulting
table to BPF_PROG_LOAD.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 27f2eec13f76f..1b8b978e7b7b9 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -4852,6 +4890,221 @@ static struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,
>  	return NULL;
>  }
>  
> +static int bpf_object__init_cleanup_info(struct bpf_object *obj)
> +{
> +	Elf_Data *data = obj->efile.cleanup_data;
> +	Elf_Data *relo = NULL;
> +	size_t i, nrels, nslots, nrecs;
> +	struct cleanup_raw_rec *recs;
> +	int *slot_sec, ret = 0;
> +	size_t *slot_val;
> +	const __u32 *vals;
> +	bool native;
> +
> +	if (!data || obj->efile.cleanup_shndx < 0)
> +		return 0;
> +
> +	native = is_native_endianness(obj);
> +
> +	for (i = 0; i < obj->efile.sec_cnt; i++) {
> +		struct elf_sec_desc *sd = &obj->efile.secs[i];
> +
> +		if (sd->sec_type == SEC_RELO && sd->shdr &&
> +		    sd->shdr->sh_info == (Elf64_Word)obj->efile.cleanup_shndx) {
> +			relo = sd->data;
> +			break;
> +		}
> +	}
> +	if (!relo) {
> +		pr_warn("%s present without relocations\n", CLEANUP_SEC);
> +		return -LIBBPF_ERRNO__FORMAT;
> +	}

[Severity: Medium]
Could this reject valid objects if the compiler emits an empty 0-byte
.bpf_cleanup section?

If the section is empty (e.g., exceptions were optimized out), no relocations
are emitted and there is no corresponding .rel.bpf_cleanup section. However,
bpf_object__init_cleanup_info() requires the relocation section to exist if
.bpf_cleanup is present.

Would it be better to return early if data->d_size is 0 before checking for
relocations?

[ ... ]

> +static int bpf_prog_collect_cleanup_info(struct bpf_object *obj,
> +					 struct bpf_program *prog)
> +{
> +	size_t i;
> +	int j;
> +
> +	for (i = 0; i < obj->cleanup_rec_cnt; i++) {
> +		struct cleanup_raw_rec *raw = &obj->cleanup_recs[i];
> +		struct bpf_program *owner = NULL;
> +		struct bpf_cleanup_info ci = {};
> +		__u32 fields[CLEANUP_REC_FIELDS];
> +		void *tmp;
> +
> +		for (j = 0; j < CLEANUP_REC_FIELDS; j++) {
> +			size_t idx = raw->insn_idx[j], final;
> +			struct bpf_program *p;
> +
> +			/* The end of a range is exclusive, so it may name the
> +			 * instruction just past the last one of a function,
> +			 * which belongs to the next function or to nothing at
> +			 * all. Ask about the last instruction the range covers,
> +			 * the way the kernel does.
> +			 */
> +			if (j == CLEANUP_REC_END) {
> +				if (!idx) {
> +					pr_warn("%s: record %zu is an empty range\n",
> +						CLEANUP_SEC, i);
> +					return -LIBBPF_ERRNO__FORMAT;
> +				}
> +				idx--;
> +			}

[Severity: Medium]
Will this check catch empty ranges emitted by the compiler at non-zero
offsets?

If the compiler emits a .bpf_cleanup record where begin and end are both 5,
the check for !idx evaluates to false. The code will then decrement idx to 4
and look up the program for the previous instruction.

If this empty range is located at the start of a function, looking up the
previous instruction will fall into the preceding function, potentially
triggering a spurious "spans functions" error or silently accepting invalid
records.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918044156.3283973-1-yonghong.song@linux.dev?part=15