Re: [PATCH bpf-next v8 15/22] libbpf: Add cleanup_info to bpf_prog_load_opts

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

On 10/1/26 6:46 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] bpf: ABI breakage due to reuse of padding in bpf_prog_load_opts
> --
>
> commit 71e2541e0b207ab646c97a6cfe861ce9bf16fe7f
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> libbpf: Add cleanup_info to bpf_prog_load_opts
>
> This commit introduces an exception cleanup table to the bpf_prog_load
> options. It adds cleanup_info_cnt, cleanup_info, and cleanup_info_rec_size
> to struct bpf_prog_load_opts and propagates them to the kernel in
> bpf_prog_load.
>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index b49822d212aed..b4031f62bdee5 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_rec_size = OPTS_GET(opts, cleanup_info_rec_size, 0);
>> +	attr.cleanup_info_cnt = OPTS_GET(opts, cleanup_info_cnt, 0);
>> +
> [Severity: High]
> Does this OPTS_GET macro read uninitialized padding from older applications?
> When an application compiled against an older version of libbpf passes its
> opts structure here, the sz field will be set to the old struct size.
>
> If the old struct had padding at the end that the new structure replaces
> with cleanup_info_cnt, OPTS_GET might think the field is present because
> the sz value includes the padding, causing it to read stack garbage.
>
>>   	if (log_level) {
>>   		attr.log_buf = ptr_to_u64(log_buf);
>>   		attr.log_size = log_size;
>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> index 826d9cc9ab65d..cbe56ddc8cf7d 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
> [ ... ]
>> @@ -128,9 +128,14 @@ struct bpf_prog_load_opts {
>>   
>>   	/* if set, provides the length of fd_array */
>>   	__u32 fd_array_cnt;
>> +
>> +	/* exception cleanup table, from the .bpf_cleanup section */
>> +	__u32 cleanup_info_cnt;
> [Severity: High]
> Does placing cleanup_info_cnt immediately after fd_array_cnt break the
> size-based backward compatibility check?
>
> Because fd_array_cnt is a 4-byte field, the struct bpf_prog_load_opts in
> older applications likely contained a 4-byte padding hole at the end to
> align with the 8-byte size_t type.
>
> An older application would pass an opts structure where sz includes those 4
> bytes of padding. Because OPTS_HAS evaluates if sz >=
> offsetofend(struct bpf_prog_load_opts, cleanup_info_cnt), it will evaluate
> to true. The library will then read the uninitialized padding left by the
> older application as the cleanup_info_cnt value.
>
> If the padding contains garbage, the kernel could reject the program load
> or fail unexpectedly. Could this be resolved by placing cleanup_info_cnt
> after the pointer, or by introducing explicit padding?
>
>> +	const void *cleanup_info;
>> +	__u32 cleanup_info_rec_size;
>>   	size_t :0;

I think the current implementation is okay. IIUC, 'size_t :0' will ensure
to filling '0''s for tailing unnamed fields.