Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1713144 > unrolled thread
| Started by | Christian Brauner <christian.brauner@ubuntu.com> |
|---|---|
| First post | 2017-08-16 19:20 +0200 |
| Last post | 2017-08-17 03:40 +0200 |
| Articles | 14 on this page of 54 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@ubuntu.com> - 2017-08-16 19:20 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 20:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 20:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-16 21:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 22:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 22:20 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 22:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 23:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-16 23:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 23:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 00:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-17 00:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-17 00:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-23 17:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-23 23:20 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 00:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 01:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 02:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 02:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 03:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 02:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 02:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 03:20 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 03:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 03:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 03:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 04:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 05:20 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 05:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 18:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 06:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 18:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:20 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 22:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 22:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 20:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 21:00 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 21:30 +0200
[PATCH v3] pty: Repair TIOCGPTPEER ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 22:20 +0200
Re: [PATCH v3] pty: Repair TIOCGPTPEER Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 23:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 21:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 21:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 22:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 23:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-25 01:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-25 01:30 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-25 01:40 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-26 03:10 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 21:50 +0200
Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 03:40 +0200
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-24 21:00 +0200 |
| Message-ID | <ui5we-1Nn-21@gated-at.bofh.it> |
| In reply to | #1719506 |
On Thu, Aug 24, 2017 at 11:40 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Here is my tested version of the patch.
Can you please take my cleanups to devpts_ptmx_path() too?
Those 'goto err' statements are disgusting, when a plain 'return
-ERRNO' works cleaner.
And that "struct file *filp = NULL;" is bogus - you added the NULL
initialization because you mis-used "filp" early, and with that fixed
it's just garbage.
Other than that, it looks fine to me.
Linus
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-24 21:30 +0200 |
| Message-ID | <ui5Zg-2bY-19@gated-at.bofh.it> |
| In reply to | #1719510 |
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, Aug 24, 2017 at 11:40 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> Here is my tested version of the patch. > > Can you please take my cleanups to devpts_ptmx_path() too? Let met take a look. > Those 'goto err' statements are disgusting, when a plain 'return > -ERRNO' works cleaner. Yes those look like good cleanups. I had tried to preserve the original logic in devpts_ptmx_path from devpts_acquire to make it easier to see if I had goofed. But that out and out failed so cleanups so the code is easier to read look like a very good thing. > And that "struct file *filp = NULL;" is bogus - you added the NULL > initialization because you mis-used "filp" early, and with that fixed > it's just garbage. Actually the NULL initialization is a hold over from the original version of that function. But I agree without it gcc could have caught my use of the wrong variable, so removing it looks like a good idea. > Other than that, it looks fine to me. Thanks. I will respin and retest and see where things are at. Eric
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-24 22:20 +0200 |
| Subject | [PATCH v3] pty: Repair TIOCGPTPEER |
| Message-ID | <ui6LF-2Jv-33@gated-at.bofh.it> |
| In reply to | #1719510 |
The implementation of TIOCGPTPEER has two issues.
When /dev/ptmx (as opposed to /dev/pts/ptmx) is opened the wrong
vfsmount is passed to dentry_open. Which results in the kernel displaying
the wrong pathname for the peer.
The second is simply by caching the vfsmount and dentry of the peer it leaves
them open, in a way they were not previously Which because of the inreased
reference counts can cause unnecessary behaviour differences resulting in
regressions.
To fix these move the ioctl into tty_io.c at a generic level allowing
the ioctl to have access to the struct file on which the ioctl is
being called. This allows the path of the slave to be derived when
opening the slave through TIOCGPTPEER instead of requiring the path to
the slave be cached. Thus removing the need for caching the path.
A new function devpts_ptmx_path is factored out of devpts_acquire and
used to implement a function devpts_mntget. The new function devpts_mntget
takes a filp to perform the lookup on and fsi so that it can confirm
that the superblock that is found by devpts_ptmx_path is the proper superblock.
v2: Lots of fixes to make the code actually work
v3: Suggestions by Linus
- Removed the unnecessary initialization of filp in ptm_open_peer
- Simplified devpts_ptmx_path as gotos are no longer required
Fixes: 54ebbfb16034 ("tty: add TIOCGPTPEER ioctl")
Reported-by: Christian Brauner <christian.brauner@canonical.com>
Reported-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
drivers/tty/pty.c | 64 ++++++++++++++++++++--------------------------
drivers/tty/tty_io.c | 3 +++
fs/devpts/inode.c | 65 +++++++++++++++++++++++++++++++++++------------
include/linux/devpts_fs.h | 10 ++++++++
4 files changed, 89 insertions(+), 53 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 284749fb0f6b..a6d5164c33a9 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -69,13 +69,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
#ifdef CONFIG_UNIX98_PTYS
if (tty->driver == ptm_driver) {
mutex_lock(&devpts_mutex);
- if (tty->link->driver_data) {
- struct path *path = tty->link->driver_data;
-
- devpts_pty_kill(path->dentry);
- path_put(path);
- kfree(path);
- }
+ if (tty->link->driver_data)
+ devpts_pty_kill(tty->link->driver_data);
mutex_unlock(&devpts_mutex);
}
#endif
@@ -607,25 +602,24 @@ static inline void legacy_pty_init(void) { }
static struct cdev ptmx_cdev;
/**
- * pty_open_peer - open the peer of a pty
- * @tty: the peer of the pty being opened
+ * ptm_open_peer - open the peer of a pty
+ * @master: the open struct file of the ptmx device node
+ * @tty: the master of the pty being opened
+ * @flags: the flags for open
*
- * Open the cached dentry in tty->link, providing a safe way for userspace
- * to get the slave end of a pty (where they have the master fd and cannot
- * access or trust the mount namespace /dev/pts was mounted inside).
+ * Provide a race free way for userspace to open the slave end of a pty
+ * (where they have the master fd and cannot access or trust the mount
+ * namespace /dev/pts was mounted inside).
*/
-static struct file *pty_open_peer(struct tty_struct *tty, int flags)
-{
- if (tty->driver->subtype != PTY_TYPE_MASTER)
- return ERR_PTR(-EIO);
- return dentry_open(tty->link->driver_data, flags, current_cred());
-}
-
-static int pty_get_peer(struct tty_struct *tty, int flags)
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
{
int fd = -1;
- struct file *filp = NULL;
+ struct file *filp;
int retval = -EINVAL;
+ struct path path;
+
+ if (tty->driver != ptm_driver)
+ return -EIO;
fd = get_unused_fd_flags(0);
if (fd < 0) {
@@ -633,7 +627,16 @@ static int pty_get_peer(struct tty_struct *tty, int flags)
goto err;
}
- filp = pty_open_peer(tty, flags);
+ /* Compute the slave's path */
+ path.mnt = devpts_mntget(master, tty->driver_data);
+ if (IS_ERR(path.mnt)) {
+ retval = PTR_ERR(path.mnt);
+ goto err_put;
+ }
+ path.dentry = tty->link->driver_data;
+
+ filp = dentry_open(&path, flags, current_cred());
+ mntput(path.mnt);
if (IS_ERR(filp)) {
retval = PTR_ERR(filp);
goto err_put;
@@ -662,8 +665,6 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
return pty_get_pktmode(tty, (int __user *)arg);
case TIOCGPTN: /* Get PT Number */
return put_user(tty->index, (unsigned int __user *)arg);
- case TIOCGPTPEER: /* Open the other end */
- return pty_get_peer(tty, (int) arg);
case TIOCSIG: /* Send signal to other side of pty */
return pty_signal(tty, (int) arg);
}
@@ -791,7 +792,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
{
struct pts_fs_info *fsi;
struct tty_struct *tty;
- struct path *pts_path;
struct dentry *dentry;
int retval;
int index;
@@ -845,26 +845,16 @@ static int ptmx_open(struct inode *inode, struct file *filp)
retval = PTR_ERR(dentry);
goto err_release;
}
- /* We need to cache a fake path for TIOCGPTPEER. */
- pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
- if (!pts_path)
- goto err_release;
- pts_path->mnt = filp->f_path.mnt;
- pts_path->dentry = dentry;
- path_get(pts_path);
- tty->link->driver_data = pts_path;
+ tty->link->driver_data = dentry;
retval = ptm_driver->ops->open(tty, filp);
if (retval)
- goto err_path_put;
+ goto err_release;
tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
tty_unlock(tty);
return 0;
-err_path_put:
- path_put(pts_path);
- kfree(pts_path);
err_release:
tty_unlock(tty);
// This will also put-ref the fsi
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 974b13d24401..10c4038c0e8d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2518,6 +2518,9 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
case TIOCSSERIAL:
tty_warn_deprecated_flags(p);
break;
+ case TIOCGPTPEER:
+ /* Special because the struct file is needed */
+ return ptm_open_peer(file, tty, (int)arg);
default:
retval = tty_jobctrl_ioctl(tty, real_tty, file, cmd, arg);
if (retval != -ENOIOCTLCMD)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 108df2e3602c..7eae33ffa3fc 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -133,6 +133,50 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
return sb->s_fs_info;
}
+static int devpts_ptmx_path(struct path *path)
+{
+ struct super_block *sb;
+ int err;
+
+ /* Has the devpts filesystem already been found? */
+ if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
+ return 0;
+
+ /* Is a devpts filesystem at "pts" in the same directory? */
+ err = path_pts(path);
+ if (err)
+ return err;
+
+ /* Is the path the root of a devpts filesystem? */
+ sb = path->mnt->mnt_sb;
+ if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
+ (path->mnt->mnt_root != sb->s_root))
+ return -ENODEV;
+
+ return 0;
+}
+
+struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
+{
+ struct path path;
+ int err;
+
+ path = filp->f_path;
+ path_get(&path);
+
+ err = devpts_ptmx_path(&path);
+ dput(path.dentry);
+ if (err) {
+ mntput(path.mnt);
+ path.mnt = ERR_PTR(err);
+ }
+ if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
+ mntput(path.mnt);
+ path.mnt = ERR_PTR(-ENODEV);
+ }
+ return path.mnt;
+}
+
struct pts_fs_info *devpts_acquire(struct file *filp)
{
struct pts_fs_info *result;
@@ -143,27 +187,16 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
path = filp->f_path;
path_get(&path);
- /* Has the devpts filesystem already been found? */
- sb = path.mnt->mnt_sb;
- if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
- /* Is a devpts filesystem at "pts" in the same directory? */
- err = path_pts(&path);
- if (err) {
- result = ERR_PTR(err);
- goto out;
- }
-
- /* Is the path the root of a devpts filesystem? */
- result = ERR_PTR(-ENODEV);
- sb = path.mnt->mnt_sb;
- if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
- (path.mnt->mnt_root != sb->s_root))
- goto out;
+ err = devpts_ptmx_path(&path);
+ if (err) {
+ result = ERR_PTR(err);
+ goto out;
}
/*
* pty code needs to hold extra references in case of last /dev/tty close
*/
+ sb = path.mnt->mnt_sb;
atomic_inc(&sb->s_active);
result = DEVPTS_SB(sb);
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 277ab9af9ac2..100cb4343763 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -19,6 +19,7 @@
struct pts_fs_info;
+struct vfsmount *devpts_mntget(struct file *, struct pts_fs_info *);
struct pts_fs_info *devpts_acquire(struct file *);
void devpts_release(struct pts_fs_info *);
@@ -32,6 +33,15 @@ void *devpts_get_priv(struct dentry *);
/* unlink */
void devpts_pty_kill(struct dentry *);
+/* in pty.c */
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags);
+
+#else
+static inline int
+ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
+{
+ return -EIO;
+}
#endif
--
2.14.1
[toc] | [prev] | [next] | [standalone]
| From | Stefan Lippers-Hollmann <s.l-h@gmx.de> |
|---|---|
| Date | 2017-08-24 23:10 +0200 |
| Subject | Re: [PATCH v3] pty: Repair TIOCGPTPEER |
| Message-ID | <ui7y2-3iu-31@gated-at.bofh.it> |
| In reply to | #1719540 |
[Multipart message — attachments visible in raw view] — view raw
Hi On 2017-08-24, Eric W. Biederman wrote: > The implementation of TIOCGPTPEER has two issues. > > When /dev/ptmx (as opposed to /dev/pts/ptmx) is opened the wrong > vfsmount is passed to dentry_open. Which results in the kernel displaying > the wrong pathname for the peer. [...] > v2: Lots of fixes to make the code actually work > v3: Suggestions by Linus > - Removed the unnecessary initialization of filp in ptm_open_peer > - Simplified devpts_ptmx_path as gotos are no longer required This version of the patch is working for me as well in all my test (including pbuilder) so far, thanks a lot. Regards Stefan Lippers-Hollmann
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-24 21:30 +0200 |
| Message-ID | <ui5Zg-2bY-25@gated-at.bofh.it> |
| In reply to | #1719506 |
On Thu, Aug 24, 2017 at 12:01 PM, Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> I've touched on this in my original message, I wonder whether we currently
> support mounting devpts at a different a location and expect an open on a
> newly created slave to work.
Yes. That is very much intended to work.
> Say I mount devpts at /mnt and to open("/mnt/ptmx", O_RDWR | O_NOCTTY) and get a new slave pty at /mnt/1 do we
> expect open("/mnt/1, O_RDWR | O_NOCTTY) to work?
Yes.
Except you actually don't want to use "/mnt/ptmx". That ptmx node
inside the pts filesystem is garbage that we never actually used,
because the permissions aren't sane. It should probably be removed,
but because somebody *might* have used it, we have left it alone.
So what you're actually *supposed* to do is
- create a ptmx node and a pts directory in /mnt
- mount devpts on /mnt/pts
- use /mnt/ptmx to create new pty's, which should just look up that
pts mount directly.
And yes, the pathname should then be /mnt/pts/X for the slave side,
and /mnt/ptmx for the master.
In fact, I just tested that TIOCGPTPEER, including using your original
test-program (this is me as root in my home directory):
[root@i7 torvalds]# mkdir dummy
[root@i7 torvalds]# cd dummy/
[root@i7 dummy]# mknod ptmx c 5 2
[root@i7 dummy]# mkdir pts
[root@i7 dummy]# mount -t devpts devpts pts
[root@i7 dummy]# ../a.out
I point to "/home/torvalds/dummy/pts/0"
[root@i7 dummy]# umount pts/
[root@i7 dummy]# cd ..
[root@i7 torvalds]# rm -rf dummy
There's two things to note there:
- look at that "I point to" - it's not hardcoded to /dev/pts/X
- look at the pts number: each pts filesystem has its own private
numbers, so despite the fact that in another window I *also* have that
ptx/0:
[torvalds@i7 linux]$ tty
/dev/pts/0
that new devpts instance has its *own* pts/0, which is a
completely different pty.
this is one of those big cleanups we did with the pts filesystem some time ago.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-24 21:30 +0200 |
| Message-ID | <ui5Zh-2bY-31@gated-at.bofh.it> |
| In reply to | #1719522 |
On Thu, Aug 24, 2017 at 12:22 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> [root@i7 dummy]# ../a.out
> I point to "/home/torvalds/dummy/pts/0"
Note: that "a.out" binary is modified from your original code.
It's modified to correctly print out the readdir() results, but it's
also modified to open just "ptmx" for the above trial (so it doesn't
open /dev/ptmx, it's opening that ptmx node in the current directory,
which is why it then reports that
I point to "/home/torvalds/dummy/pts/0"
thing.
So it's not your *original* test-program, it's slightly tweaked for this test.
Linus
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-24 22:50 +0200 |
| Message-ID | <ui7eG-2Ut-25@gated-at.bofh.it> |
| In reply to | #1719522 |
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Thu, Aug 24, 2017 at 12:01 PM, Christian Brauner
> <christian.brauner@canonical.com> wrote:
>>
>> I've touched on this in my original message, I wonder whether we currently
>> support mounting devpts at a different a location and expect an open on a
>> newly created slave to work.
>
> Yes. That is very much intended to work.
>
>> Say I mount devpts at /mnt and to open("/mnt/ptmx", O_RDWR | O_NOCTTY) and get a new slave pty at /mnt/1 do we
>> expect open("/mnt/1, O_RDWR | O_NOCTTY) to work?
>
> Yes.
>
> Except you actually don't want to use "/mnt/ptmx". That ptmx node
> inside the pts filesystem is garbage that we never actually used,
> because the permissions aren't sane. It should probably be removed,
> but because somebody *might* have used it, we have left it alone.
The ptmx node on devpts is used.
Use of that device node is way more prevalent then crazy weird cases
that required us to make /dev/ptmx perform relative lookups. People
just set the ptmxmode= boot parameter when mounting devpts if they care.
Every use case I am aware of where people actually knew about multiple
instances of devpts used the ptmx node in the devpts filesystem.
If everyone used devtmpfs we could have fixed the permissions on the
ptmx node in devpts and made /dev/ptmx a symlink instead of a device
node. Saving lots of complexity.
Unfortunately there were crazy weird cases out there where people
created chroots or mounted devpts multiple times during boot that
defeated every strategy except making /dev/ptmx perform a relative
lookup for devpts.
The reasons I did not fix the permissions on the ptmx deivcd node was
that given the magnitude of the change needed to get to the sensible
behavior of every mount of devpts creating a new filesystem, any
unnecessary changes were just plain scary.
Further the kind of regression that would be introduced if we changed
the permissions would be a security hole if someone has some really
weird and crazy permissions on /dev/ptmx and does not use devtmpfs.
That said I could not find a distribution being that crazy and I had a
very good sample of them. So I expect we can fix the default permissions
on ptmx node of devpts and not have anyone notice or care.
I would encourage people who are doing new things to actually use the
ptmx node on devpts because there is less overhead and it is simpler.
There are just enough weird one off scripts like xen image builder (I
think that was the nasty test case that broke in debian) that I can't
imagine ever being able to responsibly remove the path based lookups in
/dev/ptmx. I do dream of it sometimes.
It might be worth fixing the default permissions on the devpts ptmx node
and updating glibc to try /dev/pts/ptmx first. That would shave off a
few cycles in opening ptys. If you add TIOCGPTPEER there are probably
enough cleanups and simplifications that it would be worth it just
for the code improvements.
With glibc fixed we could even dream of a day when /dev/ptmx could be
completely removed.
Eric
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-24 23:10 +0200 |
| Message-ID | <ui7y2-3iu-27@gated-at.bofh.it> |
| In reply to | #1719559 |
On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> There are just enough weird one off scripts like xen image builder (I
> think that was the nasty test case that broke in debian) that I can't
> imagine ever being able to responsibly remove the path based lookups in
> /dev/ptmx. I do dream of it sometimes.
Not going to happen.
The fact is, /dev/ptmx is the simply the standard location.
/dev/pts/ptmx simply is *not*.
So pretty much every single user that ever uses pty's will use
/dev/ptmx, it's just how it has always worked.
Trying to change it to anything else is just stupid. There's no
upside, there is only downsides - mainly the "we'll have to support
the standard way anyway, that newfangled way doesn't add anything".
Our "pts" lookup isn't expensive.
So quite frankly, we should discourage people from using the
non-standard place. It really has no real advantages, and it's simply
not worth it.
Linus
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-25 01:10 +0200 |
| Message-ID | <ui9qa-4Bp-7@gated-at.bofh.it> |
| In reply to | #1719572 |
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> There are just enough weird one off scripts like xen image builder (I >> think that was the nasty test case that broke in debian) that I can't >> imagine ever being able to responsibly remove the path based lookups in >> /dev/ptmx. I do dream of it sometimes. > > Not going to happen. Which is what I said. > The fact is, /dev/ptmx is the simply the standard location. > /dev/pts/ptmx simply is *not*. The standard is posix_openpt(). That is a syscall on the bsds. Opening something called ptmx at this point is a Linuxism. There are a lot of programs that are going to be calling posix_openpt() simply because /dev/ptmx can not be counted on to exist. > So pretty much every single user that ever uses pty's will use > /dev/ptmx, it's just how it has always worked. > > Trying to change it to anything else is just stupid. There's no > upside, there is only downsides - mainly the "we'll have to support > the standard way anyway, that newfangled way doesn't add anything". Except the new fangled way does add quite a bit. Not everyone who mounts devpts has permission to call mknod. So /dev/ptmx frequently winds up either being a bind mount or a symlink to /dev/pts/ptmx in containers. It is going to take a long time but device nodes like one of those filesystem features thare are very slowly on their way out. > Our "pts" lookup isn't expensive. > > So quite frankly, we should discourage people from using the > non-standard place. It really has no real advantages, and it's simply > not worth it. The "pts" lookup admitted isn't runtime expensive. I could propbably measure a cost but anyone who is creating ptys fast enough to care likely has other issues. The "pts" lookup does have some real maintenance costs as it takes someone with a pretty deep understanding of things to figure out what is going on. I hope things have finally been abstracted well enough, and the code is used heavily enough we don't have to worry about a regression there. I still worry. As for non-standard locations. Anything that isn't /dev/ptmx and /dev/pts/NNN simply won't work for anything isn't very specialized. At which point I don't think there is any reason to skip using the ptmx node on the devpts filesystem as you have already given up compatibility with everything else. But I agree it doesn't look worth it to change glibc to deal with an alternate location for /dev/ptmx. I see a huge point in changing glibc to use the new TIOCGPTPEER ioctl when available as that is really the functionality the glibc internals are after. Eric
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-25 01:30 +0200 |
| Message-ID | <ui9Jx-4Jw-13@gated-at.bofh.it> |
| In reply to | #1719634 |
On Thu, Aug 24, 2017 at 4:01 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>>
>>> There are just enough weird one off scripts like xen image builder (I
>>> think that was the nasty test case that broke in debian) that I can't
>>> imagine ever being able to responsibly remove the path based lookups in
>>> /dev/ptmx. I do dream of it sometimes.
>>
>> Not going to happen.
>
> Which is what I said.
Yes, but you then went on to say that we should encourage "/dev/pts/ptmx"
Which is BS.
It's not a standard location, and it doesn't have any advantages.
It was a bad idea and due to a bad implementation. We fixed it. Let it go.
>> The fact is, /dev/ptmx is the simply the standard location.
>> /dev/pts/ptmx simply is *not*.
>
> The standard is posix_openpt(). That is a syscall on the bsds.
> Opening something called ptmx at this point is a Linuxism.
Bzzt. Thank you for playing, but you're completely and utterly wrong.
Look around a bit more.
posix_openpt() may be what you *wish* the standard was, but no,
/dev/ptmx is not a linuxism.
Really. It's the SysV STREAMS standard location, and it is what Sysv
pty users _will_ use directly.
Linux didn't make up that name.
Solaris, HP-UX-11, other sysv code bases all use /dev/ptmx
The whole "posix_openpt()" thing came later, in an attempt to just
unify the BSD and Sysv models.
Just google for "streams pty" if you don't believe me.
So really. The only linuxism here is that stupid /dev/pts/ptmx.
> There are a lot of programs that are going to be calling posix_openpt()
> simply because /dev/ptmx can not be counted on to exist.
.. and there are probably even more programs that simply use
"/dev/ptmx". If you came from a sysv world, or if you just happened to
copy any of the hundreds of examples on the interenet, that's what you
would do.
Christ, just go to Wikipedia. And I quote:
'BSD PTYs have been rendered obsolete by Unix98 ptys whose naming
system does not limit the number of pseudo-terminals and access to
which occurs without danger of race conditions. /dev/ptmx is the
"pseudo-terminal master multiplexer". Opening it returns a file
descriptor of a master node and causes an associated slave node
/dev/pts/N to be created.[5]'
That's from
https://en.wikipedia.org/wiki/Pseudoterminal
so stop blathering garbage. The fact is, /dev/ptmx is the standard
location, and /dev/pts/ptmx is, and always has been, an abomination.
Now, if you want to be portable, "posix_openpt()" is indeed what you
should use, but that doesn't change the basic point.
There's a very real reason why people use "/dev/ptmx", and no, it's
not a linuxism.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Christian Brauner <christian.brauner@canonical.com> |
|---|---|
| Date | 2017-08-25 01:40 +0200 |
| Message-ID | <ui9Tc-4MH-23@gated-at.bofh.it> |
| In reply to | #1719634 |
On Thu, Aug 24, 2017 at 06:01:36PM -0500, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
> > <ebiederm@xmission.com> wrote:
> >>
> >> There are just enough weird one off scripts like xen image builder (I
> >> think that was the nasty test case that broke in debian) that I can't
> >> imagine ever being able to responsibly remove the path based lookups in
> >> /dev/ptmx. I do dream of it sometimes.
> >
> > Not going to happen.
>
> Which is what I said.
>
> > The fact is, /dev/ptmx is the simply the standard location.
> > /dev/pts/ptmx simply is *not*.
>
> The standard is posix_openpt(). That is a syscall on the bsds.
> Opening something called ptmx at this point is a Linuxism.
>
> There are a lot of programs that are going to be calling posix_openpt()
> simply because /dev/ptmx can not be counted on to exist.
>
> > So pretty much every single user that ever uses pty's will use
> > /dev/ptmx, it's just how it has always worked.
> >
> > Trying to change it to anything else is just stupid. There's no
> > upside, there is only downsides - mainly the "we'll have to support
> > the standard way anyway, that newfangled way doesn't add anything".
>
> Except the new fangled way does add quite a bit. Not everyone who
> mounts devpts has permission to call mknod. So /dev/ptmx frequently
> winds up either being a bind mount or a symlink to /dev/pts/ptmx in
> containers.
In fact, /dev/ptmx being a symlink or bind-mount is the *standard* in containers
even for non-user namespaced containers or containers that do not retain
CAP_MKNOD.
>
> It is going to take a long time but device nodes like one of those
> filesystem features thare are very slowly on their way out.
This related to the point above: The fact that we can mount a devpts at its
standard location but are unable to also have/create an additional device node
at the *standard location* is usually quite irritating for people who do not
know about this "legacy" behaviour. But yeah, it's probably going away but
that's going to be a long long time. I agree that userspace is the place to
slowly make the transition though. :)
>
> > Our "pts" lookup isn't expensive.
> >
> > So quite frankly, we should discourage people from using the
> > non-standard place. It really has no real advantages, and it's simply
> > not worth it.
>
> The "pts" lookup admitted isn't runtime expensive. I could propbably
> measure a cost but anyone who is creating ptys fast enough to care
> likely has other issues.
>
> The "pts" lookup does have some real maintenance costs as it takes
> someone with a pretty deep understanding of things to figure out what is
> going on. I hope things have finally been abstracted well enough, and
> the code is used heavily enough we don't have to worry about a
> regression there. I still worry.
>
> As for non-standard locations. Anything that isn't /dev/ptmx and
> /dev/pts/NNN simply won't work for anything isn't very specialized.
I was mainly asking about non-standard locations because I experienced weird
behaviour when trying to open("/mnt/<slave-idx", O_RDWR | O_NOCTTY). Mind you I
did all the steps that grantpt() + unlockpt() usually do purely file descriptor
based. But I think this was due to the faulty TIOCGPTPEER implemenation before
which should now be fixed.
> At which point I don't think there is any reason to skip using the ptmx
> node on the devpts filesystem as you have already given up compatibility
> with everything else.
>
> But I agree it doesn't look worth it to change glibc to deal with an
> alternate location for /dev/ptmx. I see a huge point in changing glibc
> to use the new TIOCGPTPEER ioctl when available as that is really the
> functionality the glibc internals are after.
That's a patch I've been looking into. But TIOCGPTPEER alone won't be enough. A
couple of other function such as grantpt() need to switch from path-based
operation to file descriptor based operations too (Something I tried to point
out in one of my previous mails.). The whole user-space api could do - imho -
with a redo. The kernel is doing the right thing and exposing the right bits
mostly; TIOCGPTPEER being a good step. But user-space wise it's actually a
little security nightmare as soon as namespaces and - sorry for the buzzword -
*containers* come into play. @Eric, are you going to be at Plumbers again this
year? That's maybe a good chance to discuss some of this if there's still
interest.
Christian
>
> Eric
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-26 03:10 +0200 |
| Message-ID | <uixLQ-30m-9@gated-at.bofh.it> |
| In reply to | #1719650 |
On Thu, Aug 24, 2017 at 4:37 PM, Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> In fact, /dev/ptmx being a symlink or bind-mount is the *standard* in containers
> even for non-user namespaced containers or containers that do not retain
> CAP_MKNOD.
Yes.
I think using /dev/pts/ptmx is nice from a kernel standpoint, but I
really think that user space should *never* use it.
The distro or container setup can do whatever it wants to made
/dev/ptmx then point into the pts directory. Either the traditional
device node, the symlink, or the bind mount works fine. But the point
is that glibc definitely should *not* point to /dev/pts/ptmx itself,
because it's simply not the right path. On lots of distributions that
path simply will not work.
And yes, I agree that the user interface to this all is particularly
nasty. With TIOCGPTPEER we have a nice way to get the pts file
descriptor, but the "normal" way to get to it involves opening a path
given by ptsname(), so we en dup in the crazy situation that we can
easily open the file without the path, but then we use the fd to get
the path (that we didn't need) and then people open it with that path,
because the standard sequence to get a pts is
master = getpt() / posix_openpt() / open("/dev/ptmx", O_RDWR | O_NOCTTY);
grantpt(master);
unlockpt(master);
name = ptsname(master);
slave = open(name, O_RDWR);
which is kind of silly. And I'm not talking about the three different
ways to open the master side. I'm talking about all the rest, which is
all just pretty much garbage.
But I guess none of this is really performance-critical.
Linus
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-24 21:50 +0200 |
| Message-ID | <ui6iC-2kS-17@gated-at.bofh.it> |
| In reply to | #1719506 |
Christian Brauner <christian.brauner@canonical.com> writes:
> On Aug 24, 2017 20:41, "Eric W. Biederman" <ebiederm@xmission.com> wrote:
>
> > The implementation of TIOCGPTPEER has two issues.
> >
> > When /dev/ptmx (as opposed to
>
> I've touched on this in my original message, I wonder whether we
> currently support mounting devpts at a different a location and expect
> an open on a newly created slave to work. Say I mount devpts at /mnt
> and to open("/mnt/ptmx", O_RDWR | O_NOCTTY) and get a new slave pty at
> /mnt/1 do we expect open("/mnt/1, O_RDWR | O_NOCTTY) to work?
Yes.
In particular one of my crazy test cases when I did the last round
of cleanups to devpts was someone had created a chroot including
a /dev/ptmx device node and mounted devpts at the appropriate path
inside the chroot. Which is in part why /dev/ptmx does a relative
lookup of the devpts filesystem.
Now glibc won't work with devpts mounted somewhere else. As it has
/dev/pts/... hardcoded. But the kernel should work fine. The case you
described using /mnt/ptmx instead of a random /dev/ptmx device now
should work especially well as none of this crazy relative path lookup
work needs to happen.
There are little things such as TIOCSPTLCK and perhaps chmod that need
to be called in your example before the slave open will succeed (without
O_PATH) but yes that case most definitely should work.
Eric
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-17 03:40 +0200 |
| Message-ID | <ufhWV-6gk-7@gated-at.bofh.it> |
| In reply to | #1713289 |
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Aug 16, 2017 at 12:56 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So the fact that we _don't_ get the right pathname for the pts entry
>> here means that something got screwed up in setting filp->f_path to
>> the right thing. We have all the code in place that _tries_ to do it,
>> but it clearly has a bug somewhere.
>
> Ok, I think I see what the bug is, although I don't have a fix for it yet.
>
> We generate the path largely correctly: the path has a nice dentry
> that contains the right pts number, and has the right parent pointer
> that points to the root of the pts mount.
>
> And we also fill in the path 'mnt' field. Everything should be fine.
>
> Except when we actually hit that root dentry of the pts mount, the
> code in prepend_path() hits this condition:
>
> if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
> struct mount *parent = ACCESS_ONCE(mnt->mnt_parent);
> /* Escaped? */
> if (dentry != vfsmnt->mnt_root) {
>
> and we break out, and reset the path to '/' because we think it
> somehow escaped out of the user namespace.
Escaped it's bind mount actually. There should always be a path
from mnt_root to a dentry under that mount point. In some rare caseses
involving bind mounts a rename that moves a dentry from one directory to
another can result in dentries that are not reachable from mnt_root.
As those entries do not have a path in a meaningful sense setting the
path to '/' is the best we can do.
This condition should be limited to bind mounts as any dentry on a
filesystem is descendent from the filesystems root directory.
The rest of your analysis below is correct.
My apologies for the pendantic reply. I am repling just so that someone
doesn't find this in an email archive 20 years from now and become
impossibly confused.
> So it looks like we filled in the path with the *wrong* mount information.
>
> And THAT in turn is because we fill the path with the mount
> information for the "/dev/ptmx" field - which is *not* in the
> /dev/pts/ mount - that's the mount for '/dev'.
>
> So we have a dentry and a mnt, but they simply aren't paired up correctly.
>
> And you can see this with your test program: if you open /dev/pts/ptmx
> for the master, it actually works correctly (but you need to make sure
> the permissions for that ptmx node allow that).
>
> Anyway, I know what's wrong, next step is to figure out what the fix is.
>
> Linus
[toc] | [prev] | [standalone]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web