Re: [PATCH bpf-next v2 18/23] selftests/bpf: Add BTF fixup for __naked subprog parameter names

On 5/9/26 3:40 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The BPF object 'tobj' is leaked when 'fixup_btf_from_path()' fails.
> --
>
> commit af529dd65e68c8ac479551b0ac6332c3b5f56771
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add BTF fixup for __naked subprog parameter names
>
> This commit adds a mechanism to fix up missing parameter names in the BTF
> FUNC_PROTO entries of __naked subprogs by reading them from a separate BTF
> file. This allows the verifier to resolve stack argument slots by name.
>
>> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
>> --- a/tools/testing/selftests/bpf/test_loader.c
>> +++ b/tools/testing/selftests/bpf/test_loader.c
> [ ... ]
>
>> @@ -1231,13 +1351,27 @@ void run_subtest(struct test_loader *tester,
>>   		}
>>   	}
>>   
>> -	/* Implicitly reset to NULL if next test case doesn't specify */
>> +	/* Implicitly reset to NULL if next test case doesn't specify.
>> +	 * btf_custom_func_path also serves as btf_custom_path for kfunc resolution.
>> +	 */
>>   	open_opts->btf_custom_path = spec->btf_custom_path;
>> +	if (!open_opts->btf_custom_path)
>> +		open_opts->btf_custom_path = spec->btf_custom_func_path;
>>   
>>   	tobj = bpf_object__open_mem(obj_bytes, obj_byte_cnt, open_opts);
>>   	if (!ASSERT_OK_PTR(tobj, "obj_open_mem")) /* shouldn't happen */
>>   		goto subtest_cleanup;
>>   
>> +	/* Fix up __naked subprog BTF using a separate file with named params */
>> +	if (spec->btf_custom_func_path) {
>> +		err = fixup_btf_from_path(tobj, spec->btf_custom_func_path);
>> +		if (err) {
>> +			PRINT_FAIL("failed to fixup BTF from %s: %d\n",
>> +				   spec->btf_custom_func_path, err);
>> +			goto subtest_cleanup;
> Does this error path leak tobj?
>
> If fixup_btf_from_path() fails, the code jumps to subtest_cleanup, which
> appears to bypass the tobj_cleanup label where tobj would normally be
> closed and freed. Should this be goto tobj_cleanup instead?

Thanks for catching up this bug. Will fix in the next revision.