[RFC PATCH bpf-next 12/12] selftests/bpf: exercise the lockless path ancestor iterator

[bot+bpf-ci at kernel.org]

> selftests/bpf: exercise the lockless path ancestor iterator

TL;DR: Test issue in path_ancestors selftest: if setxattr() gives EOPNOTSUPP,
the xattr checks are silently dropped and the run reports a full pass. Also a
redundant test_err variable and a stale comment (nits).

> diff --git a/tools/testing/selftests/bpf/prog_tests/path_ancestors.c b/tools/testing/selftests/bpf/prog_tests/path_ancestors.c
> index 2de79673a13b..ce1ded844c3a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/path_ancestors.c
> +++ b/tools/testing/selftests/bpf/prog_tests/path_ancestors.c

[ ... ]

> @@ -20,6 +23,12 @@ void test_path_ancestors(void)
>  	if (!ASSERT_OK(mkdir(suba, 0755), "mkdir_a"))
>  		goto out_rm;
>  
> +	/* Read back by the program at the escalated position (== base). */
> +	err = setxattr(base, "user.walk", "hello", 6, 0);
> +	xattr_works = !err;
> +	if (err && errno != EOPNOTSUPP && !ASSERT_OK(err, "setxattr"))
> +		goto out_rm;
> +

[ ... ]

> @@ -31,14 +40,34 @@ void test_path_ancestors(void)

[ ... ]

> +	/* The escalated position is the walk's second one: base. */
> +	ASSERT_STREQ(skel->bss->escalated_path, base, "escalated_path");
> +	ASSERT_EQ(skel->bss->escalated_len, strlen(base) + 1, "escalated_len");
> +	if (xattr_works) {
> +		ASSERT_EQ(skel->bss->xattr_ret, 6, "xattr_len");
> +		ASSERT_STREQ(skel->bss->xattr_value, "hello", "xattr_value");
> +	}
> +

When setxattr() fails with EOPNOTSUPP (for example /tmp on a tmpfs built
without CONFIG_TMPFS_XATTR), xattr_works is false and the xattr_ret and
xattr_value checks are dropped without any message.  test__skip() is not
called either, so the run reports a full pass.

A subsystem pattern flags this as potentially concerning: a test that
skips a capability-dependent check should say why, rather than skipping
silently.

The rest of the test (walk counts, d_path on the escalated position)
still runs, and the program's bpf_get_dentry_xattr() call just returns an
error that nothing looks at, so nothing fails spuriously.  But the xattr
read is one of the two sleepable operations the commit message says the
escalation exists for, and its coverage disappears from the report.

prog_tests/fs_kfuncs.c handles the same errno from setxattr() on /tmp
explicitly:

	if (err && errno == EOPNOTSUPP) {
		printf("%s:SKIP:local fs doesn't support xattr (%d)\n"
		       "To run this test, make sure /tmp filesystem supports xattr.\n",
		       __func__, errno);
		test__skip();
		...
	}

Could this test print a similar note, or run the xattr check as a subtest
that reports itself skipped, when xattr_works is false?

> diff --git a/tools/testing/selftests/bpf/progs/path_ancestors.c b/tools/testing/selftests/bpf/progs/path_ancestors.c
> index af6b777e8bec..50ce0ce163dd 100644
> --- a/tools/testing/selftests/bpf/progs/path_ancestors.c
> +++ b/tools/testing/selftests/bpf/progs/path_ancestors.c
> @@ -11,29 +11,74 @@ char _license[] SEC("license") = "GPL";

[ ... ]

> +int retry_flags;	/* BPF_PATH_ANCESTORS_RETRY observations */

[ ... ]

> +int escalate_err;	/* bpf_path_ancestors_legitimize() result */
> +int test_err;

[ ... ]

> @@ -45,5 +90,45 @@ int BPF_PROG(walk_modes, const struct path *dir, struct dentry *dentry,

[ ... ]

> +	escalate_err = bpf_path_ancestors_legitimize(&it, &rit);
> +	retry_flags |= bpf_path_ancestors_rcu_pos_flags(&rit);
> +	bpf_iter_path_ancestors_rcu_destroy(&rit);
> +	bpf_rcu_read_unlock();
> +
> +	if (escalate_err)
> +		test_err = 1;

this isn't a bug, but is test_err needed alongside escalate_err?  It is
set in exactly one place, from escalate_err being nonzero:

	if (escalate_err)
		test_err = 1;

and the userspace half asserts both of them are 0 one after the other:

	ASSERT_EQ(skel->bss->test_err, 0, "test_err");
	ASSERT_EQ(skel->bss->escalate_err, 0, "escalate_err");

Could the program keep just escalate_err, which already records the actual
error code, and have the test assert only that?

test_err is also the only new global without a describing comment.

Separately, the retry_flags comment only mentions BPF_PATH_ANCESTORS_RETRY
observations, but at the hybrid escalation point it ORs in the whole
bpf_path_ancestors_rcu_pos_flags() result, not just the RETRY bit.


---
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/37395354107