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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Double free of the `extra` BPF link in the error path of `test_init_inode_xattr_attach_cap()`.
- [Medium] Test fails instead of skipping when the filesystem lacks xattr support (`EOPNOTSUPP`).
- [Low] Test uses `open` with `O_CREAT` without `O_EXCL` or unlinking first, leading to flaky failures if the file already exists.
--

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

selftests/bpf: add tests for bpf_init_inode_xattr kfunc

Test bpf atomic inode xattr labeling in inode_init_security.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> index 43a26ec69a8e3..0898898fb1253 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c

[ ... ]

> @@ -268,6 +269,102 @@ 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];
> +	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);
> +	if (!ASSERT_GE(fd, 0, "create_file"))
> +		goto out;

[Severity: Low]
If the file already exists from a previously aborted test run, will open()
without O_EXCL just open the existing file instead of creating a new one?

If so, this might bypass the inode_init_security hook the test relies on to
trigger the BPF program, causing unexpected failures. Would it be safer to
add O_EXCL or explicitly unlink the file beforehand?

> +
> +	ASSERT_EQ(skel->data->init_result, 0, "init_result");
> +
> +	/* initxattrs prepends "security." to the name. */
> +	err = getxattr(testfile_new, "security.bpf.test_label", value_out,
> +		       sizeof(value_out));
> +	if (err < 0 && errno == ENODATA) {
> +		printf("%s:SKIP:filesystem did not apply LSM xattrs\n",
> +		       __func__);
> +		test__skip();
> +		goto out;
> +	}
> +	if (!ASSERT_GE(err, 0, "getxattr"))
> +		goto out;

[Severity: Medium]
Does this skip logic need to handle errno == EOPNOTSUPP as well?

If the test runs on a filesystem that doesn't support xattrs at all (like
some tmpfs configurations), getxattr() might return EOPNOTSUPP. The test
would then fall through to the ASSERT_GE() check and fail rather than
gracefully skipping.

[ ... ]

> +static void test_init_inode_xattr_attach_cap(void)
> +{
> +	struct test_init_inode_xattr *skel[INIT_INODE_XATTR_MAX + 1] = {};
> +	struct bpf_link *link[INIT_INODE_XATTR_MAX + 1] = {};
> +	struct bpf_link *extra = NULL;
> +	int i, err;
> +
> +	/* Fill all available xattr slots */
> +	for (i = 0; i < INIT_INODE_XATTR_MAX; i++) {
> +		skel[i] = test_init_inode_xattr__open_and_load();
> +		if (!ASSERT_OK_PTR(skel[i], "open_and_load"))
> +			goto out;
> +
> +		link[i] = bpf_program__attach_lsm(skel[i]->progs.test_init_inode_xattr);
> +		if (!ASSERT_OK_PTR(link[i], "attach_within_cap"))
> +			goto out;
> +	}
> +
> +	skel[INIT_INODE_XATTR_MAX] = test_init_inode_xattr__open_and_load();
> +	if (!ASSERT_OK_PTR(skel[INIT_INODE_XATTR_MAX], "open_and_load_extra"))
> +		goto out;
> +
> +	/* New additions fail with -E2BIG */
> +	extra = bpf_program__attach_lsm(skel[INIT_INODE_XATTR_MAX]->progs.test_init_inode_xattr);
> +	err = -errno;
> +	if (!ASSERT_ERR_PTR(extra, "attach_over_cap_should_fail")) {
> +		bpf_link__destroy(extra);
> +		goto out;
> +	}
> +	ASSERT_EQ(err, -E2BIG, "attach_over_cap_errno");
> +
> +	bpf_link__destroy(link[0]);
> +	link[0] = NULL; /* avoid double free in cleanup */
> +
> +	/* Freeing a slot lets the extra program attach */
> +	extra = bpf_program__attach_lsm(skel[INIT_INODE_XATTR_MAX]->progs.test_init_inode_xattr);
> +	ASSERT_OK_PTR(extra, "attach_after_detach");
> +
> +out:
> +	bpf_link__destroy(extra);

[Severity: Medium]
Can this cause a double free of the extra link if the first assert fails?

If extra is unexpectedly valid in the attach_over_cap_should_fail check,
bpf_link__destroy(extra) is called before jumping to the out label. Once
at the out label, bpf_link__destroy(extra) will be called a second time
on the exact same pointer since it wasn't set to NULL before the jump.

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