Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1289863 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-11 20:50 +0100
    Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-11 22:00 +0100
      Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-11 22:20 +0100
    Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-11 22:10 +0100
      Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-11 22:30 +0100
        Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Andy Lutomirski <luto@amacapital.net> - 2015-12-11 22:50 +0100
          Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-11 23:20 +0100
            Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Andy Lutomirski <luto@amacapital.net> - 2015-12-11 23:30 +0100
              Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-11 23:40 +0100
          Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Andy Lutomirski <luto@amacapital.net> - 2015-12-11 23:20 +0100
            Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-11 23:50 +0100
              Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Jann Horn <jann@thejh.net> - 2015-12-12 00:00 +0100
                Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-12 00:10 +0100
                  Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Andy Lutomirski <luto@amacapital.net> - 2015-12-12 00:20 +0100
                    Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-12 00:40 +0100
                Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Andy Lutomirski <luto@amacapital.net> - 2015-12-12 00:10 +0100
              Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-12 00:00 +0100
              Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Andy Lutomirski <luto@amacapital.net> - 2015-12-12 00:00 +0100
          Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-11 23:20 +0100
    Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Peter Hurley <peter@hurleysoftware.com> - 2015-12-14 20:50 +0100
      Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-14 21:00 +0100
      Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-19 22:30 +0100
        Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-20 05:30 +0100
          Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance "H. Peter Anvin" <hpa@zytor.com> - 2015-12-20 05:40 +0100
            Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-20 11:00 +0100
              Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance ebiederm@xmission.com (Eric W. Biederman) - 2015-12-21 23:20 +0100
                Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-21 23:30 +0100

Page 1 of 2  [1] 2  Next page →

#1289863 — [PATCH] devpts: Sensible /dev/ptmx & force newinstance

The suid root helper of grantpt /usr/lib/pt_chown remains in use today
because of sloppy userspace code that mount devpts and does not realize
their change in mount options also applies to the primary system devpts
instance.  As the system devpts instance looses it gid=5 mount option
/usr/lib/pt_chown becomes required to set the gid properly on slave
pty instances.

That can be trivially fixed by making each and every mount of devpts
their own independent filesystems.  Which can be done by always
forcing the existing newinstance option on.

To make the fix work one more thing has to be accomplished.  The
device node /dev/ptmx needs to associate itself with the instance
of the devpts filesystem currently mounted at /dev/pts.

The bulk of this patch adds path walking code to the implementation
of /dev/ptmx so that it finds the devpts filesystem to create a
pty on by looking at the pts entry in the device nodes parent
directory.

The path walker is called kern_path_pts and is stored in fs/namei.c so
the are no weird vfs exports are necessary.  The path walker is also
special in that it performs no permission checks for the path walk.

This patch addresses one additional practical problem with the opening
of /dev/ptmx.  How to find which instance of the devpts filesystem
userspace is dealing with.  The opening of /dev/ptmx now ensures that
fstat on the file descriptor returns st_dev of devpts filesystem on
which the slave pty resides, and that readlink /proc/self/NNN returns
the path to ptmx on the devpts filesystem.

Forcing newinstance for every mount of the devpts filesystem actually
requires the association between /dev/ptmx and the currently mounted
instance of devpts at /dev/pts.  Simply remembering the first mount of
the devpts filesystem and associating that with /dev/ptmx is not
enough.  I am aware of at least one instance where an initramfs mounts
devpts before the main system instance of devpts is mounted.  In that
system ptys simply did not work after boot when I tested associating
/dev/ptmx with the first mount of the devpts filesystem.

Similary replacing the /dev/ptmx node in devtmpfs with a symlink to
/dev/pts/ptmx is not sufficient as there are some versions of udev
that look at sysfs see that a device node is supposed to be at
/dev/ptmx and replace the symlink with the device node.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---

Greg I assume this change should go through your tty tree?

 drivers/tty/pty.c         |  4 +++
 fs/devpts/inode.c         | 46 ++++++++++++++++++++++++++++++-
 fs/namei.c                | 69 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/devpts_fs.h |  1 +
 include/linux/namei.h     |  1 +
 5 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a45660f62db5..81ae0945cd53 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -738,6 +738,10 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	int retval;
 	int index;
 
+	inode = devpts_ptmx(inode, filp);
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
+
 	nonseekable_open(inode, filp);
 
 	/* We refuse fsnotify events on ptmx, since it's a shared resource */
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c35ffdc12bba..79e8d60ba0fe 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -136,6 +136,50 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
+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);
+	}
+
+	sb = path.mnt->mnt_sb;
+	if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
+		path_put(&path);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Advance path to the ptmx dentry */
+	root = path.dentry;
+	path.dentry = dget(DEVPTS_SB(sb)->ptmx_dentry);
+	dput(root);
+
+	/*
+	 * 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.
+	 */
+	inode = path.dentry->d_inode;
+	filp->f_path = path;
+	filp->f_inode = inode;
+	filp->f_mapping = inode->i_mapping;
+	path_put(&old);
+#endif
+	return inode;
+}
+
 static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 {
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
@@ -175,7 +219,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 
 	/* newinstance makes sense only on initial mount */
 	if (op == PARSE_MOUNT)
-		opts->newinstance = 0;
+		opts->newinstance = 1;
 
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
diff --git a/fs/namei.c b/fs/namei.c
index d84d7c7515fc..bd19db26a898 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2515,6 +2515,75 @@ kern_path_mountpoint(int dfd, const char *name, struct path *path,
 }
 EXPORT_SYMBOL(kern_path_mountpoint);
 
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+int kern_path_pts(struct path *path)
+{
+	/* This is a path walk to ${path}/../pts without permission checks */
+	struct path root;
+	struct dentry *parent, *pts;
+	struct qstr this;
+	int err;
+
+	get_fs_root(current->fs, &root);
+
+	/* Find the parent of the /dev/ptmx device node.
+	 * This is a variation of follow_dotdot.
+	 */
+	err = -ENOENT;
+	while (1) {
+		struct dentry *old = path->dentry;
+
+		if ((old == root.dentry) &&
+		    (path->mnt == root.mnt))
+			goto fail;
+
+		if (old != path->mnt->mnt_root) {
+			path->dentry = dget_parent(old);
+			dput(old);
+			if (unlikely(!path_connected(path)))
+				goto fail;
+			break;
+		}
+		if (!follow_up(path))
+			goto fail;
+	}
+	follow_mount(path);
+
+	/* In the parent directory find the cached pts dentry.  The
+	 * dentry must be cached if it is a mountpoint for the devpts
+	 * filesystem.
+	 */
+	parent = path->dentry;
+	this.name = "pts";
+	this.len = 3;
+	this.hash = full_name_hash(this.name, this.len);
+	if (parent->d_flags & DCACHE_OP_HASH) {
+		err = parent->d_op->d_hash(parent, &this);
+		if (err < 0)
+			goto fail;
+	}
+
+	err = -ENOENT;
+	mutex_lock(&parent->d_inode->i_mutex);
+	pts = d_lookup(parent, &this);
+	mutex_unlock(&parent->d_inode->i_mutex);
+	if (!pts)
+		goto fail;
+
+	/* Find what is mounted on pts */
+	path->dentry = pts;
+	dput(parent);
+	follow_mount(path);
+
+	path_put(&root);
+	return 0;
+
+fail:
+	path_put(&root);
+	return err;
+}
+#endif
+
 int __check_sticky(struct inode *dir, struct inode *inode)
 {
 	kuid_t fsuid = current_fsuid();
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 251a2090a554..8834dba07ff9 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -17,6 +17,7 @@
 
 #ifdef CONFIG_UNIX98_PTYS
 
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp);
 int devpts_new_index(struct inode *ptmx_inode);
 void devpts_kill_index(struct inode *ptmx_inode, int idx);
 /* mknod in devpts */
diff --git a/include/linux/namei.h b/include/linux/namei.h
index d8c6334cd150..cfac431bcd31 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -75,6 +75,7 @@ extern struct dentry *user_path_create(int, const char __user *, struct path *,
 extern void done_path_create(struct path *, struct dentry *);
 extern struct dentry *kern_path_locked(const char *, struct path *);
 extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int);
+extern int kern_path_pts(struct path *path);
 
 extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
 
-- 
2.6.3

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

[toc] | [next] | [standalone]

#1289897

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.

> +       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.

> +       /*
> +        * 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..

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

[toc] | [prev] | [next] | [standalone]

#1289906

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

[toc] | [prev] | [next] | [standalone]

#1289899

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.

I'm less than thrilled about the whole approach as it is, but this definitely
pushes it into the NAK-on-sight territory.  We'd been through that 3 years
ago when you brought that kind of hacks up last time; the objections still
stand.

Don't make it any more of a special snowflake than it absolutely has to be.
We already have more than enough potential headache sources to keep track
of when doing any kind of work on core kernel and extra ones are not
appreciated.  Really.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289924

Al Viro <viro@ZenIV.linux.org.uk> 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.

If it would help I am happy to add a helper to change the filp path that
lives next to d_dentry_open so that this is not insane to maintain.  I
am not ready to consider yet another attempt by kernel people to solve
userspace problems that is a half thought out mess that means we will
still have bugs and problems 20 years hence.

We need to actually solve the problem and anything less than that is
just stupid.

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

[toc] | [prev] | [next] | [standalone]

#1289933

On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Al Viro <viro@ZenIV.linux.org.uk> 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?"

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

[toc] | [prev] | [next] | [standalone]

#1289950

On 12/11/15 14:12, Andy Lutomirski wrote:
>>
>> 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.
> 

Sure.  My thinking, though, was whether or not we can do something that
works on legacy kernels, and/or is less intrusive than new ioctls.

What is the actual operation we need?

	-hpa

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

[toc] | [prev] | [next] | [standalone]

#1289955

On Fri, Dec 11, 2015 at 2:18 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/11/15 14:12, Andy Lutomirski wrote:
>>>
>>> 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.
>>
>
> Sure.  My thinking, though, was whether or not we can do something that
> works on legacy kernels, and/or is less intrusive than new ioctls.
>
> What is the actual operation we need?

To do the whole shebang at once:

ioctl(ptmx_fd, TIOCWHATEVER, fd_to_devpts_mount);

returns the slave number if fd_to_devpts_mount points to the right
place or an error if not.

ptsname(fd) logically does:

fd_to_devpts_mount = open("/dev/pts", O_RDONLY | O_DIRECTORY);
int n = ioctl(fd, TIOCWHATEVER, fd_to_devpts_mount);
close(fd_to_devpts_mount);
if (n < 0)
  return some error;
return "/dev/pts/" + n;

I think that all kinds of variants are possible.

--Andy

>
>         -hpa
>



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

[toc] | [prev] | [next] | [standalone]

#1289971

On 12/11/15 14:24, Andy Lutomirski wrote:
> 
> To do the whole shebang at once:
> 
> ioctl(ptmx_fd, TIOCWHATEVER, fd_to_devpts_mount);
> 
> returns the slave number if fd_to_devpts_mount points to the right
> place or an error if not.
> 
> ptsname(fd) logically does:
> 
> fd_to_devpts_mount = open("/dev/pts", O_RDONLY | O_DIRECTORY);
> int n = ioctl(fd, TIOCWHATEVER, fd_to_devpts_mount);
> close(fd_to_devpts_mount);
> if (n < 0)
>   return some error;
> return "/dev/pts/" + n;
> 
> I think that all kinds of variants are possible.
> 

If we're going to invent new names, any reason to not simply have
TIOCPTSNAME, since we can find the pts inode from the ptm fd, and then
walk the dentry tree (returning error if unreachable)?

Or does that open up entirely new issues?

	-hpa


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

[toc] | [prev] | [next] | [standalone]

#1289953

On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/11/15 13:48, Andy Lutomirski wrote:
>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>> Al Viro <viro@ZenIV.linux.org.uk> 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.

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

[toc] | [prev] | [next] | [standalone]

#1289976

Andy Lutomirski <luto@amacapital.net> writes:

> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>> Al Viro <viro@ZenIV.linux.org.uk> 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.

I even posted an implementation earlier in the discussion and no one was
interested.

Honestly the more weird special cases we add to devpts the less likely
userspace will be to get things right.  We have been trying since 1998
and devpts is still a poor enough design we have not been able to get
rid of /usr/lib/pt_chown.  Adding another case where we have to sand on
one foot and touch our nose does not seem to likely to achieve
widespread adoption.  How many version of libc are there now?

So I think the following incremental patch makes sense to improve the
maintainability of what I have written, but I haven't seen any arguments
that it is actually a bad idea.

Especially given that ptys are a core part of unix and they are used by
everyone all of the time.

Eric

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 79e8d60ba0fe..588e0a049daf 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -139,15 +139,14 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
 {
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
-	struct path path, old;
+	struct path path;
 	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 = filp->f_path;
 	path_get(&path);
 	if (kern_path_pts(&path)) {
 		path_put(&path);
@@ -172,10 +171,8 @@ struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
 	 * path to the devpts filesystem for reporting slave inodes.
 	 */
 	inode = path.dentry->d_inode;
-	filp->f_path = path;
-	filp->f_inode = inode;
-	filp->f_mapping = inode->i_mapping;
-	path_put(&old);
+	filp_set_path(filp, &path);
+	path_put(&path);
 #endif
 	return inode;
 }
diff --git a/fs/open.c b/fs/open.c
index b6f1e96a7c0b..5234a791d9ae 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -679,6 +679,19 @@ int open_check_o_direct(struct file *f)
 	return 0;
 }
 
+void filp_set_path(struct file *filp, struct path *path)
+{
+	/* Only safe during open */
+	struct path old = filp->f_path;
+	struct inode *inode = path->dentry->d_inode;
+
+	path_get(path);
+	filp->f_path = *path;
+	filp->f_inode = inode;
+	filp->f_mapping = inode->i_mapping;
+	path_put(&old);
+}
+
 static int do_dentry_open(struct file *f,
 			  struct inode *inode,
 			  int (*open)(struct inode *, struct file *),
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3aa514254161..f3659a8a2eec 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2220,6 +2220,7 @@ extern struct file *file_open_root(struct dentry *, struct vfsmount *,
 				   const char *, int);
 extern struct file * dentry_open(const struct path *, int, const struct cred *);
 extern int filp_close(struct file *, fl_owner_t id);
+extern void filp_set_path(struct file *filp, struct path *path);
 
 extern struct filename *getname_flags(const char __user *, int, int *);
 extern struct filename *getname(const char __user *);

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

[toc] | [prev] | [next] | [standalone]

#1289985

[Multipart message — attachments visible in raw view] — view raw

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@xmission.com> wrote:
> > Andy Lutomirski <luto@amacapital.net> writes:
> >
> >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> >>> On 12/11/15 13:48, Andy Lutomirski wrote:
> >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
> >>>> <ebiederm@xmission.com> wrote:
> >>>>> Al Viro <viro@ZenIV.linux.org.uk> 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?

[toc] | [prev] | [next] | [standalone]

#1289994

On December 11, 2015 3:00:49 PM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>On Fri, Dec 11, 2015 at 2:58 PM, Jann Horn <jann@thejh.net> 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@xmission.com> wrote:
>>> > Andy Lutomirski <luto@amacapital.net> writes:
>>> >
>>> >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com>
>wrote:
>>> >>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>> >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>> >>>> <ebiederm@xmission.com> wrote:
>>> >>>>> Al Viro <viro@ZenIV.linux.org.uk> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290006

On Fri, Dec 11, 2015 at 3:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On December 11, 2015 3:00:49 PM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>>On Fri, Dec 11, 2015 at 2:58 PM, Jann Horn <jann@thejh.net> 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@xmission.com> wrote:
>>>> > Andy Lutomirski <luto@amacapital.net> writes:
>>>> >
>>>> >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com>
>>wrote:
>>>> >>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>>> >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>>> >>>> <ebiederm@xmission.com> wrote:
>>>> >>>>> Al Viro <viro@ZenIV.linux.org.uk> 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

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

[toc] | [prev] | [next] | [standalone]

#1290014

On December 11, 2015 3:16:48 PM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>On Fri, Dec 11, 2015 at 3:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On December 11, 2015 3:00:49 PM PST, Andy Lutomirski
><luto@amacapital.net> wrote:
>>>On Fri, Dec 11, 2015 at 2:58 PM, Jann Horn <jann@thejh.net> 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@xmission.com> wrote:
>>>>> > Andy Lutomirski <luto@amacapital.net> writes:
>>>>> >
>>>>> >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com>
>>>wrote:
>>>>> >>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>>>> >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>>>> >>>> <ebiederm@xmission.com> wrote:
>>>>> >>>>> Al Viro <viro@ZenIV.linux.org.uk> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290000

On Fri, Dec 11, 2015 at 2:58 PM, Jann Horn <jann@thejh.net> 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@xmission.com> wrote:
>> > Andy Lutomirski <luto@amacapital.net> writes:
>> >
>> >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> >>> On 12/11/15 13:48, Andy Lutomirski wrote:
>> >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>> >>>> <ebiederm@xmission.com> wrote:
>> >>>>> Al Viro <viro@ZenIV.linux.org.uk> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289986

On December 11, 2015 2:35:16 PM PST, ebiederm@xmission.com wrote:
>Andy Lutomirski <luto@amacapital.net> writes:
>
>> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com>
>wrote:
>>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>>> <ebiederm@xmission.com> wrote:
>>>>> Al Viro <viro@ZenIV.linux.org.uk> 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.
>
>I even posted an implementation earlier in the discussion and no one
>was
>interested.
>
>Honestly the more weird special cases we add to devpts the less likely
>userspace will be to get things right.  We have been trying since 1998
>and devpts is still a poor enough design we have not been able to get
>rid of /usr/lib/pt_chown.  Adding another case where we have to sand on
>one foot and touch our nose does not seem to likely to achieve
>widespread adoption.  How many version of libc are there now?
>
>So I think the following incremental patch makes sense to improve the
>maintainability of what I have written, but I haven't seen any
>arguments
>that it is actually a bad idea.
>
>Especially given that ptys are a core part of unix and they are used by
>everyone all of the time.
>
>Eric
>
>diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
>index 79e8d60ba0fe..588e0a049daf 100644
>--- a/fs/devpts/inode.c
>+++ b/fs/devpts/inode.c
>@@ -139,15 +139,14 @@ static inline struct pts_fs_info
>*DEVPTS_SB(struct super_block *sb)
> struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
> {
> #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
>-	struct path path, old;
>+	struct path path;
> 	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 = filp->f_path;
> 	path_get(&path);
> 	if (kern_path_pts(&path)) {
> 		path_put(&path);
>@@ -172,10 +171,8 @@ struct inode *devpts_ptmx(struct inode *inode,
>struct file *filp)
> 	 * path to the devpts filesystem for reporting slave inodes.
> 	 */
> 	inode = path.dentry->d_inode;
>-	filp->f_path = path;
>-	filp->f_inode = inode;
>-	filp->f_mapping = inode->i_mapping;
>-	path_put(&old);
>+	filp_set_path(filp, &path);
>+	path_put(&path);
> #endif
> 	return inode;
> }
>diff --git a/fs/open.c b/fs/open.c
>index b6f1e96a7c0b..5234a791d9ae 100644
>--- a/fs/open.c
>+++ b/fs/open.c
>@@ -679,6 +679,19 @@ int open_check_o_direct(struct file *f)
> 	return 0;
> }
> 
>+void filp_set_path(struct file *filp, struct path *path)
>+{
>+	/* Only safe during open */
>+	struct path old = filp->f_path;
>+	struct inode *inode = path->dentry->d_inode;
>+
>+	path_get(path);
>+	filp->f_path = *path;
>+	filp->f_inode = inode;
>+	filp->f_mapping = inode->i_mapping;
>+	path_put(&old);
>+}
>+
> static int do_dentry_open(struct file *f,
> 			  struct inode *inode,
> 			  int (*open)(struct inode *, struct file *),
>diff --git a/include/linux/fs.h b/include/linux/fs.h
>index 3aa514254161..f3659a8a2eec 100644
>--- a/include/linux/fs.h
>+++ b/include/linux/fs.h
>@@ -2220,6 +2220,7 @@ extern struct file *file_open_root(struct dentry
>*, struct vfsmount *,
> 				   const char *, int);
>extern struct file * dentry_open(const struct path *, int, const struct
>cred *);
> extern int filp_close(struct file *, fl_owner_t id);
>+extern void filp_set_path(struct file *filp, struct path *path);
> 
>extern struct filename *getname_flags(const char __user *, int, int *);
> extern struct filename *getname(const char __user *);

I'm calling bullshit on that.  pt_chown is not and has not been needed on anything but severely misconfigured userspace since devpts was constructed.  The problem, rather, is that by not disabling pt_chown at the same time they switched to devpts distros allowed these severe misconfigurations to go on unnoticed.

So pt_chown *created* the problem.

The other problem we are trying to deal with is that the layout is suboptimal for the multi instance case, a legacy from an older SysV  layout with /dev/ptm/# and /dev/pts/#, the former replaced with the multiplex device /dev/ptmx, but that just shows the sheer amount of inertia we are dealing with.
-- 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289991

On Fri, Dec 11, 2015 at 2:35 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>>> <ebiederm@xmission.com> wrote:
>>>>> Al Viro <viro@ZenIV.linux.org.uk> 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.

>
> I even posted an implementation earlier in the discussion and no one was
> interested.
>
> Honestly the more weird special cases we add to devpts the less likely
> userspace will be to get things right.  We have been trying since 1998
> and devpts is still a poor enough design we have not been able to get
> rid of /usr/lib/pt_chown.  Adding another case where we have to sand on
> one foot and touch our nose does not seem to likely to achieve
> widespread adoption.  How many version of libc are there now?

Old libc can stay buggy, I think.  Given that this mess is partially a
libc mis-design, I don't see why it needs to be fixed entirely in the
kernel.

For new systems, it would be really nice if we can make a /dev/ptmx
symlink be 100% functional.

In any event, this is semi-moot.  If we actually want to retire the
newinstance=0 thing from the kernel, we apparently need a magic
/dev/ptmx node.  That doesn't mean that new userspace needs to *use*
that magic node.  So why not implement the magic node without fiddling
with f_path?

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

[toc] | [prev] | [next] | [standalone]

#1289954

On 12/11/15 13:48, Andy Lutomirski wrote:
> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Al Viro <viro@ZenIV.linux.org.uk> 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.

	-hpa

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

[toc] | [prev] | [next] | [standalone]

#1291508

On 12/11/2015 11:40 AM, Eric W. Biederman wrote:
> Forcing newinstance for every mount of the devpts filesystem actually
> requires the association between /dev/ptmx and the currently mounted
> instance of devpts at /dev/pts.  Simply remembering the first mount of
> the devpts filesystem and associating that with /dev/ptmx is not
> enough.  I am aware of at least one instance where an initramfs mounts
> devpts before the main system instance of devpts is mounted.

Can you point me to that usage please?

I ask because there's a patch to move devpts init from module initcall
up to fs initcall (neither devpts nor the pty driver is actually built
as a module anyway), and I'd like to look at what the consequences
might be for that userspace configuration.


> In that system ptys simply did not work after boot when I tested
> associating /dev/ptmx with the first mount of the devpts filesystem.

Assuming userspace isn't broken by that patch, is a fixed association
with first mount otherwise an acceptable solution for magic /dev/ptmx
(where /dev/ptmx is not a symlink to /dev/pts/ptmx)?

Regards,
Peter Hurley

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web