lkml.org 
[lkml]   [2015]   [Dec]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] devpts: Sensible /dev/ptmx & force newinstance
    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


    \
     
     \ /
      Last update: 2015-12-11 22:41    [W:4.207 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site