Re: [PATCH bpf-next 2/3] selftests/bpf: Add bpffs xattr API coverage test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The test `test_umount_lifetime` attempts to verify that root directory xattrs are cleared upon umount, but trivially succeeds because the xattr was never actually set on the root directory.
--

commit eee62f5b4a9e13a37724984ba00f475e46c7075f
Author: Daniel Borkmann <daniel@iogearbox.net>

selftests/bpf: Add bpffs xattr API coverage test

This commit adds basic bpffs xattr API coverage tests, including validation
of supported namespaces, flag semantics, inode types, and mount-scoped
lifetimes.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpffs_xattr.c b/tools/testing/selftests/bpf/prog_tests/bpffs_xattr.c
> new file mode 100644
> index 0000000000000..3d02e30da338e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpffs_xattr.c

[ ... ]

> +static void test_umount_lifetime(void)
> +{
> +	char dir[64], path[128], buf[16];
> +	int map_fd = -1, err;
> +
> +	if (!ASSERT_OK(mount_bpffs(dir), "mount"))
> +		return;
> +	snprintf(path, sizeof(path), "%s/m", dir);
> +
> +	map_fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, "x", 4, 4, 1, NULL);
> +	if (!ASSERT_GE(map_fd, 0, "map_create"))
> +		goto out;
> +	if (!ASSERT_OK(bpf_obj_pin(map_fd, path), "pin"))
> +		goto out;
> +	ASSERT_OK(setxattr(path, "user.k", val_a, sizeof(val_a), XATTR_CREATE),
> +		  "set");
> +
> +	close(map_fd);
> +	map_fd = -1;
> +
> +	if (!ASSERT_OK(umount(dir), "umount"))
> +		goto out;
> +	if (!ASSERT_OK(mount("bpf", dir, "bpf", 0, NULL), "mount_fresh"))
> +		goto out;
> +
> +	err = access(path, F_OK);
> +	ASSERT_EQ(err, -1, "path_gone_ret");
> +	ASSERT_EQ(errno, ENOENT, "path_gone_errno");
> +
> +	err = getxattr(dir, "user.k", buf, sizeof(buf));

[Severity: Medium]
Does this test actually verify the root directory's xattr lifetime?

It looks like the user.k xattr is set on the pinned map file (path) earlier
in the function, but after remounting, this check looks for the xattr on the
mount root (dir).

Because user.k was never set on dir initially, won't this check trivially
succeed with ENODATA?

> +	ASSERT_EQ(err, -1, "root_xattr_gone_ret");
> +	ASSERT_EQ(errno, ENODATA, "root_xattr_gone_errno");
> +out:
> +	if (map_fd >= 0)
> +		close(map_fd);
> +	umount(dir);
> +	rmdir(dir);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522135626.120732-1-daniel@iogearbox.net?part=2