> 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
> @@ -9,6 +9,7 @@
> #include <test_progs.h>
> #include "test_get_xattr.skel.h"
> #include "test_set_remove_xattr.skel.h"
> +#include "test_init_inode_xattr.skel.h"
> #include "test_fsverity.skel.h"
>
> static const char testfile[] = "/tmp/test_progs_fs_kfuncs";
> @@ -268,6 +269,51 @@ struct fsverity_enable_arg arg = {0};
> 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 the 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);
> +}
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25291381237
