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

[bot+bpf-ci at kernel.org]

> selftests/bpf: exercise the path ancestor iterator

TL;DR:
- Selftest's ref_count >= 3 may not verify the walk reaches /; early stop or
  bad mount crossing could pass
- Test may not exercise using a position after a later step, as its commit
  message and comment claim

> diff --git a/tools/testing/selftests/bpf/prog_tests/path_ancestors.c b/tools/testing/selftests/bpf/prog_tests/path_ancestors.c
> new file mode 100644
> index 000000000000..2de79673a13b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/path_ancestors.c
> @@ -0,0 +1,48 @@

[ ... ]

> +	/* suba, base, /tmp, / at least. */
> +	ASSERT_GE(skel->bss->ref_count, 3, "ref_count");
> +	ASSERT_EQ(skel->bss->ref_flags, 0, "ref_flags");

The comment lists four positions (suba, base, /tmp, /), and the commit
message says the test checks "that the iteration reaches the root: the
temporary directory, its parent and / are visited".  Does ref_count >= 3
verify that?

For /tmp/path_ancestors_XXXXXX/a the walk should yield at least four
positions, so a count of exactly 3 would mean the walk stopped before /.
Two cases that would pass this check:

- a broken mount crossing out of a tmpfs /tmp, where the iteration ends at
  the tmpfs root.

- an iteration ended early by the allocation failure path in
  bpf_iter_path_ancestors_next():

    held = kmalloc_obj(*held);
    if (!held) {
        kit->step = -ENOMEM;
        return NULL;
    }

The ref_flags check cannot catch the second case, since
bpf_path_ancestors_pos_flags() is only called from the loop for positions
that were returned, and at that point kit->step is still 0.

Should this be ASSERT_GE(skel->bss->ref_count, 4, "ref_count")?  Should the
program also record bpf_path_ancestors_pos_flags() after the loop ends and
check that the last position was the root, so that "reaches the root" is
actually verified?

The follow-up commit 70352ce0817c ("selftests/bpf: exercise the lockless
path ancestor iterator") keeps the same comment and the same ">= 3" lower
bound (on rcu_count), so this is not tightened later in the series.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/path_ancestors.c b/tools/testing/selftests/bpf/progs/path_ancestors.c
> new file mode 100644
> index 000000000000..af6b777e8bec
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/path_ancestors.c
> @@ -0,0 +1,49 @@

[ ... ]

> +	/*
> +	 * Referenced walk: every position comes acquired, so it stays valid
> +	 * for sleepable work and past the step that yielded it.
> +	 */
> +	bpf_iter_path_ancestors_new(&it, (struct path *)dir, 0);
> +	while ((pos = bpf_iter_path_ancestors_next(&it))) {
> +		ref_count++;
> +		ref_flags |= bpf_path_ancestors_pos_flags(&it);
> +		if (ref_count == 2)
> +			second_len = bpf_path_d_path(pos, second_path,
> +						     sizeof(second_path));
> +		bpf_path_put(pos);
> +	}

The commit message says the second position's pathname is resolved "from
the acquired position after the step that yielded it has been taken, so
that the acquired reference is what the sleepable kfunc runs on rather than
the walk's", and the test comment says "The acquired second position, used
after its step was taken."  Does this loop exercise that?

bpf_path_d_path() does not look sleepable.  It is registered without
KF_SLEEPABLE:

    BTF_ID_FLAGS(func, bpf_path_d_path)

Also, bpf_path_d_path(pos, ...) is called before the next
bpf_iter_path_ancestors_next() call.  At that point the walk's own view
still refers to the same position with its own reference, because
bpf_path_ancestors_step() returns &kit->aw.pos and vfs_walk_step_ref() only
drops that reference at the next step:

    dput(aw->pos.dentry);
    aw->pos.dentry = parent;

So during bpf_path_d_path() the position is held by both the acquired copy
and the walk, and the assertion on second_path cannot tell whether the
acquired reference is what keeps it alive.  Choosing ref_count == 2 instead
of the first position makes no difference here.

The commit 8dd2a68fe0e7 ("bpf: add a path ancestor iterator") justifies the
per-position allocation and KF_ACQUIRE with "a position that outlives the
step it came from ... has to be kept alive by the program's reference
rather than by the iterator's".  This test never uses a position after a
later step.

Could the program use a position after the next step, without a loop?  For
example:

    pos1 = bpf_iter_path_ancestors_next(&it);
    pos2 = bpf_iter_path_ancestors_next(&it);
    bpf_path_put(pos2);
    bpf_path_d_path(pos1, ...);
    bpf_path_put(pos1);

Otherwise, could the commit message and the test comment be reworded to
match what is tested?  The follow-up commit 70352ce0817c leaves this check
and comment unchanged.


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