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

[Song Liu]

On Sat, May 31, 2025 at 7:05 AM Tingmao Wang <m at maowtm.org> wrote:
>
> On 5/30/25 19:55, Song Liu wrote:
> > On Fri, May 30, 2025 at 5:20 AM Mickaël Salaün <mic at digikod.net> wrote:
> > [...]
> >>>
> >>> If we update path_parent in this patchset with choose_mountpoint(),
> >>> and use it in Landlock, we will close this race condition, right?
> >>
> >> choose_mountpoint() is currently private, but if we add a new filesystem
> >> helper, I think the right approach would be to expose follow_dotdot(),
> >> updating its arguments with public types.  This way the intermediates
> >> mount points will not be exposed, RCU optimization will be leveraged,
> >> and usage of this new helper will be simplified.
> >
> > I think it is easier to add a helper similar to follow_dotdot(), but not with
> > nameidata. follow_dotdot() touches so many things in nameidata, so it
> > is better to keep it as-is. I am having the following:
> >
> > /**
> >  * path_parent - Find the parent of path
> >  * @path: input and output path.
> >  * @root: root of the path walk, do not go beyond this root. If @root is
> >  *        zero'ed, walk all the way to real root.
> >  *
> >  * Given a path, find the parent path. Replace @path with the parent path.
> >  * If we were already at the real root or a disconnected root, @path is
> >  * not changed.
> >  *
> >  * Returns:
> >  *  true  - if @path is updated to its parent.
> >  *  false - if @path is already the root (real root or @root).
> >  */
> > bool path_parent(struct path *path, const struct path *root)
> > {
> >         struct dentry *parent;
> >
> >         if (path_equal(path, root))
> >                 return false;
> >
> >         if (unlikely(path->dentry == path->mnt->mnt_root)) {
> >                 struct path p;
> >
> >                 if (!choose_mountpoint(real_mount(path->mnt), root, &p))
> >                         return false;
> >                 path_put(path);
> >                 *path = p;
> >                 return true;
> >         }
> >
> >         if (unlikely(IS_ROOT(path->dentry)))
> >                 return false;
> >
> >         parent = dget_parent(path->dentry);
> >         if (unlikely(!path_connected(path->mnt, parent))) {
> >                 dput(parent);
> >                 return false;
> >         }
> >         dput(path->dentry);
> >         path->dentry = parent;
> >         return true;
> > }
> > EXPORT_SYMBOL_GPL(path_parent);
> >
> > And for Landlock, it is simply:
> >
> >                 if (path_parent(&walker_path, &root))
> >                         continue;
> >
> >                 if (unlikely(IS_ROOT(walker_path.dentry))) {
> >                         /*
> >                          * Stops at disconnected or real root directories.
> >                          * Only allows access to internal filesystems
> >                          * (e.g. nsfs, which is reachable through
> >                          * /proc/<pid>/ns/<namespace>).
> >                          */
> >                         if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
> >                                 allowed_parent1 = true;
> >                                 allowed_parent2 = true;
> >                         }
> >                         break;
>
>
> Hi, maybe I'm missing the complete picture of this code, but since
> path_parent doesn't change walker_path if it returns false (e.g. if it's
> disconnected, or choose_mountpoint fails), I think this `break;` should be
> outside the
>
>     if (unlikely(IS_ROOT(walker_path.dentry)))
>
> right? (Assuming this whole thing is under a `while (true)`) Otherwise we
> might get stuck at the current path and get infinite loop?

Right, we need "break" outside the if condition.

Thanks,
Song