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

[Andy Lutomirski]

On Fri, Dec 11, 2015 at 2:58 PM, Jann Horn <jann@xxxxxxxxx> wrote:
> 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?

Hmm, fair enough.  So maybe that does make sense after all.

Anyway, I still think there are two pieces here:

1. Fix /dev/ptmx so that we can banish newinstance=0.

2. Fix libc.  If that needs kernel help, then so be it.

ISTM we could still implement the "open the slave" operation for (2)
as an ioctl that does the appropriate magic the fd is /dev/ptmx as
opposed to /dev/pts/ptmx.


--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/