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

[Christian Brauner]

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.