Re: [PATCH v1] devpts: resolve devpts bind-mounts

From: Christian Brauner
Date: Sun Mar 11 2018 - 07:30:27 EST


On Fri, Mar 09, 2018 at 10:37:34AM -0800, Linus Torvalds wrote:
> Hmm. This hunk annoys me and makes me go "Whaa?":
>
> On Fri, Mar 9, 2018 at 2:57 AM, Christian Brauner
> <christian.brauner@xxxxxxxxxx> wrote:
> > @@ -163,6 +159,26 @@ struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
> >
> > path = filp->f_path;
> > path_get(&path);
> > + if ((DEVPTS_SB(path.mnt->mnt_sb) == fsi) &&
> > + (path.mnt->mnt_root == fsi->ptmx_dentry)) {
> > + /* 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;
> > +
> > + /* Is this path a valid devpts filesystem? */
> > + err = devpts_ptmx_path(&path);
> > + dput(path.dentry);
> > + if (err == 0)
> > + goto check_devpts_sb;
> > +
> > + path_put(&path);
> > + path = filp->f_path;
> > + path_get(&path);
> > + goto check_devpts_sb;
> > + }
> >
> > err = devpts_ptmx_path(&path);
> > dput(path.dentry);
>
> why did you duplicate the devpts_ptmx_path() and then do that odd
> error handling?
>
> We only go into that "if()" statement if
> DEVPTS_SB(filp->f_path.mnt->mnt_sb) == fsi, so then when you do that
> "put path and re-get it, and go to check_devpts_sb", the
> check_devpts_sb won't actually _do_ anything, because it has
>
> > +check_devpts_sb:
> > if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
>
> and we know that "if()" there cannot trigger, since we just checked it earlier.
>
> So abou two thirds of the above seems unnecessary.
>
> Why isn't the code just doing
>
>
> if ((DEVPTS_SB(path.mnt->mnt_sb) == fsi) &&
> (path.mnt->mnt_root == fsi->ptmx_dentry)) {
> /* 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;
> }
>
> and then just falling through to the existing "devpts_ptmx_path()" etc
> code? Duplicating it seems wrong, and the error handling in the
> duplicated path seems wrong too.
>
> Am I missing something?

Right, the sb information can't be changed by follow_up() so we can
actually do it simpler. In the first iteration of the patch I wasn't
sure of that when I thought through the whole source pathname vs.
location on a different device mess for /dev, /dev/pts, and /dev/ptmx
being a bind-mount.

>
>
> > @@ -187,10 +206,16 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
> > path = filp->f_path;
> > path_get(&path);
> >
> > - err = devpts_ptmx_path(&path);
> > - if (err) {
> > - result = ERR_PTR(err);
> > - goto out;
> > + /* Has the devpts filesystem already been found? */
> > + if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) {
> > + /* Is there an appropriate devpts filesystem in the parent
> > + * directory?
> > + */
> > + err = devpts_ptmx_path(&path);
> > + if (err) {
> > + result = ERR_PTR(err);
> > + goto out;
> > + }
> > }
>
> This part (and the accompanying removal from devpts_ptmx_path() should
> just have been a separate preparatory patch that doesn't change
> semantics, no? Also, the scope of 'err' is now entirely inside that
> if(), so I think it should just be declared there too.

Yeah, I split this out into a non-functional change in the new version
of the patch.

Christian

>
> I didn't actually test the patch, this is just from reading it, so I

> might have missed something.