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

[H. Peter Anvin]

On December 11, 2015 3:16:48 PM PST, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>On Fri, Dec 11, 2015 at 3:07 PM, H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>> On December 11, 2015 3:00:49 PM PST, Andy Lutomirski
><luto@xxxxxxxxxxxxxx> wrote:
>>>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
>>
>> I want to be clear:
>>
>> If /dev/ptmx -> pts/ptmx and devpts is mounted with the proper
>options, I believe ask the remaining parts of userspace should be fine,
>and pt_chown can be removed even with glibc.
>>
>> The magic ptmx we are talking about is all about dealing with a
>mismanaged /dev.
>> --
>> Sent from my Android device with K-9 Mail. Please excuse brevity and
>formatting.
>
>I think you're right, modulo the one stupidity that a configuration
>like that is prone to breakage with container apps running on the same
>system.
>
>Hmm.  Could userspace be changed to set newinstance=1 on its /dev/pts
>mount to work around that?
>
>--Andy

 The newinstance option was always meant to be transitory, to let the kernel know that userspace has been properly enabled with a ptmx symlink, except that the user space enabling never happened, just as it was never done properly with devpts in the first place.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
--
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/