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

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

On 9/16/26 11:12 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] bpf: undefined behavior when calling qsort with a NULL pointer
> - [Low] bpf: array bounds violation via type-punned pointer arithmetic
> --
>
> commit 1e591245d7ec663b872eae44c8fa7f02cfd956a4
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> libbpf: Collect .bpf_cleanup records and pass them to the kernel
>
> This commit parses the compiler-emitted .bpf_cleanup section and hands
> the resulting table to BPF_PROG_LOAD so the kernel can resolve exceptions.
>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index ea1c09fa37939..d7cb93a70f58d 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
> [ ... ]
>
>> @@ -4847,6 +4885,214 @@ static struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,
> [ ... ]
>
>> +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 = (__u32 *)&ci;
>> +		void *tmp;
>> +
>> +		for (j = 0; j < CLEANUP_REC_FIELDS; j++) {
>> +			size_t idx = raw->insn_idx[j], final;
>> +			struct bpf_program *p;
> [ ... ]
>
>> +			if (owner == prog) {
>> +				final = raw->insn_idx[j] - prog->sec_insn_off;
>> +			} else if (prog_is_subprog(obj, owner) && owner->sub_insn_off) {
>> +				/* sub_insn_off is where this subprogram was
>> +				 * appended to the main program being relocated;
>> +				 * zero means it is not part of it.
>> +				 */
>> +				final = owner->sub_insn_off +
>> +					raw->insn_idx[j] - owner->sec_insn_off;
>> +			} else {
>> +				owner = NULL;
>> +				break;
>> +			}
>> +			fields[j] = final;
> [Severity: Low]
> Is it safe to cast a pointer to a struct field into an array pointer and access
> subsequent members via pointer arithmetic in this loop?
>
> The struct bpf_cleanup_info fields are populated by casting a pointer to the
> struct to a __u32 * array and indexing into it in a loop.
>
> According to C11 6.5.6, pointer arithmetic on a pointer to a scalar (the first
> struct field) only allows accessing up to one element past it, making fields[1]
> and fields[2] undefined behavior.
>
> This can trigger compiler warnings (-Warray-bounds) and potentially cause
> aggressive optimizers to generate incorrect code.

Yes, the code is correct but it is awkward and my trigger compiler warnings with
additional compiler flag. Will make it explicit with bpf_cleanup_info fields.

>
>> +		}
>> +		if (!owner)
>> +			continue;
>> +
>> +		tmp = libbpf_reallocarray(prog->cleanup_info, prog->cleanup_info_cnt + 1,
>> +					  sizeof(*prog->cleanup_info));
>> +		if (!tmp)
>> +			return -ENOMEM;
>> +		prog->cleanup_info = tmp;
>> +		prog->cleanup_info_rec_size = sizeof(struct bpf_cleanup_info);
>> +		prog->cleanup_info[prog->cleanup_info_cnt++] = ci;
>> +
>> +		pr_debug("prog '%s': cleanup region [%u,%u) -> landing pad %u\n",
>> +			 prog->name, ci.begin_off, ci.end_off, ci.landing_pad_off);
>> +	}
>> +
>> +	qsort(prog->cleanup_info, prog->cleanup_info_cnt,
>> +	      sizeof(*prog->cleanup_info), cmp_cleanup_info);
> [Severity: Medium]
> Can this unconditionally call qsort() with a NULL base pointer?

You are right. It is possible prog->cleanup_info_cnt is 0 and in such cases,
cleanup_info will be NULL. Will add a condition like 'prog->cleanup_info_cnt > 0'.

>
> If obj->cleanup_rec_cnt is 0, or if none of the records match the program,
> prog->cleanup_info_cnt remains 0 and prog->cleanup_info will be NULL.
>
> Calling qsort() with a NULL pointer invokes undefined behavior under C11 7.1.4,
> even if the element count is 0. This triggers UBSAN errors (nonnull attribute
> violation in libc headers) and can fail runtime checks.
>