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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Silent test skipping if the testfile exists from a previous aborted run.
- [Low] Multi-line comment does not follow the required BPF subsystem style.
--

--- Patch [4]: [PATCH v6 bpf-next 4/4] selftests/bpf: add tests for bpf_init_inode_xattr kfunc ---
commit a299ac47c44a77d98dbdf43f3ba7c981f13fd930
Author: David Windsor <[email protected]>

selftests/bpf: add tests for bpf_init_inode_xattr kfunc

This commit adds selftests for BPF atomic inode xattr labeling in
the inode_init_security hook.

> --- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c

[ ... ]

> @@ -268,6 +269,155 @@ 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[64];
> +
> +	/* This test must be run from a fs that calls
> +	 * security_inode_init_security().
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment violate the BPF subsystem
style guidelines? The guidelines prefer the opening /* on its own line:

	/*
	 * This test must be run from a fs that calls
	 * security_inode_init_security().
	 */

> +	const char *testfile_new = "/dev/shm/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]
Could this result in silently skipping the test if the file already exists
from a previous aborted run?

By omitting O_EXCL, if the test file is already present, open() will not
create a new inode. This means the inode_init_security hook won't trigger,
and the test will hit the skip condition below. Should we use
O_CREAT | O_EXCL | O_RDWR here to ensure we always create a new inode?

> +	if (!ASSERT_GE(fd, 0, "create_file"))
> +		goto out;
> +
> +	/*
> +	 * Probably should not be needed as we will be labeling a file
> +	 * in /dev/shm, but just in case we check if the hook was actually
> +	 * called.
> +	 */
> +	if (!skel->bss->hook_ran) {
> +		printf("%s:SKIP:inode_init_security hook was not invoked\n",
> +		       __func__);
> +		test__skip();
> +		goto out;
> +	}

[ ... ]

> +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 file from a previous
aborted run could cause the test to silently skip? 

Adding O_EXCL would guarantee the inode is created and the hook is
triggered.

> +	if (!ASSERT_GE(fd, 0, "create_file"))
> +		goto out;

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