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

[ebiederm@xmission.com (Eric W. Biederman)]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Fri, Dec 11, 2015 at 11:40 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>>
>> +struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
>> +{
>> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
>> +       struct path path, old;
>> +       struct super_block *sb;
>> +       struct dentry *root;
>> +
>> +       if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
>> +               return inode;
>> +
>> +       old = filp->f_path;
>> +       path = old;
>> +       path_get(&path);
>> +       if (kern_path_pts(&path)) {
>> +               path_put(&path);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>
> So this is definitely crap.
>
> You can't return an error. You should just return the old inode. If
> somebody doesn't have /dev/pts/ mounted there, the legacy /dev/ptmx
> should still work, not return ENOENT or whatever.

I return an error because an association is not found.  -EINVAL is the
historical error when that happens.

We can't actually support the old devpts_mnt hack, because we are
forcing newinstance and so the devpts instance association devpts_mnt
will never be delivered to userspace.   Furthermore things such as
initramfs images mounting devpts guarantee that even if we did try to
support devpts_mnt it would not work in practice.

So I really don't see a point in attemptting to support something that
won't actually matter, and won't work even if I try.  That is really
crap.

>> +       sb = path.mnt->mnt_sb;
>> +       if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
>> +               path_put(&path);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>
> Same deal. Returning an error is wrong.
>
> Of, alternatively, make the caller not consider an error an error, but
> fall back to the old behavior in the caller.

I understand why it would be nice to keep the old path still working,
but we can't.

>> +       /*
>> +        * Update filp with the new path so that userspace can use
>> +        * fstat to know which instance of devpts is open, and so
>> +        * userspace can use readlink /proc/self/fd/NNN to find the
>> +        * path to the devpts filesystem for reporting slave inodes.
>> +        */
>
> Hmm. I'm not 100% convinced about this. Normally we do *not* allow
> f_path and f_inode to change. I guess this file descriptor hasn't been
> exposed yet, so it might be ok, but it makes me a bit nervous that
> this code violates the basic filp rules..

It is not my favorite choice but it is backwards compatible.  So we can
at least write a single version of ptsname that makes sense and works on
old and new kernels in all the crazy corner cases without too much
pain.

If we don't expose this in such a way that a general purpose version of
ptsname can be written and exposed to userspace we might as well pack
our bags and go home because there will be no way to make grantpt race
free in the face of multiple distinct instances of devpts.

Eric