Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance

[Jann Horn]

On Fri, Dec 11, 2015 at 02:52:01PM -0800, Andy Lutomirski wrote:
> On Fri, Dec 11, 2015 at 2:35 PM, Eric W. Biederman
> <ebiederm@xxxxxxxxxxxx> wrote:
> > Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes:
> >
> >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@xxxxxxxxx> wrote:
> >>> On 12/11/15 13:48, Andy Lutomirski wrote:
> >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
> >>>> <ebiederm@xxxxxxxxxxxx> wrote:
> >>>>> Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes:
> >>>>>
> >>>>>> On Fri, Dec 11, 2015 at 01:40:40PM -0600, Eric W. Biederman wrote:
> >>>>>>
> >>>>>>> +    inode = path.dentry->d_inode;
> >>>>>>> +    filp->f_path = path;
> >>>>>>> +    filp->f_inode = inode;
> >>>>>>> +    filp->f_mapping = inode->i_mapping;
> >>>>>>> +    path_put(&old);
> >>>>>>
> >>>>>> Don't.  You are creating a fairly subtle constraint on what the code in
> >>>>>> fs/open.c and fs/namei.c can do, for no good reason.  You can bloody
> >>>>>> well maintain the information you need without that.
> >>>>>
> >>>>> There is a good reason.  We can not write a race free version of ptsname
> >>>>> without it.
> >>>>
> >>>> As long as this is for new userspace code, would it make sense to just
> >>>> add a new ioctl to ask "does this ptmx fd match this /dev/pts fd?"
> >>>>
> >>>
> >>> For the newinstance case st_dev should match between the master and the
> >>> slave.  Unfortunately this is not the case for a legacy ptmx, as a
> >>> stat() on the master descriptor still returns the st_dev, st_rdev, and
> >>> st_ino for the ptmx device node.
> >>
> >> Sure, but I'm not talking about stat.  I'm saying that we could add a
> >> new ioctl that works on any ptmx fd (/dev/ptmx or /dev/pts/ptmx) that
> >> answers the question "does this ptmx logically belong to the given
> >> devpts filesystem".
> >>
> >> Since it's not stat, we can make it do whatever we want, including
> >> following a link to the devpts instance that isn't f_path or f_inode.
> >
> > The useful ioctl to add in my opinion would be one that actually opens
> > the slave, at which point ptsname could become ttyname, and that closes
> > races in grantpt.
> 
> Unfortunately, ptsname is POSIX, so we can't get rid of it.  It's a
> bad idea, but it's in the standard.

But then ptsname could become "open the slave, call ttyname() on it, close
the slave". Unless opening the slave would have side effects?

Attachment: signature.asc
Description: Digital signature