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

[Yonghong Song <yonghong.song@linux.dev>]

On 9/17/26 10:00 PM, sashiko-bot@kernel.org wrote:
> 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?

I think it is okay, as you mentioned, there will be one error message later
when relocation section checks.

>
> [ ... ]
>
>> +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.

Good point. Will fix this corner case.

>