Re: [RFC PATCH bpf-next 11/12] bpf: add a lockless path ancestor iterator

[Justin Suess <utilityemal77@gmail.com>]

On Tue, Oct 06, 2026 at 01:10:47AM +0000, bot+bpf-ci@kernel.org wrote:
> > bpf: add a lockless path ancestor iterator
> 
> TL;DR:
> - Verifier may give a plain trusted dentry from the lockless path iterator,
>   risking use-after-free in sleepable kfuncs after RCU exit
> - A failed legitimize may look like a finished walk, hiding a lost race
> 
> > diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> > index 265cb414a08a..9dad65df4267 100644
> > --- a/fs/bpf_fs_kfuncs.c
> > +++ b/fs/bpf_fs_kfuncs.c
> 
> [ ... ]
> 
> > @@ -633,6 +656,79 @@ bpf_path_ancestors_pos_flags(struct bpf_iter_path_ancestors *it__iter)
> 
> [ ... ]
> 
> > +/*
> > + * Unlike the referenced variant, this hands out the walk's own position:
> > + * a lockless iteration holds no references to pass on, and the verifier
> > + * keeps the whole of it inside one RCU read-side critical section.
> > + */
> > +__bpf_kfunc struct path *
> > +bpf_iter_path_ancestors_rcu_next(struct bpf_iter_path_ancestors_rcu *it)
> > +{
> > +	return bpf_path_ancestors_step((void *)it);
> > +}
> 
> The commit message says that "nothing read out of the position may be passed
> to a kfunc demanding a trusted argument", and the comment above struct
> bpf_iter_path_ancestors says the same.  Does the verifier actually enforce
> this?
> 
> The position returned by bpf_iter_path_ancestors_rcu_next() is typed
> PTR_TO_BTF_ID | MEM_RCU, because check_kfunc_call() adds MEM_RCU to the
> iter_next result of a KF_RCU_PROTECTED iterator.  Commit 7ed5aa71ad77 ("bpf:
> mark struct path trusted") added:
> 
>     BTF_TYPE_SAFE_TRUSTED(struct path) {
>             struct dentry *dentry;
>     };
> 
> In check_ptr_to_btf_access(), the branch taken for
> is_trusted_reg(env, reg) || is_rcu_reg(reg) applies type_is_trusted() and
> sets PTR_TRUSTED on a safe-trusted field even when the base pointer is only
> MEM_RCU.
> 
> So loading the dentry field of a lockless position yields a plain
> PTR_TRUSTED dentry.  It does not keep MEM_RCU, so
> invalidate_rcu_protected_refs() leaves it alone at bpf_rcu_read_unlock().
> 
> The safe-trusted rule for struct path assumes the path pins its dentry.  That
> holds for the referenced variant, but the lockless walk takes no reference on
> the dentry, and an ancestor's lifetime is not tied to the original path once
> a concurrent rename moves the descendant.
> 
> After the program leaves the RCU read-side section, could a sleepable kfunc
> that takes a trusted dentry (the xattr kfuncs in this set, for example) be
> handed a dentry that has neither a reference nor RCU protection?
> 
> Would it work to keep MEM_RCU on fields loaded from a MEM_RCU struct path
> (for example by applying the struct path safe-trusted rule only to trusted
> bases), or to avoid exposing struct path as the return type of the lockless
> iterator?  A negative selftest showing that such a load is rejected would
> also help.
This feedback was repeated several times, and it's real (missed this).
I assumed that the RCU characteristic would propagate, but it does not.

Basically the issue is that the dentry is trusted even if the path is
under rcu, so it can be stored and becomes a UAF outside the rcu
critical section for RCU walks.

Two ways this can be handled:

1. Don't mark the dentry as trusted. (patch 4) 
This is the simplest fix, but limits the utility of this patchset
as you couldn't use the dentry for kfuncs expecting trusted dentry args,
including the xattr kfuncs.

2. Propagate MEM_RCU for a __safe_trusted field reached from a MEM_RCU
base. (as stated in the above suggestion)

More invasive and affects existing BTF definitions. I'd have to dig
more into this to see what this would affect.

...

Leaning towards 1. 2 can be a follow up series without breaking
compatibility (path* equivalents of the dentry* kfuncs would work too).
But if it's desired I can try 2.

> 
> [ ... ]
> 
> > + * Return: 0, -%ENOENT if the lockless iteration was not on a position, or
> > + * -%ECHILD if it lost the race to acquire one; %BPF_PATH_ANCESTORS_RETRY is
> > + * then also flagged, and the program has reached no conclusion about the
> > + * ancestry.  @it__uninit is initialized whatever this returns, so a program
> > + * need not branch on the result: a walk that could not be escalated simply
> > + * yields no position.
> > + */
> > +__bpf_kfunc int
> > +bpf_path_ancestors_legitimize(struct bpf_iter_path_ancestors *it__uninit,
> > +			      struct bpf_iter_path_ancestors_rcu *rcu_it__iter)
> > +{
> > +	struct bpf_path_ancestors_kern *rcu_kit = (void *)rcu_it__iter;
> > +	struct bpf_path_ancestors_kern *kit = (void *)it__uninit;
> > +
> > +	/* A zeroed walk makes destroying the iterator a no-op. */
> > +	memset(kit, 0, sizeof(*kit));
> > +	kit->step = 1;
> > +
> > +	/* Drained, or already failed: nothing to hand over. */
> > +	if (rcu_kit->step)
> > +		return -ENOENT;
> > +	if (!vfs_walk_handover(&kit->aw, &rcu_kit->aw)) {
> > +		rcu_kit->step = -ECHILD;
> > +		return -ECHILD;
> > +	}
> > +	kit->step = 0;
> > +	return 0;
> > +}
> 
> When the lockless iteration had already failed with -ECHILD, or when
> vfs_walk_handover() fails, @it__uninit is left with step == 1.  That is the
> same state as a referenced walk that has passed the real root, so
> bpf_iter_path_ancestors_next() returns NULL and bpf_path_ancestors_pos_flags()
> on that iterator returns 0.
> 
> The documented contract of bpf_iter_path_ancestors_next() says NULL comes
> "once the walk has passed the real root - or on an allocation failure ...
> reported as NOMEM", so a lost race reads as a completed walk.  The kernel-doc
> above also tells programs they "need not branch on the result".
> 
> Can a policy program that follows that advice and only inspects the resumed
> iterator treat a lost race as a finished ancestry walk with no match?
> BPF_PATH_ANCESTORS_RETRY is flagged only on the lockless iterator, which has
> to be destroyed before the resumed iteration can run.
> 
> Would setting kit->step = -ECHILD on the destination in both failure paths
> work?  Stepping would still stop (step != 0), the zeroed walk would keep
> destroy a no-op, and bpf_path_ancestors_pos_flags() on the resumed iterator
> would report BPF_PATH_ANCESTORS_RETRY.
> 
> Separately, when the source had already lost a race, the first check returns
> -ENOENT rather than -ECHILD, which conflicts with the kernel-doc describing
> -ENOENT as "not on a position".
>
This is also real, just forgot to add kit->step = -ECHILD on the failure
paths. ~2 lines.

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