Re: [PATCH bpf-next 2/2] selftests/bpf: add new negative tests for xattr related BPF kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The BPF program will be unconditionally rejected by the verifier because a mutable `.bss` variable is passed to a kfunc argument requiring a constant string, causing the test to fail and leak a temporary file.
- [Low] Mismatch between BPF program variable names and the generated skeleton fields accessed in the test, which will cause a build failure.
--

commit fced0fe9408f12be9c0811a441c1071b5b9e4b39
Author: Matt Bobrowski <mattbobrowski@google.com>

selftests/bpf: add new negative tests for xattr related BPF kfuncs

Add a set of negative tests to verify the newly enforced constraints
applied to xattr related BPF kfuncs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> --- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> @@ -128,6 +128,11 @@ static void test_set_remove_xattr(void)
>  	if (!ASSERT_OK_PTR(skel, "test_set_remove_xattr__open_and_load"))
>  		return;
>  
> +	/* Prepare the long name for negative test */
> +	memset(skel->bss->long_name, 'a', 256);
> +	memcpy(skel->bss->long_name, "security.bpf.", 13);
> +	skel->bss->long_name[256] = '\0';
> +

Can this leak the temporary testfile on the filesystem? If open_and_load
fails due to verifier issues with long_name, this returns directly instead
of taking the goto out path to reach remove(testfile) called later in the
function.

[ ... ]

> @@ -188,6 +193,14 @@ static void test_set_remove_xattr(void)
>  	ASSERT_TRUE(skel->bss->locked_remove_security_selinux_fail,
>  		    "locked_remove_security_selinux_fail");
>  
> +	ASSERT_EQ(skel->bss->ret_name_empty, -ERANGE, "ret_code_name_empty");
> +	ASSERT_EQ(skel->bss->ret_name_too_long, -ERANGE,
> +		  "ret_code_name_too_long");
> +	ASSERT_EQ(skel->bss->ret_value_too_large, -E2BIG,
> +		  "ret_code_value_too_large");
> +	ASSERT_EQ(skel->bss->ret_invalid_flags, -EINVAL,
> +		  "ret_code_invalid_flags");
> +

Will this cause a build failure? The BPF program declares these variables
with a ret_code_ prefix (such as ret_code_name_empty), so the generated
skeleton structure will not contain fields like ret_name_empty.

> diff --git a/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c b/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
> --- a/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
> +++ b/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
> @@ -17,6 +17,14 @@ static const char xattr_selinux[] = "security.selinux";
>  char value_bar[] = "world";
>  char read_value[32];
>  
> +const char xattr_negative[] = "security.bpf.negative";
> +int ret_code_name_empty;
> +int ret_code_name_too_long;
> +int ret_code_value_too_large;
> +int ret_code_invalid_flags;
> +char long_name[257];

Will the verifier reject this program? The kfunc bpf_get_dentry_xattr
expects a constant string for its name__str argument. Because long_name
is declared without the const qualifier, it is placed in the writable
.bss map rather than the read-only .rodata map.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260503200819.1530328-1-mattbobrowski@google.com?part=2