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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing file cleanup before file creation causes flaky tests. If the test files already exist, `open(O_CREAT)` will not trigger the LSM hook, leading to test failure.
--

--- Patch [3]: [PATCH v5 bpf-next 3/3] selftests/bpf: add tests for bpf_init_inode_xattr kfunc ---
commit fb13c41f9d223f18ed7c34e0e581869c6933b5e5
Author: David Windsor <[email protected]>

selftests/bpf: add tests for bpf_init_inode_xattr kfunc

This commit adds BPF selftests to verify atomic inode xattr labeling in the
inode_init_security hook. It introduces tests that attach to the hook and
validate that bpf_init_inode_xattr properly sets xattrs and respects the
per-inode slot limit.

> 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
[ ... ]
> @@ -268,6 +269,123 @@ static void test_fsverity(void)
> +static void test_init_inode_xattr(void)
> +{
> +	struct test_init_inode_xattr *skel = NULL;
> +	int fd = -1, err;
> +	char value_out[64];
> +	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;
> +
> +	/* Trigger inode_init_security */
> +	fd = open(testfile_new, O_CREAT | O_RDWR, 0644);

[Severity: Medium]
Will this test become flaky if the file already exists from a previously
aborted test run?

If the testfile remains on disk from an earlier crash or failure,
open() with O_CREAT will simply open the existing file rather than create a
new inode. This would bypass the inode_init_security LSM hook entirely,
meaning the BPF program won't execute and init_result will remain at its
default value, causing the test to fail.

Would it be safer to call remove(testfile_new) before this open(), or use a
unique temporary filename to ensure the hook is always triggered?

> +	if (!ASSERT_GE(fd, 0, "create_file"))
> +		goto out;
> +
> +	ASSERT_EQ(skel->data->init_result, 0, "init_result");
[ ... ]
> +static void test_init_inode_xattr_slot_limit(void)
> +{
[ ... ]
> +	/* Trigger inode_init_security once with all programs attached. */
> +	fd = open(testfile_slot, O_CREAT | O_RDWR, 0644);

[Severity: Medium]
Does this face the same issue where a pre-existing testfile_slot will
prevent the LSM hook from firing?

If remove(testfile_slot) is called prior to open(), it would guarantee a
new inode is created each time.

> +	if (!ASSERT_GE(fd, 0, "create_file"))
> +		goto out;
> +
> +	/*
> +	 * Exactly INIT_INODE_XATTR_MAX programs claim a slot; the program past
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3