[PATCH 0/3 RESEND] namei: add follow_up_bind()

[PATCH 0/3 RESEND] namei: add follow_up_bind()

[Christian Brauner]

Hi everyone,

(Resending since Linus email got mangled on my terminal. Sorry.)

Back when we fixed TIOCGPTPEER again in

commit a319b01d9095 ("devpts: resolve devpts bind-mounts")

we discovered [1] that the code for bind-mount resolution we needed to add
in devpts_mtnget() was already duplicated in nfsd code. So we briefly
discussed [2] adding a helper to namei.{c,h} that would resolve
bind-mounts. The bind-mount resolution code is replicated in at least two
places:
- fs/nfsd/vfs.c:follow_to_parent()
- fs/devpts/inode.c:devpts_mntget()

This series adds:
- follow_up_bind() to namei.{c,h}
- switches fs/nfsd/vfs.c:follow_to_parent() to use follow_up_bind()
- switches fs/devpts/inode.c:devpts_mntget() to use follow_up_bind()

I just wanted to wait until the devpts patches I sent would make it into
mainline. Seems to me that this helper might be worth having around. Not
just because it avoids (granted rather trivial) code duplication but also
because it makes the concept of resolving a bind-mount up to the origin
mountpoint of the vfs's mount obvious (which at least to me wasn't
obivous before).

[1]: https://lkml.org/lkml/2018/3/11/219
[2]: https://lkml.org/lkml/2018/3/12/486

Thanks!
Christian

Christian Brauner (3):
  namei: add follow_up_bind()
  devpts: use follow_up_bind() helper
  nfsd: use follow_up_bind() helper

 fs/devpts/inode.c     |  4 +---
 fs/namei.c            | 10 ++++++++++
 fs/nfsd/vfs.c         |  4 ++--
 include/linux/namei.h |  1 +
 4 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.15.1

[PATCH 1/3 RESEND] namei: add follow_up_bind()

[Christian Brauner]

This adds a new helper for resolving bind-mounts.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/namei.c            | 10 ++++++++++
 include/linux/namei.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index a09419379f5d..4fa56ec78f63 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1085,6 +1085,16 @@ int follow_up(struct path *path)
 }
 EXPORT_SYMBOL(follow_up);
 
+/*
+ * follow_up_bind - Resolve bind-mounts to mountpoint of path's vfsmount
+ */
+inline void follow_up_bind(struct path *path)
+{
+	while (path->mnt->mnt_root == path->dentry && follow_up(path))
+		;
+}
+EXPORT_SYMBOL(follow_up_bind);
+
 /*
  * 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 a982bb7cd480..ea93127be26c 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -87,6 +87,7 @@ extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int
 extern int follow_down_one(struct path *);
 extern int follow_down(struct path *);
 extern int follow_up(struct path *);
+extern void follow_up_bind(struct path *path);
 
 extern struct dentry *lock_rename(struct dentry *, struct dentry *);
 extern void unlock_rename(struct dentry *, struct dentry *);
-- 
2.15.1

[PATCH 2/3 RESEND] devpts: use follow_up_bind() helper

[Christian Brauner]

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/devpts/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e072e955ce33..5e516846074e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -181,9 +181,7 @@ struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
 	/* Walk upward while the start point is a bind mount of
 	 * a single file.
 	 */
-	while (path.mnt->mnt_root == path.dentry)
-		if (follow_up(&path) == 0)
-			break;
+	follow_up_bind(&path);
 
 	/* devpts_ptmx_path() finds a devpts fs or returns an error. */
 	if ((path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) ||
-- 
2.15.1

[PATCH 3/3 RESEND] nfsd: use follow_up_bind() helper

[Christian Brauner]

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/nfsd/vfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index a3c9bfa77def..52e64036d2bd 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -141,8 +141,8 @@ static void follow_to_parent(struct path *path)
 {
 	struct dentry *dp;
 
-	while (path->dentry == path->mnt->mnt_root && follow_up(path))
-		;
+	follow_up_bind(path);
+
 	dp = dget_parent(path->dentry);
 	dput(path->dentry);
 	path->dentry = dp;
-- 
2.15.1

Re: [PATCH 0/3 RESEND] namei: add follow_up_bind()

[Linus Torvalds]

On Thu, Apr 5, 2018 at 3:51 AM, Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> This series adds:
> - follow_up_bind() to namei.{c,h}
> - switches fs/nfsd/vfs.c:follow_to_parent() to use follow_up_bind()
> - switches fs/devpts/inode.c:devpts_mntget() to use follow_up_bind()

Hmm. Seems fair enough to me, although I wonder how much this really
helps. It does get rid of a duplicate code pattern, but:

 4 files changed, 14 insertions(+), 5 deletions(-)

and while some of that is just the new comment, some of it is just "overhead".

It's also a bit odd how the new helper is marked "inline", but nobody
will inline it because it's not actually in the header file or any of
the isers in the same C file. So instead, it has to be exported. I
wonder if it should just be a trivial inline in <linux/namei.h>? Maybe
it originally was, and that's where the inline came from, and then
Christian decided to make it be by the regular "follow_up()" instead?

But with all that said, I certainly don't *mind* the patch series.

Al, I'm leaving this up to you, and expect to get it from your vfs
tree eventually. Or not.

                      Linus

Re: [PATCH 0/3 RESEND] namei: add follow_up_bind()

[Christian Brauner]

On Thu, Apr 05, 2018 at 09:28:56AM -0700, Linus Torvalds wrote:
> On Thu, Apr 5, 2018 at 3:51 AM, Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > This series adds:
> > - follow_up_bind() to namei.{c,h}
> > - switches fs/nfsd/vfs.c:follow_to_parent() to use follow_up_bind()
> > - switches fs/devpts/inode.c:devpts_mntget() to use follow_up_bind()
> 
> Hmm. Seems fair enough to me, although I wonder how much this really
> helps. It does get rid of a duplicate code pattern, but:
> 
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> and while some of that is just the new comment, some of it is just "overhead".

Fwiw, it does get read of these while loops in two places but I
personally see the biggest value in making it obvious what bind-mount
resolution means.

> 
> It's also a bit odd how the new helper is marked "inline", but nobody
> will inline it because it's not actually in the header file or any of
> the isers in the same C file. So instead, it has to be exported. I
> wonder if it should just be a trivial inline in <linux/namei.h>? Maybe
> it originally was, and that's where the inline came from, and then
> Christian decided to make it be by the regular "follow_up()" instead?

I head it inline first but it would have required to forward declare
struct vfsmount in the head and I wasn't sure if that was going to fly.
But I explicitly left the inline in there because I was following
user_path_create() ([1], [2]) which does the same. But if that's an
issue I can make it static inline in the header like I had, forward
declare struct vfsmount and remove the unnecessary inline from
user_path_create() in a separate patch unless there's a specific reason
to leave it in there.

[1]: https://elixir.bootlin.com/linux/latest/source/include/linux/namei.h#L79
[2]: https://elixir.bootlin.com/linux/latest/source/fs/namei.c#L3680

> 
> But with all that said, I certainly don't *mind* the patch series.

Cool.

Thanks!
Christian

> 
> Al, I'm leaving this up to you, and expect to get it from your vfs
> tree eventually. Or not.
> 
>                       Linus

Re: [PATCH 0/3 RESEND] namei: add follow_up_bind()

[Christian Brauner]

On Thu, Apr 05, 2018 at 07:45:15PM +0200, Christian Brauner wrote:
> On Thu, Apr 05, 2018 at 09:28:56AM -0700, Linus Torvalds wrote:
> > On Thu, Apr 5, 2018 at 3:51 AM, Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > >
> > > This series adds:
> > > - follow_up_bind() to namei.{c,h}
> > > - switches fs/nfsd/vfs.c:follow_to_parent() to use follow_up_bind()
> > > - switches fs/devpts/inode.c:devpts_mntget() to use follow_up_bind()
> > 
> > Hmm. Seems fair enough to me, although I wonder how much this really
> > helps. It does get rid of a duplicate code pattern, but:
> > 
> >  4 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > and while some of that is just the new comment, some of it is just "overhead".
> 
> Fwiw, it does get read of these while loops in two places but I
> personally see the biggest value in making it obvious what bind-mount
> resolution means.
> 
> > 
> > It's also a bit odd how the new helper is marked "inline", but nobody
> > will inline it because it's not actually in the header file or any of
> > the isers in the same C file. So instead, it has to be exported. I
> > wonder if it should just be a trivial inline in <linux/namei.h>? Maybe
> > it originally was, and that's where the inline came from, and then
> > Christian decided to make it be by the regular "follow_up()" instead?
> 
> I head it inline first but it would have required to forward declare
> struct vfsmount in the head and I wasn't sure if that was going to fly.
> But I explicitly left the inline in there because I was following
> user_path_create() ([1], [2]) which does the same. But if that's an
> issue I can make it static inline in the header like I had, forward
> declare struct vfsmount and remove the unnecessary inline from
> user_path_create() in a separate patch unless there's a specific reason
> to leave it in there.
> 
> [1]: https://elixir.bootlin.com/linux/latest/source/include/linux/namei.h#L79
> [2]: https://elixir.bootlin.com/linux/latest/source/fs/namei.c#L3680

In case that wasn't clear from the previous message: I'd wait for a go
ahead on this if that's ok.

Christian

> 
> > 
> > But with all that said, I certainly don't *mind* the patch series.
> 
> Cool.
> 
> Thanks!
> Christian
> 
> > 
> > Al, I'm leaving this up to you, and expect to get it from your vfs
> > tree eventually. Or not.
> > 
> >                       Linus