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

[sashiko-bot@kernel.org]

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 <alan.maguire@oracle.com>

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260414195019.684531-1-alan.maguire@oracle.com?part=5