> diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h
> b/tools/testing/selftests/bpf/bpf_kfuncs.h
> index 7dad01439391..fd2da5487506 100644
> --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
[ ... ]
> 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
[ ... ]
> +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";
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.
Referenced in v1/v2 review comments at
https://lore.kernel.org/bpf/[email protected]/
> +
> + 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?
Referenced in v1/v2 review comments at
https://lore.kernel.org/bpf/[email protected]/
> + 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?
Referenced in v1/v2 review comments at
https://lore.kernel.org/bpf/[email protected]/
> + 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)?
Referenced in v1 review comments at
https://lore.kernel.org/bpf/[email protected]/
> + 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/25639388555
