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

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [Medium] Test flakiness due to missing file removal before open(O_CREAT).
- [Medium] Test fails instead of gracefully skipping on filesystems without xattr support.
- [Low] Uninitialized stack memory read if getxattr returns an unexpected length.
- [Low] Insecure temporary file creation in world-writable directory.
- [Low] Closing uninitialized file descriptor on error paths.
--

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

selftests/bpf: add tests for bpf_init_inode_xattr kfunc

This commit adds selftests for the bpf_init_inode_xattr kfunc to test
atomic inode xattr labeling via the inode_init_security 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];

Could value_out be read while uninitialized?

If getxattr() succeeds but returns fewer bytes than expected, the size
ASSERT_EQ check will fail but execution will continue. The subsequent
strncmp() might then read uninitialized stack memory.

Would it be better to zero-initialize this buffer, or use
if (!ASSERT_EQ(...)) to abort the test early?

> +	const char *testfile_new = "/tmp/test_progs_fs_kfuncs_new";
> +
> +	skel = test_init_inode_xattr__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "test_init_inode_xattr__open_and_load"))
> +		return;
> +
> +	skel->bss->monitored_pid = getpid();
> +	err = test_init_inode_xattr__attach(skel);
> +	if (!ASSERT_OK(err, "test_init_inode_xattr__attach"))
> +		goto out;
> +
> +	/* Create a new file — this triggers inode_init_security */
> +	fd = open(testfile_new, O_CREAT | O_RDWR, 0644);

If this test file already exists from a previous failed run, will open() just
open the existing file without triggering the inode_init_security hook?
Since the hook only fires when a new inode is instantiated, this would cause
the test to persistently fail.

Also, since this runs as root in the world-writable /tmp directory, could
this predictable path be vulnerable to a symlink attack?

Would adding O_EXCL to the open flags ensure both that the hook is triggered
and prevent symlink issues?

> +	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) {

If the underlying /tmp filesystem does not support extended attributes at
all, getxattr() returns -1 with errno set to EOPNOTSUPP (or ENOTSUP).

Should this condition also be handled to correctly skip the test instead of
failing on the next ASSERT_GE check?

> +		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");
> +
> +out:
> +	close(fd);

If an error occurs before the file is opened (e.g. during skeleton load
or attach), fd will be -1. Does this result in an unnecessary close(-1)
which returns -EBADF?

Should this be guarded with if (fd >= 0)?

> +	test_init_inode_xattr__destroy(skel);
> +	remove(testfile_new);
> +}

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