This helper walks an input path to its parent. Logic are added to handle
walking across mount tree.

This will be used by landlock, and BPF LSM.

Signed-off-by: Song Liu <song@kernel.org>
---
 fs/namei.c            | 51 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/namei.h |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 4bb889fc980b..f02183e9c073 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1424,6 +1424,57 @@ static bool choose_mountpoint(struct mount *m, const struct path *root,
 	return found;
 }
 
+/**
+ * path_walk_parent - Walk to 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.
+ *
+ * The logic of path_walk_parent() is similar to follow_dotdot(), except
+ * that path_walk_parent() will continue walking for !path_connected case.
+ * This effectively means we are walking from disconnected bind mount to
+ * the original mount. If this behavior is not desired, the caller can add
+ * a check like:
+ *
+ *   if (path_walk_parent(&path) && !path_connected(path.mnt, path.dentry)
+ *           // continue walking
+ *   else
+ *           // stop walking
+ *
+ * Returns:
+ *  true  - if @path is updated to its parent.
+ *  false - if @path is already the root (real root or @root).
+ */
+bool path_walk_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;
+	}
+
+	if (unlikely(IS_ROOT(path->dentry)))
+		return false;
+
+	parent = dget_parent(path->dentry);
+	dput(path->dentry);
+	path->dentry = parent;
+	return true;
+}
+EXPORT_SYMBOL_GPL(path_walk_parent);
+
 /*
  * Perform an automount
  * - return -EISDIR to tell follow_managed() to stop and return the path we
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 5d085428e471..cba5373ecf86 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -85,6 +85,8 @@ extern int follow_down_one(struct path *);
 extern int follow_down(struct path *path, unsigned int flags);
 extern int follow_up(struct path *);
 
+bool path_walk_parent(struct path *path, const struct path *root);
+
 extern struct dentry *lock_rename(struct dentry *, struct dentry *);
 extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
 extern void unlock_rename(struct dentry *, struct dentry *);
-- 
2.47.1

On Sat, 07 Jun 2025, Song Liu wrote:
> This helper walks an input path to its parent. Logic are added to handle
> walking across mount tree.
> 
> This will be used by landlock, and BPF LSM.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  fs/namei.c            | 51 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/namei.h |  2 ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 4bb889fc980b..f02183e9c073 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1424,6 +1424,57 @@ static bool choose_mountpoint(struct mount *m, const struct path *root,
>  	return found;
>  }
>  
> +/**
> + * path_walk_parent - Walk to 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.
> + *
> + * The logic of path_walk_parent() is similar to follow_dotdot(), except
> + * that path_walk_parent() will continue walking for !path_connected case.
> + * This effectively means we are walking from disconnected bind mount to
> + * the original mount. If this behavior is not desired, the caller can add
> + * a check like:
> + *
> + *   if (path_walk_parent(&path) && !path_connected(path.mnt, path.dentry)
> + *           // continue walking
> + *   else
> + *           // stop walking
> + *
> + * Returns:
> + *  true  - if @path is updated to its parent.
> + *  false - if @path is already the root (real root or @root).
> + */
> +bool path_walk_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;
> +	}
> +
> +	if (unlikely(IS_ROOT(path->dentry)))
> +		return false;
> +
> +	parent = dget_parent(path->dentry);
> +	dput(path->dentry);
> +	path->dentry = parent;
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(path_walk_parent);

The above looks a lot like follow_dotdot().  This is good because it
means that it is likely correct.  But it is bad because it means there
are two copies of essentially the same code - making maintenance harder.

I think it would be good to split the part that you want out of
follow_dotdot() and use that.  Something like the following.

You might need a small wrapper in landlock which would, for example,
pass LOOKUP_BENEATH and replace path->dentry with the parent on success.

NeilBrown

diff --git a/fs/namei.c b/fs/namei.c
index 4bb889fc980b..b81d07b4417b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2048,36 +2048,65 @@ static struct dentry *follow_dotdot_rcu(struct nameidata *nd)
 	return nd->path.dentry;
 }
 
-static struct dentry *follow_dotdot(struct nameidata *nd)
+/**
+ * path_walk_parent - Find the parent of the given struct path
+ * @path  - The struct path to start from
+ * @root  - A struct path which serves as a boundary not to be crosses
+ * @flags - Some LOOKUP_ flags
+ *
+ * Find and return the dentry for the parent of the given path (mount/dentry).
+ * If the given path is the root of a mounted tree, it is first updated to
+ * the mount point on which that tree is mounted.
+ *
+ * If %LOOKUP_NO_XDEV is given, then *after* the path is updated to a new mount,
+ * the error EXDEV is returned.
+ * If no parent can be found, either because the tree is not mounted or because
+ * the @path matches the @root, then @path->dentry is returned unless @flags
+ * contains %LOOKUP_BENEATH, in which case -EXDEV is returned.
+ *
+ * Returns: either an ERR_PTR() or the chosen parent which will have had the
+ * refcount incremented.
+ */
+struct dentry *path_walk_parent(struct path *path, struct path *root, int flags)
 {
 	struct dentry *parent;
 
-	if (path_equal(&nd->path, &nd->root))
+	if (path_equal(path, root))
 		goto in_root;
-	if (unlikely(nd->path.dentry == nd->path.mnt->mnt_root)) {
-		struct path path;
+	if (unlikely(path->dentry == path->mnt->mnt_root)) {
+		struct path new_path;
 
-		if (!choose_mountpoint(real_mount(nd->path.mnt),
-				       &nd->root, &path))
+		if (!choose_mountpoint(real_mount(path->mnt),
+				       root, &new_path))
 			goto in_root;
-		path_put(&nd->path);
-		nd->path = path;
-		nd->inode = path.dentry->d_inode;
-		if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+		path_put(path);
+		*path = new_path;
+		if (unlikely(flags & LOOKUP_NO_XDEV))
 			return ERR_PTR(-EXDEV);
 	}
 	/* rare case of legitimate dget_parent()... */
-	parent = dget_parent(nd->path.dentry);
+	parent = dget_parent(path->dentry);
+	return parent;
+
+in_root:
+	if (unlikely(flags & LOOKUP_BENEATH))
+		return ERR_PTR(-EXDEV);
+	return dget(path->dentry);
+}
+EXPORT_SYMBOL(path_walk_parent);
+
+static struct dentry *follow_dotdot(struct nameidata *nd)
+{
+	struct dentry *parent = path_walk_parent(&nd->path, &nd->root, nd->flags);
+
+	if (IS_ERR(parent))
+		return parent;
 	if (unlikely(!path_connected(nd->path.mnt, parent))) {
 		dput(parent);
 		return ERR_PTR(-ENOENT);
 	}
+	nd->inode = nd->path.dentry->d_inode;
 	return parent;
-
-in_root:
-	if (unlikely(nd->flags & LOOKUP_BENEATH))
-		return ERR_PTR(-EXDEV);
-	return dget(nd->path.dentry);
 }
 
 static const char *handle_dots(struct nameidata *nd, int type)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 5d085428e471..4cc15a58d900 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -80,6 +80,7 @@ struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
 struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
 					    struct qstr *name,
 					    struct dentry *base);
+struct dentry *path_walk_parent(struct path *path, struct path *root, int flags);
 
 extern int follow_down_one(struct path *);
 extern int follow_down(struct path *path, unsigned int flags);

Hi Neil,

Thanks for your suggestion! It does look like a good solution.

On Tue, Jun 10, 2025 at 4:34 PM NeilBrown <neil@brown.name> wrote:

> The above looks a lot like follow_dotdot().  This is good because it
> means that it is likely correct.  But it is bad because it means there
> are two copies of essentially the same code - making maintenance harder.
>
> I think it would be good to split the part that you want out of
> follow_dotdot() and use that.  Something like the following.
>
> You might need a small wrapper in landlock which would, for example,
> pass LOOKUP_BENEATH and replace path->dentry with the parent on success.
>
> NeilBrown
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 4bb889fc980b..b81d07b4417b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2048,36 +2048,65 @@ static struct dentry *follow_dotdot_rcu(struct nameidata *nd)
>         return nd->path.dentry;
>  }
>
> -static struct dentry *follow_dotdot(struct nameidata *nd)
> +/**
> + * path_walk_parent - Find the parent of the given struct path
> + * @path  - The struct path to start from
> + * @root  - A struct path which serves as a boundary not to be crosses
> + * @flags - Some LOOKUP_ flags
> + *
> + * Find and return the dentry for the parent of the given path (mount/dentry).
> + * If the given path is the root of a mounted tree, it is first updated to
> + * the mount point on which that tree is mounted.
> + *
> + * If %LOOKUP_NO_XDEV is given, then *after* the path is updated to a new mount,
> + * the error EXDEV is returned.
> + * If no parent can be found, either because the tree is not mounted or because
> + * the @path matches the @root, then @path->dentry is returned unless @flags
> + * contains %LOOKUP_BENEATH, in which case -EXDEV is returned.
> + *
> + * Returns: either an ERR_PTR() or the chosen parent which will have had the
> + * refcount incremented.
> + */
> +struct dentry *path_walk_parent(struct path *path, struct path *root, int flags)

We can probably call this __path_walk_parent() and make it static.

Then we can add an exported path_walk_parent() that calls
__path_walk_parent() and adds extra logic.

If this looks good to folks, I can draft v4 based on this idea.

Thanks,
Song

[...]

On Tue, Jun 10, 2025 at 05:56:01PM -0700, Song Liu wrote:
> Hi Neil,
> 
> Thanks for your suggestion! It does look like a good solution.
> 
> On Tue, Jun 10, 2025 at 4:34 PM NeilBrown <neil@brown.name> wrote:
> 
> > The above looks a lot like follow_dotdot().  This is good because it
> > means that it is likely correct.  But it is bad because it means there
> > are two copies of essentially the same code - making maintenance harder.
> >
> > I think it would be good to split the part that you want out of
> > follow_dotdot() and use that.  Something like the following.
> >
> > You might need a small wrapper in landlock which would, for example,
> > pass LOOKUP_BENEATH and replace path->dentry with the parent on success.
> >
> > NeilBrown
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 4bb889fc980b..b81d07b4417b 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2048,36 +2048,65 @@ static struct dentry *follow_dotdot_rcu(struct nameidata *nd)
> >         return nd->path.dentry;
> >  }
> >
> > -static struct dentry *follow_dotdot(struct nameidata *nd)
> > +/**
> > + * path_walk_parent - Find the parent of the given struct path
> > + * @path  - The struct path to start from
> > + * @root  - A struct path which serves as a boundary not to be crosses
> > + * @flags - Some LOOKUP_ flags
> > + *
> > + * Find and return the dentry for the parent of the given path (mount/dentry).
> > + * If the given path is the root of a mounted tree, it is first updated to
> > + * the mount point on which that tree is mounted.
> > + *
> > + * If %LOOKUP_NO_XDEV is given, then *after* the path is updated to a new mount,
> > + * the error EXDEV is returned.
> > + * If no parent can be found, either because the tree is not mounted or because
> > + * the @path matches the @root, then @path->dentry is returned unless @flags
> > + * contains %LOOKUP_BENEATH, in which case -EXDEV is returned.
> > + *
> > + * Returns: either an ERR_PTR() or the chosen parent which will have had the
> > + * refcount incremented.
> > + */
> > +struct dentry *path_walk_parent(struct path *path, struct path *root, int flags)
> 
> We can probably call this __path_walk_parent() and make it static.
> 
> Then we can add an exported path_walk_parent() that calls
> __path_walk_parent() and adds extra logic.
> 
> If this looks good to folks, I can draft v4 based on this idea.

This looks good but it would be better if we could also do a full path
walk within RCU when possible.

> 
> Thanks,
> Song
> 
> [...]
> 

On Wed, Jun 11, 2025 at 8:42 AM Mickaël Salaün <mic@digikod.net> wrote:
[...]
> > We can probably call this __path_walk_parent() and make it static.
> >
> > Then we can add an exported path_walk_parent() that calls
> > __path_walk_parent() and adds extra logic.
> >
> > If this looks good to folks, I can draft v4 based on this idea.
>
> This looks good but it would be better if we could also do a full path
> walk within RCU when possible.

I think we will need some callback mechanism for this. Something like:

for_each_parents(starting_path, root, callback_fn, cb_data, bool try_rcu) {
   if (!try_rcu)
      goto ref_walk;

   __read_seqcount_begin();
    /* rcu walk parents, from starting_path until root */
   walk_rcu(starting_path, root, path) {
    callback_fn(path, cb_data);
  }
  if (!read_seqcount_retry())
    return xxx;  /* successful rcu walk */

ref_walk:
  /* ref walk parents, from starting_path until root */
   walk(starting_path, root, path) {
    callback_fn(path, cb_data);
  }
  return xxx;
}

Personally, I don't like this version very much, because the callback
mechanism is not very flexible, and it is tricky to use it in BPF LSM.

Thanks,
Song

On 6/11/25 17:31, Song Liu wrote:
> On Wed, Jun 11, 2025 at 8:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> [...]
>>> We can probably call this __path_walk_parent() and make it static.
>>>
>>> Then we can add an exported path_walk_parent() that calls
>>> __path_walk_parent() and adds extra logic.
>>>
>>> If this looks good to folks, I can draft v4 based on this idea.
>>
>> This looks good but it would be better if we could also do a full path
>> walk within RCU when possible.
> 
> I think we will need some callback mechanism for this. Something like:
> 
> for_each_parents(starting_path, root, callback_fn, cb_data, bool try_rcu) {
>    if (!try_rcu)
>       goto ref_walk;
> 
>    __read_seqcount_begin();
>     /* rcu walk parents, from starting_path until root */
>    walk_rcu(starting_path, root, path) {
>     callback_fn(path, cb_data);
>   }
>   if (!read_seqcount_retry())
>     return xxx;  /* successful rcu walk */
> 
> ref_walk:
>   /* ref walk parents, from starting_path until root */
>    walk(starting_path, root, path) {
>     callback_fn(path, cb_data);
>   }
>   return xxx;
> }
> 
> Personally, I don't like this version very much, because the callback
> mechanism is not very flexible, and it is tricky to use it in BPF LSM.

Aside from the "exposing mount seqcounts" problem, what do you think about
the parent_iterator approach I suggested earlier?  I feel that it is
better than such a callback - more flexible, and also fits in right with
the BPF API you already designed (i.e. with a callback you might then have
to allow BPF to pass a callback?).  There are some specifics that I can
improve - Mickaël suggested some in our discussion:

- Letting the caller take rcu_read_lock outside rather than doing it in
path_walk_parent_start

- Instead of always requiring a struct parent_iterator, allow passing in
NULL for the iterator to path_walk_parent to do a reference walk without
needing to call path_walk_parent_start - this way might be simpler and
path_walk_parent_start/end can just be for rcu case.

but what do you think about the overall shape of it?

And while it is technically doing two separate things (rcu walk and
reference walk), so is this callback to some extent.  The pro of callback
however is that the retry on ref walk failure is automatic, but the user
still has to be aware and detect such cases.  For example, landlock needs
to re-initialize the layer masks previously collected if parent walk is
restarting.

(and of course, this also hides the seqcounts from non VFS code, but I'm
wondering if there are other ways to make the seqcounts in the
parent_iterator struct private, if that is the only issue with it?)

Also, if the common logic with follow_dotdot is extracted out to
__path_walk_parent, path_walk_parent might just defer to that for the
non-rcu case, and so the complexity of that function is further reduced.

> 
> Thanks,
> Song

On Wed, Jun 11, 2025 at 10:50 AM Tingmao Wang <m@maowtm.org> wrote:
[...]
> > I think we will need some callback mechanism for this. Something like:
> >
> > for_each_parents(starting_path, root, callback_fn, cb_data, bool try_rcu) {
> >    if (!try_rcu)
> >       goto ref_walk;
> >
> >    __read_seqcount_begin();
> >     /* rcu walk parents, from starting_path until root */
> >    walk_rcu(starting_path, root, path) {
> >     callback_fn(path, cb_data);
> >   }
> >   if (!read_seqcount_retry())
> >     return xxx;  /* successful rcu walk */
> >
> > ref_walk:
> >   /* ref walk parents, from starting_path until root */
> >    walk(starting_path, root, path) {
> >     callback_fn(path, cb_data);
> >   }
> >   return xxx;
> > }
> >
> > Personally, I don't like this version very much, because the callback
> > mechanism is not very flexible, and it is tricky to use it in BPF LSM.
>
> Aside from the "exposing mount seqcounts" problem, what do you think about
> the parent_iterator approach I suggested earlier?  I feel that it is
> better than such a callback - more flexible, and also fits in right with
> the BPF API you already designed (i.e. with a callback you might then have
> to allow BPF to pass a callback?).  There are some specifics that I can
> improve - Mickaël suggested some in our discussion:
>
> - Letting the caller take rcu_read_lock outside rather than doing it in
> path_walk_parent_start
>
> - Instead of always requiring a struct parent_iterator, allow passing in
> NULL for the iterator to path_walk_parent to do a reference walk without
> needing to call path_walk_parent_start - this way might be simpler and
> path_walk_parent_start/end can just be for rcu case.
>
> but what do you think about the overall shape of it?

Personally, I don't have strong objections to this design. But VFS
folks may have other concerns with it.

Thanks,
Song

[...]

On Wed 11-06-25 11:08:30, Song Liu wrote:
> On Wed, Jun 11, 2025 at 10:50 AM Tingmao Wang <m@maowtm.org> wrote:
> [...]
> > > I think we will need some callback mechanism for this. Something like:
> > >
> > > for_each_parents(starting_path, root, callback_fn, cb_data, bool try_rcu) {
> > >    if (!try_rcu)
> > >       goto ref_walk;
> > >
> > >    __read_seqcount_begin();
> > >     /* rcu walk parents, from starting_path until root */
> > >    walk_rcu(starting_path, root, path) {
> > >     callback_fn(path, cb_data);
> > >   }
> > >   if (!read_seqcount_retry())
> > >     return xxx;  /* successful rcu walk */
> > >
> > > ref_walk:
> > >   /* ref walk parents, from starting_path until root */
> > >    walk(starting_path, root, path) {
> > >     callback_fn(path, cb_data);
> > >   }
> > >   return xxx;
> > > }
> > >
> > > Personally, I don't like this version very much, because the callback
> > > mechanism is not very flexible, and it is tricky to use it in BPF LSM.
> >
> > Aside from the "exposing mount seqcounts" problem, what do you think about
> > the parent_iterator approach I suggested earlier?  I feel that it is
> > better than such a callback - more flexible, and also fits in right with
> > the BPF API you already designed (i.e. with a callback you might then have
> > to allow BPF to pass a callback?).  There are some specifics that I can
> > improve - Mickaël suggested some in our discussion:
> >
> > - Letting the caller take rcu_read_lock outside rather than doing it in
> > path_walk_parent_start
> >
> > - Instead of always requiring a struct parent_iterator, allow passing in
> > NULL for the iterator to path_walk_parent to do a reference walk without
> > needing to call path_walk_parent_start - this way might be simpler and
> > path_walk_parent_start/end can just be for rcu case.
> >
> > but what do you think about the overall shape of it?
> 
> Personally, I don't have strong objections to this design. But VFS
> folks may have other concerns with it.

From what I've read above I'm not sure about details of the proposal but I
don't think mixing of RCU & non-RCU walk in a single function / iterator is
a good idea. IMHO the code would be quite messy. After all we have
follow_dotdot_rcu() and follow_dotdot() as separate functions for a reason.
Also given this series went through several iterations and we don't yet
have an acceptable / correct solution suggests getting even the standard
walk correct is hard enough. RCU walk is going to be only worse. So I'd
suggest to get the standard walk finished and agreed on first and
investigate feasibility of RCU variant later.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

On Thu 12-06-25 11:01:16, Jan Kara wrote:
> On Wed 11-06-25 11:08:30, Song Liu wrote:
> > On Wed, Jun 11, 2025 at 10:50 AM Tingmao Wang <m@maowtm.org> wrote:
> > [...]
> > > > I think we will need some callback mechanism for this. Something like:
> > > >
> > > > for_each_parents(starting_path, root, callback_fn, cb_data, bool try_rcu) {
> > > >    if (!try_rcu)
> > > >       goto ref_walk;
> > > >
> > > >    __read_seqcount_begin();
> > > >     /* rcu walk parents, from starting_path until root */
> > > >    walk_rcu(starting_path, root, path) {
> > > >     callback_fn(path, cb_data);
> > > >   }
> > > >   if (!read_seqcount_retry())
> > > >     return xxx;  /* successful rcu walk */
> > > >
> > > > ref_walk:
> > > >   /* ref walk parents, from starting_path until root */
> > > >    walk(starting_path, root, path) {
> > > >     callback_fn(path, cb_data);
> > > >   }
> > > >   return xxx;
> > > > }
> > > >
> > > > Personally, I don't like this version very much, because the callback
> > > > mechanism is not very flexible, and it is tricky to use it in BPF LSM.
> > >
> > > Aside from the "exposing mount seqcounts" problem, what do you think about
> > > the parent_iterator approach I suggested earlier?  I feel that it is
> > > better than such a callback - more flexible, and also fits in right with
> > > the BPF API you already designed (i.e. with a callback you might then have
> > > to allow BPF to pass a callback?).  There are some specifics that I can
> > > improve - Mickaël suggested some in our discussion:
> > >
> > > - Letting the caller take rcu_read_lock outside rather than doing it in
> > > path_walk_parent_start
> > >
> > > - Instead of always requiring a struct parent_iterator, allow passing in
> > > NULL for the iterator to path_walk_parent to do a reference walk without
> > > needing to call path_walk_parent_start - this way might be simpler and
> > > path_walk_parent_start/end can just be for rcu case.
> > >
> > > but what do you think about the overall shape of it?
> > 
> > Personally, I don't have strong objections to this design. But VFS
> > folks may have other concerns with it.
> 
> From what I've read above I'm not sure about details of the proposal but I
> don't think mixing of RCU & non-RCU walk in a single function / iterator is
> a good idea. IMHO the code would be quite messy. After all we have
> follow_dotdot_rcu() and follow_dotdot() as separate functions for a reason.
> Also given this series went through several iterations and we don't yet
> have an acceptable / correct solution suggests getting even the standard
> walk correct is hard enough. RCU walk is going to be only worse. So I'd
> suggest to get the standard walk finished and agreed on first and
> investigate feasibility of RCU variant later.

OK, I've now read some of Tingmaon's and Christian's replies which I've
missed previously so I guess I now better understand why you complicate
things with RCU walking but still I'm of the opinion that we should start
with getting the standard walk working. IMHO pulling in RCU walk into the
iterator will bring it to a completely new complexity level...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

On Thu, Jun 12, 2025 at 11:49:08AM +0200, Jan Kara wrote:
> On Thu 12-06-25 11:01:16, Jan Kara wrote:
> > On Wed 11-06-25 11:08:30, Song Liu wrote:
> > > On Wed, Jun 11, 2025 at 10:50 AM Tingmao Wang <m@maowtm.org> wrote:
> > > [...]
> > > > > I think we will need some callback mechanism for this. Something like:
> > > > >
> > > > > for_each_parents(starting_path, root, callback_fn, cb_data, bool try_rcu) {
> > > > >    if (!try_rcu)
> > > > >       goto ref_walk;
> > > > >
> > > > >    __read_seqcount_begin();
> > > > >     /* rcu walk parents, from starting_path until root */
> > > > >    walk_rcu(starting_path, root, path) {
> > > > >     callback_fn(path, cb_data);
> > > > >   }
> > > > >   if (!read_seqcount_retry())
> > > > >     return xxx;  /* successful rcu walk */
> > > > >
> > > > > ref_walk:
> > > > >   /* ref walk parents, from starting_path until root */
> > > > >    walk(starting_path, root, path) {
> > > > >     callback_fn(path, cb_data);
> > > > >   }
> > > > >   return xxx;
> > > > > }
> > > > >
> > > > > Personally, I don't like this version very much, because the callback
> > > > > mechanism is not very flexible, and it is tricky to use it in BPF LSM.
> > > >
> > > > Aside from the "exposing mount seqcounts" problem, what do you think about
> > > > the parent_iterator approach I suggested earlier?  I feel that it is
> > > > better than such a callback - more flexible, and also fits in right with
> > > > the BPF API you already designed (i.e. with a callback you might then have
> > > > to allow BPF to pass a callback?).  There are some specifics that I can
> > > > improve - Mickaël suggested some in our discussion:
> > > >
> > > > - Letting the caller take rcu_read_lock outside rather than doing it in
> > > > path_walk_parent_start
> > > >
> > > > - Instead of always requiring a struct parent_iterator, allow passing in
> > > > NULL for the iterator to path_walk_parent to do a reference walk without
> > > > needing to call path_walk_parent_start - this way might be simpler and
> > > > path_walk_parent_start/end can just be for rcu case.
> > > >
> > > > but what do you think about the overall shape of it?
> > > 
> > > Personally, I don't have strong objections to this design. But VFS
> > > folks may have other concerns with it.
> > 
> > From what I've read above I'm not sure about details of the proposal but I
> > don't think mixing of RCU & non-RCU walk in a single function / iterator is
> > a good idea. IMHO the code would be quite messy. After all we have
> > follow_dotdot_rcu() and follow_dotdot() as separate functions for a reason.
> > Also given this series went through several iterations and we don't yet
> > have an acceptable / correct solution suggests getting even the standard
> > walk correct is hard enough. RCU walk is going to be only worse. So I'd
> > suggest to get the standard walk finished and agreed on first and
> > investigate feasibility of RCU variant later.
> 
> OK, I've now read some of Tingmaon's and Christian's replies which I've
> missed previously so I guess I now better understand why you complicate
> things with RCU walking but still I'm of the opinion that we should start
> with getting the standard walk working. IMHO pulling in RCU walk into the
> iterator will bring it to a completely new complexity level...

I would not want it in the first place. But I have a deep seated
aversion to exposing two different variants. Especially if the second
variant wants or needs access to internal details such as mount or
dentry sequence counts. I'm not at all in favor of that.

On 6/12/25 13:31, Christian Brauner wrote:
> On Thu, Jun 12, 2025 at 11:49:08AM +0200, Jan Kara wrote:
>> On Thu 12-06-25 11:01:16, Jan Kara wrote:
>>> On Wed 11-06-25 11:08:30, Song Liu wrote:
>>>> On Wed, Jun 11, 2025 at 10:50 AM Tingmao Wang <m@maowtm.org> wrote:
>>>>> [...]
>>>>> Aside from the "exposing mount seqcounts" problem, what do you think about
>>>>> the parent_iterator approach I suggested earlier?  I feel that it is
>>>>> better than such a callback - more flexible, and also fits in right with
>>>>> the BPF API you already designed (i.e. with a callback you might then have
>>>>> to allow BPF to pass a callback?).  There are some specifics that I can
>>>>> improve - Mickaël suggested some in our discussion:
>>>>>
>>>>> - Letting the caller take rcu_read_lock outside rather than doing it in
>>>>> path_walk_parent_start
>>>>>
>>>>> - Instead of always requiring a struct parent_iterator, allow passing in
>>>>> NULL for the iterator to path_walk_parent to do a reference walk without
>>>>> needing to call path_walk_parent_start - this way might be simpler and
>>>>> path_walk_parent_start/end can just be for rcu case.
>>>>>
>>>>> but what do you think about the overall shape of it?
>>>>
>>>> Personally, I don't have strong objections to this design. But VFS
>>>> folks may have other concerns with it.
>>>
>>> From what I've read above I'm not sure about details of the proposal but I
>>> don't think mixing of RCU & non-RCU walk in a single function / iterator is
>>> a good idea. IMHO the code would be quite messy. After all we have
>>> follow_dotdot_rcu() and follow_dotdot() as separate functions for a reason.
>>> Also given this series went through several iterations and we don't yet
>>> have an acceptable / correct solution suggests getting even the standard
>>> walk correct is hard enough. RCU walk is going to be only worse. So I'd
>>> suggest to get the standard walk finished and agreed on first and
>>> investigate feasibility of RCU variant later.
>>
>> OK, I've now read some of Tingmaon's and Christian's replies which I've
>> missed previously so I guess I now better understand why you complicate
>> things with RCU walking but still I'm of the opinion that we should start
>> with getting the standard walk working. IMHO pulling in RCU walk into the
>> iterator will bring it to a completely new complexity level...
> 
> I would not want it in the first place. But I have a deep seated
> aversion to exposing two different variants.

Hi Christian, Jan, Song,

I do appreciate your thoughts here and thanks for taking the time to
explain.  I just have some specific points which I would like you to
consider:

Taking a step back, maybe the specific designs need a bit more thought,
but are you at all open to the idea of letting other subsystems take
advantage of a rcu-based parent walk?  Testing shows that for specific
cases of a deep directory hierarchy the speedup (for time in Landlock) can
be almost 60%, and still very significant for the average case. [1]

I think what I'm proposing here is basically what follow_dotdot_rcu
already does (aside from checking rename_seq, but actually that was mostly
a conservative check. I think we're good even if we just check dentry seq
across the path walk calls), and in fact given the latest suggestion to
base the path walk helper on a modified version of follow_dotdot (that
takes path argument instead of using nameidata), I can see one approach
here being to do the same for follow_dotdot_rcu (i.e. extracting the logic
from start to before "nd->next_seq = read_seqcount_begin(&parent->d_seq);").
That way, we will not be "inventing" any new code that messes with VFS
internals.

In respect to the comment from Jan, I'm putting the suggestion out right
now to avoid only surfacing this ask after Song's path iterator API has
just been merged.  I'm not saying we have to do it here and now, but if
there is at all a possibility of incorporating rcu-based walk in this
helper (or a separate helper - I personally don't mind either way), I
would like to make sure that possibility stays open.

I'm happy to wait till Song's current patch is finished before continuing
this, but if there is strong objection to two separate APIs, I would
really appreciate if we can end up in a state where further change to
implement this is possible.

> Especially if the second variant wants or needs access to internal details
> such as mount or dentry sequence counts. I'm not at all in favor of that.

I don't want to expose VFS internals, but are you worried about even
making use of them?  (well, rename_lock and d_seq is already accessible
from outside since they are defined in include/linux/dcache.h, and so it's
just the (readout of the) mount seqcount here that will gain additional
exposure, but maybe we can mark it with __private.)

Would it be less worrying if any checks against those seqcount values are
kept within follow_dotdot_rcu, but just that it is stored in an iterator
that outside code are supposed to treat as opaque?  (We can maybe define
the semantic here as basically "this iterator makes sure your rcu walk
can't result in states where a reference-taking walk won't get to, as long
as you retry when the function returns -EAGAIN (or maybe -ECHILD)").

[1]: https://lore.kernel.org/all/cover.1748997840.git.m@maowtm.org/#t

Thanks a lot,
Tingmao

On Sun, Jun 15, 2025 at 5:24 PM Tingmao Wang <m@maowtm.org> wrote:
[...]
> >
> > I would not want it in the first place. But I have a deep seated
> > aversion to exposing two different variants.
>
> Hi Christian, Jan, Song,
>
> I do appreciate your thoughts here and thanks for taking the time to
> explain.  I just have some specific points which I would like you to
> consider:
>
> Taking a step back, maybe the specific designs need a bit more thought,
> but are you at all open to the idea of letting other subsystems take
> advantage of a rcu-based parent walk?

I cannot really speak for VFS folks, but I guess rcu-based parent walk
out of fs/ is not preferred.

> Testing shows that for specific
> cases of a deep directory hierarchy the speedup (for time in Landlock) can
> be almost 60%, and still very significant for the average case. [1]
[...]
> I'm happy to wait till Song's current patch is finished before continuing
> this, but if there is strong objection to two separate APIs, I would
> really appreciate if we can end up in a state where further change to
> implement this is possible.

In v5, path_walk_parent API is not exported. We can easily change it
in the future. Therefore, I don't think we need to rush into a rcu-walk
design before landing path_walk_parent.

Thanks,
Song

[...]

On Fri, Jun 06, 2025 at 02:30:11PM -0700, Song Liu wrote:
> This helper walks an input path to its parent. Logic are added to handle
> walking across mount tree.
> 
> This will be used by landlock, and BPF LSM.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  fs/namei.c            | 51 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/namei.h |  2 ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 4bb889fc980b..f02183e9c073 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1424,6 +1424,57 @@ static bool choose_mountpoint(struct mount *m, const struct path *root,
>  	return found;
>  }
>  
> +/**
> + * path_walk_parent - Walk to 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.
> + *
> + * The logic of path_walk_parent() is similar to follow_dotdot(), except
> + * that path_walk_parent() will continue walking for !path_connected case.
> + * This effectively means we are walking from disconnected bind mount to
> + * the original mount. If this behavior is not desired, the caller can add
> + * a check like:
> + *
> + *   if (path_walk_parent(&path) && !path_connected(path.mnt, path.dentry)
> + *           // continue walking
> + *   else
> + *           // stop walking
> + *
> + * Returns:
> + *  true  - if @path is updated to its parent.
> + *  false - if @path is already the root (real root or @root).
> + */
> +bool path_walk_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;
> +	}
> +
> +	if (unlikely(IS_ROOT(path->dentry)))

path would be updated while false is returned, which is not correct.

> +		return false;
> +
> +	parent = dget_parent(path->dentry);
> +	dput(path->dentry);
> +	path->dentry = parent;
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(path_walk_parent);
> +
>  /*
>   * Perform an automount
>   * - return -EISDIR to tell follow_managed() to stop and return the path we
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 5d085428e471..cba5373ecf86 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -85,6 +85,8 @@ extern int follow_down_one(struct path *);
>  extern int follow_down(struct path *path, unsigned int flags);
>  extern int follow_up(struct path *);
>  
> +bool path_walk_parent(struct path *path, const struct path *root);
> +
>  extern struct dentry *lock_rename(struct dentry *, struct dentry *);
>  extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
>  extern void unlock_rename(struct dentry *, struct dentry *);
> -- 
> 2.47.1
> 
>

On Tue, Jun 10, 2025 at 10:19 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Fri, Jun 06, 2025 at 02:30:11PM -0700, Song Liu wrote:
> > This helper walks an input path to its parent. Logic are added to handle
> > walking across mount tree.
> >
> > This will be used by landlock, and BPF LSM.
> >
> > Signed-off-by: Song Liu <song@kernel.org>
> > ---
> >  fs/namei.c            | 51 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/namei.h |  2 ++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 4bb889fc980b..f02183e9c073 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1424,6 +1424,57 @@ static bool choose_mountpoint(struct mount *m, const struct path *root,
> >       return found;
> >  }
> >
> > +/**
> > + * path_walk_parent - Walk to 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.
> > + *
> > + * The logic of path_walk_parent() is similar to follow_dotdot(), except
> > + * that path_walk_parent() will continue walking for !path_connected case.
> > + * This effectively means we are walking from disconnected bind mount to
> > + * the original mount. If this behavior is not desired, the caller can add
> > + * a check like:
> > + *
> > + *   if (path_walk_parent(&path) && !path_connected(path.mnt, path.dentry)
> > + *           // continue walking
> > + *   else
> > + *           // stop walking
> > + *
> > + * Returns:
> > + *  true  - if @path is updated to its parent.
> > + *  false - if @path is already the root (real root or @root).
> > + */
> > +bool path_walk_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;
> > +     }
> > +
> > +     if (unlikely(IS_ROOT(path->dentry)))
>
> path would be updated while false is returned, which is not correct.

Good catch.. How about the following:

bool path_walk_parent(struct path *path, const struct path *root)
{
        struct dentry *parent;
        bool ret = false;

        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;
                ret = true;
        }

        if (unlikely(IS_ROOT(path->dentry)))
                return ret;

        parent = dget_parent(path->dentry);
        dput(path->dentry);
        path->dentry = parent;
        return true;
}

Thanks,
Song

On 6/10/25 18:26, Song Liu wrote:
> On Tue, Jun 10, 2025 at 10:19 AM Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On Fri, Jun 06, 2025 at 02:30:11PM -0700, Song Liu wrote:
>>> [...]
>>> + * Returns:
>>> + *  true  - if @path is updated to its parent.
>>> + *  false - if @path is already the root (real root or @root).
>>> + */
>>> +bool path_walk_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;
>>> +     }
>>> +
>>> +     if (unlikely(IS_ROOT(path->dentry)))
>>
>> path would be updated while false is returned, which is not correct.
> 
> Good catch.. How about the following:
> 
> bool path_walk_parent(struct path *path, const struct path *root)
> {
>         struct dentry *parent;
>         bool ret = false;
> 
>         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;
>                 ret = true;
>         }
> 
>         if (unlikely(IS_ROOT(path->dentry)))
>                 return ret;

Returning true here would be the wrong semantic right?  This whole thing
is only possible when some mount shadows "/".  Say if you have a landlock
rule on the old "/", but then we mount a new "/" and chroot into it (via
"/.."), the landlock rule on the old "/" should not apply, but if we
change *path and return true here then this will "expose" that old "/" to
landlock.

A quick suggestion although I haven't tested anything - maybe we should do
a special case check for IS_ROOT inside the
    if (unlikely(path->dentry == path->mnt->mnt_root))
? Before "path_put(path);", if IS_ROOT(p.dentry) then we just path_get(p)
and return false.

> 
>         parent = dget_parent(path->dentry);
>         dput(path->dentry);
>         path->dentry = parent;
>         return true;
> }
> 
> Thanks,
> Song

On Tue, Jun 10, 2025 at 3:26 PM Tingmao Wang <m@maowtm.org> wrote:
[..]
> >
> >                 if (!choose_mountpoint(real_mount(path->mnt), root, &p))
> >                         return false;
> >                 path_put(path);
> >                 *path = p;
> >                 ret = true;
> >         }
> >
> >         if (unlikely(IS_ROOT(path->dentry)))
> >                 return ret;
>
> Returning true here would be the wrong semantic right?  This whole thing
> is only possible when some mount shadows "/".  Say if you have a landlock
> rule on the old "/", but then we mount a new "/" and chroot into it (via
> "/.."), the landlock rule on the old "/" should not apply, but if we
> change *path and return true here then this will "expose" that old "/" to
> landlock.

Could you please provide more specific information about this case?

Thanks,
Song

> A quick suggestion although I haven't tested anything - maybe we should do
> a special case check for IS_ROOT inside the
>     if (unlikely(path->dentry == path->mnt->mnt_root))
> ? Before "path_put(path);", if IS_ROOT(p.dentry) then we just path_get(p)
> and return false.
>
> >
> >         parent = dget_parent(path->dentry);
> >         dput(path->dentry);
> >         path->dentry = parent;
> >         return true;
> > }
> >
> > Thanks,
> > Song
>

On 6/11/25 00:08, Song Liu wrote:
> On Tue, Jun 10, 2025 at 3:26 PM Tingmao Wang <m@maowtm.org> wrote:
> [..]
>>>
>>>                 if (!choose_mountpoint(real_mount(path->mnt), root, &p))
>>>                         return false;
>>>                 path_put(path);
>>>                 *path = p;
>>>                 ret = true;
>>>         }
>>>
>>>         if (unlikely(IS_ROOT(path->dentry)))
>>>                 return ret;
>>
>> Returning true here would be the wrong semantic right?  This whole thing
>> is only possible when some mount shadows "/".  Say if you have a landlock
>> rule on the old "/", but then we mount a new "/" and chroot into it (via
>> "/.."), the landlock rule on the old "/" should not apply, but if we
>> change *path and return true here then this will "expose" that old "/" to
>> landlock.
> 
> Could you please provide more specific information about this case?

Apologies, it looks like I was mistaken in the above statement.

I was thinking of something like

# mount --mkdir -t tmpfs none tmproot
# cp busybox tmproot/ && chmod +x tmproot/busybox
# mount --move tmproot /
# env LL_FS_RW=/ LL_FS_RO=/.. ./sandboxer chroot /.. /busybox sh
  # echo can write to root > /a
  sh: can't create /a: Permission denied
  ^^^^ this does not work, but I was mistakenly thinking it would

I think because choose_mountpoint_rcu only returns true if
    if (mountpoint != m->mnt.mnt_root)
passes, this situation won't cause ret to be true in your code.

But then I can't think of when
      if (unlikely(IS_ROOT(path->dentry)))
          return ret;
would ever return true, unless somehow d_parent is corrupted?  Maybe I'm
just missing something obvious here.

Anyway, since there's a suggestion from Neil to refactor this, this might
not be too important, so feel free to ignore for now.

> 
> Thanks,
> Song
> 
>> A quick suggestion although I haven't tested anything - maybe we should do
>> a special case check for IS_ROOT inside the
>>     if (unlikely(path->dentry == path->mnt->mnt_root))
>> ? Before "path_put(path);", if IS_ROOT(p.dentry) then we just path_get(p)
>> and return false.
>>
>>>
>>>         parent = dget_parent(path->dentry);
>>>         dput(path->dentry);
>>>         path->dentry = parent;
>>>         return true;
>>> }
>>>
>>> Thanks,
>>> Song
>>

On 6/10/25 23:26, Tingmao Wang wrote:
> [...]
> 
> A quick suggestion although I haven't tested anything - maybe we should do
> a special case check for IS_ROOT inside the
>     if (unlikely(path->dentry == path->mnt->mnt_root))
> ? Before "path_put(path);", if IS_ROOT(p.dentry) then we just path_get(p)
                                                                     ^^^ sorry I meant put
> and return false.