Re: [PATCH bpf-next 3/4] bpf: Introduce path iterator

[Al Viro <viro@zeniv.linux.org.uk>]

On Thu, May 29, 2025 at 03:13:10PM -0700, Song Liu wrote:

> Is it an issue if we only hold a reference to a MNT_LOCKED mount for
> short period of time? "Short period" means it may get interrupted, page
> faults, or wait for an IO (read xattr), but it won't hold a reference to the
> mount and sleep indefinitely.

MNT_LOCKED mount itself is not a problem.  What shouldn't be done is
looking around in the mountpoint it covers.  It depends upon the things
you are going to do with that, but it's very easy to get an infoleak
that way.

> > OTOH, there's a good cause for moving some of the flags, MNT_LOCKED
> > included, out of ->mnt_flags and into a separate field in struct mount.
> > However, that would conflict with any code using that to deal with
> > your iterator safely.
> >
> > What's more, AFAICS in case of a stack of mounts each covering the root
> > of parent mount, you stop in each of those.  The trouble is, umount(2)
> > propagation logics assumes that intermediate mounts can be pulled out of
> > such stack without causing trouble.  For pathname resolution that is
> > true; it goes through the entire stack atomically wrt that stuff.
> > For your API that's not the case; somebody who has no idea about an
> > intermediate mount being there might get caught on it while it's getting
> > pulled from the stack.
> >
> > What exactly do you need around the mountpoint crossing?
> 
> I thought about skipping intermediate mounts (that are hidden by
> other mounts). AFAICT, not skipping them will not cause any issue.

It can.  Suppose e.g. that /mnt gets propagation from another namespace,
but not the other way round and you mount something on /mnt.

Later, in that another namespace, somebody mounts something on wherever
your /mnt gets propagation to.  A copy will be propagated _between_
your /mnt and whatever you've mounted on top of it; it will be entirely
invisible until you umount your /mnt.  At that point the propagated
copy will show up there, same as if it had appeared just after your
umount.  Prior to that it's entirely invisible.  If its original
counterpart in another namespace gets unmounted first, the copy will
be quietly pulled out.

Note that choose_mountpoint_rcu() callers (including choose_mountpoint())
will have mount_lock seqcount sampled before the traversal _and_ recheck
it after having reached the bottom of stack.  IOW, if you traverse ..
on the way to root, you won't get caught on the sucker being pulled out.

Your iterator, OTOH, would stop in that intermediate mount - and get
an unpleasant surprise when it comes back to do the next step (towards
/mnt on root filesystem, that is) and finds that path->mnt points
to something that is detached from everything - no way to get from
it any further.  That - despite the fact that location you've started
from is still mounted, still has the same pathname, etc. and nothing
had been disrupted for it.

And yes, landlock has a narrow race in the matching place.  Needs to
be fixed.  At least it does ignore those as far as any decisions are
concerned...

Note, BTW, that it might be better off by doing that similar to
d_path.c - without arseloads of dget_parent/dput et.al.; not sure
how feasible it is, but if everything in it can be done under
rcu_read_lock(), that's something to look into.