Re: [PATCH bpf-next v2 14/20] libbpf: Add cleanup_info to bpf_prog_load_opts

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

On 9/17/26 9:57 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] libbpf: missing E2BIG fallback for cleanup_info breaks forward compatibility
> --
>
> commit d67b69538e39a329708fcc9fa943f0ad05afaa05
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> libbpf: Add cleanup_info to bpf_prog_load_opts
>
> Let a caller hand the kernel an exception cleanup table. Extend struct
> bpf_prog_load_opts with cleanup_info, cleanup_info_cnt and
> cleanup_info_rec_size, pass them through to BPF_PROG_LOAD, and grow the
> attr size bpf_prog_load() computes to cover the new fields.
>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 96819c082c772..bcf490570961c 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
> [ ... ]
>> @@ -370,6 +370,10 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
>>   	attr.fd_array = ptr_to_u64(OPTS_GET(opts, fd_array, NULL));
>>   	attr.fd_array_cnt = OPTS_GET(opts, fd_array_cnt, 0);
>>   
>> +	attr.cleanup_info = ptr_to_u64(OPTS_GET(opts, cleanup_info, NULL));
>> +	attr.cleanup_info_cnt = OPTS_GET(opts, cleanup_info_cnt, 0);
>> +	attr.cleanup_info_rec_size = OPTS_GET(opts, cleanup_info_rec_size, 0);
> [Severity: Medium]
> Will this break forward compatibility for bpf_prog_load() if cleanup_info
> triggers an -E2BIG error?
>
> Later in bpf_prog_load(), there is a retry loop to handle -E2BIG when the
> userspace-provided record size is larger than what the kernel supports:
>
> 	while (errno == E2BIG && (!finfo || !linfo)) {
> 		if (!finfo && attr.func_info_cnt &&
> 		    attr.func_info_rec_size < func_info_rec_size) {
>              /* try with corrected func info records */
>              ...
>
> The loop condition and body don't seem to include checks for cleanup_info.
>
> If the kernel returns -E2BIG because of a larger cleanup_info_rec_size,
> won't this hit the else { break; } block or skip the loop entirely, causing
> the load to fail instead of retrying with truncated records?

This is the first version to support .bpf_cleanup. It is too early to
talk about the future extension for .bpf_cleanup although we do have some
room to grow struct size if needed:
   #define MAX_FUNCINFO_REC_SIZE   252
   #define MAX_CLEANUP_INFO_REC_SIZE       MAX_FUNCINFO_REC_SIZE

>
>> +
>>   	if (log_level) {
>>   		attr.log_buf = ptr_to_u64(log_buf);
>>   		attr.log_size = log_size;