Re: [PATCH v2 2/2] selftests/bpf: add tests for bpf_init_inode_xattr kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test will fail spuriously if the target test file already exists.
- [Medium] Uninitialized stack memory is read if `getxattr` returns a smaller length than expected.
- [Low] The test fails instead of gracefully skipping on filesystems that do not support xattrs.
--

commit 96e5b274b42a271a01409d6e21dc11302496f1d3
Author: David Windsor <dwindsor@gmail.com>

selftests/bpf: add tests for bpf_init_inode_xattr kfunc

This commit tests BPF atomic inode xattr labeling via the
inode_init_security LSM hook.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> index 43a26ec69a8e..26daef116ee2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
[ ... ]
> @@ -268,6 +269,51 @@ static void test_fsverity(void)
>  	remove(testfile);
>  }
>  
> +static void test_init_inode_xattr(void)
> +{
> +	struct test_init_inode_xattr *skel = NULL;
> +	int fd = -1, err;
> +	char value_out[32];
> +	const char *testfile_new = "/tmp/test_progs_fs_kfuncs_new";
[ ... ]
> +	/* Create a new file — this triggers inode_init_security */
> +	fd = open(testfile_new, O_CREAT | O_RDWR, 0644);

Could this cause spuriously failing tests if the file already exists from a
previous, aborted run?

If the file exists, no new inode is created, so the inode_init_security
hook won't be triggered, leaving init_result at -1.

Should we unlink the file before opening it, or use the O_EXCL flag to ensure
we're creating a new inode?

> +	if (!ASSERT_GE(fd, 0, "create_file"))
> +		goto out;
> +
> +	ASSERT_EQ(skel->data->init_result, 0, "init_result");
> +
> +	/* The initxattrs callback prepends "security." to the name */
> +	err = getxattr(testfile_new, "security.bpf.test_label", value_out,
> +		       sizeof(value_out));
> +	if (err < 0 && errno == ENODATA) {

Will this fail the test instead of gracefully skipping it on filesystems
that don't support extended attributes?

If the filesystem lacks xattr support (like some tmpfs configs), getxattr
will return -EOPNOTSUPP.

Could we broaden the check to include EOPNOTSUPP?

> +		printf("%s:SKIP:filesystem did not apply LSM xattrs\n",
> +		       __func__);
> +		test__skip();
> +		goto out;
> +	}
> +	if (!ASSERT_GE(err, 0, "getxattr"))
> +		goto out;
> +
> +	ASSERT_EQ(err, (int)sizeof(skel->data->xattr_value), "xattr_size");
> +	ASSERT_EQ(strncmp(value_out, "test_value",
> +			  sizeof("test_value")), 0, "xattr_value");

Could this lead to an uninitialized stack memory read?

If getxattr returns a smaller size than expected, the size assertion will
fail, but execution will continue to strncmp since there is no goto out.
Because value_out is uninitialized, strncmp may read garbage data and
trigger KMSAN errors.

Would it be better to wrap the size assertion with if (!ASSERT_EQ(...))
goto out, or zero-initialize value_out?

> +
> +out:
> +	close(fd);
> +	test_init_inode_xattr__destroy(skel);
> +	remove(testfile_new);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260503211835.16103-1-dwindsor@gmail.com?part=2