Re: [PATCH bpf-next 5/6] selftests/bpf: Test BTF sanitization rejection for invalid vlen

On 4/14/26 11:26 PM, [email protected] wrote:
> Sashiko AI review found 3 potential issue(s):
> - [Low] Leaking the BTF object on unexpected success of sanitization.
> - [Low] Lack of error checks for BTF construction calls.
> - [Low] Using `BTF_MAX_VLEN` as the `byte_sz` argument in `btf__add_datasec()`.
> --
> 
> commit 9c5af444155b34aa840793bf185bc0c0aaa04f3f
> Author: Alan Maguire <[email protected]>
> 
> selftests/bpf: Test BTF sanitization rejection for invalid vlen
>      
> This patch adds a selftest to verify that BTF objects with extended vlen
> are properly rejected by kernels that do not support the extended feature.
> It simulates the lack of support using the feature cache and checks if
> sanitization fails when the vlen limit is surpassed.
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_sanitize.c b/tools/testing/selftests/bpf/prog_tests/btf_sanitize.c
>> index 652b51efafc27..f41cea5a769a2 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/btf_sanitize.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/btf_sanitize.c
> [ ... ]
>> @@ -95,3 +95,65 @@ void test_btf_sanitize_layout(void)
>> +static void test_btf_sanitize_extended(void)
>> +{
> [ ... ]
>> +	btf = btf__new_empty();
>> +	if (!ASSERT_OK_PTR(btf, "empty_btf"))
>> +		goto out;
>> +	btf__add_int(btf, "int", 4, BTF_INT_SIGNED);
>> +	btf__add_var(btf, "var1", 0, 1);
> 
> Are we missing error checks on these btf__add_*() calls?
> 
> If any of these allocations silently fail, the test might proceed with an
> incomplete BTF object that hasn't actually surpassed the vlen limit. This
> would cause the subsequent bpf_object__sanitize_btf() call to unexpectedly
> succeed, failing the test for the wrong reason.
> 
>> +	btf__add_datasec(btf, "datasec1", BTF_MAX_VLEN);
> 
> Is it intentional to use BTF_MAX_VLEN for the byte_sz argument here?
> 
> BTF_MAX_VLEN is a bitmask representing the maximum number of items, not a byte
> size. Using a proper calculated byte size like 0x10000 * 4 might be clearer.
> 
>> +	for (i = 0; i <= 0xffff; i++)
>> +		btf__add_datasec_var_info(btf, 2, i * 4, 4);
>> +
>> +	/* Sanitization should fail here as vlen surpasses unextended limit. */
>> +	sanitized = bpf_object__sanitize_btf(skel->obj, btf);
>> +	if (!ASSERT_ERR_PTR(sanitized, "bpf_object__sanitize_btf_should_fail")) {
>> +		sanitized = NULL;
> 
> If the first bpf_object__sanitize_btf() call unexpectedly succeeds, sanitized
> will hold a valid pointer.
> 
> Doesn't setting it to NULL before the goto out path leak the memory, since
> btf__free(sanitized) will be called with a NULL pointer at the end of the
> function?
> 

This is no leak, but setting to NULL is not needed, because btf__free() 
checks IS_ERR_OR_NULL(btf).

>> +		goto out;
>> +	}
>> +	/* Now switch extended feature on and ensure success. */
>