[RFC] fix devpts mount behavior

[RFC] fix devpts mount behavior

[Serge Hallyn]

Multiple devpts support currently behaves in a way userspace would likely
not expect.  You can mount a new devpts instance using '-o newinstance'.
However, if you subsequently mount devpts without '-o newinstance', you
will remount the global initial devpts instance.

As a result, if a container is created with a private devpts, but
has devpts in its fstab, then the container's init will end up mounting
the host's devpts.  This (a) confuses the software 'creating' the
container (which usually uses a pty for consoles), (b) confuses
userspace inside the container (especially if the ptmx from the
newinstance is bind-mounted over /dev/ptmx), and (c) is a security
risk to the host.

I've been playing for the last week with ways to fix this, and in the
end I can see two ways.  One is to pin+cache, on every newinstance devpts
mount, the new pts_fs_info at the current mount namespace.  Then if a task
does

	mount -t devpts -o newinstance d2 /mnt
	mount -t devpts devpts /dev/pts

then the new instance will be mounted at /dev/pts.

Dave Hansen commented that this basically turns the newinstance mount 
into an unshare, and that therefore perhaps we should fully support
a devpts namespace.  I actually like this better as it avoids making
awkward new relations between the mnt_namespace and pts_fs_info, and
leaves a comfy well-known 'unshare' operation between the current and
new behaviors.  However, it is behaviorally a bit awkward.  If a task
simply does

	mount -t devpts -o newinstance d2 /mnt
	mount -t devpts devpts /dev/pts

Then /dev/pts will still get the host's initial instance, but if it
does

	lxc-unshare -s MOUNT -- /bin/bash
	mount -t devpts -o newinstance d2 /mnt
	mount -t devpts devpts /dev/pts

then /dev/pts will have the new instance (in the new bash shell with
the unshared mount ns).

The other problem with this approach is that it consumes one of the
very few clone flags (we have 0x02000000 and maybe 0x00002000
available).

The patch below implements the first approach - though it's broken,
so only meant to show roughly how it might look.  If consensus is
that the second approach is best, I'll happily implement that and
gobble up 0x02000000.

Is there perhaps another option I haven't considered?

Am I wrong in even considering the current behavior bad?

BROKEN PATCH, DON'T RUN IT.

Signed-off-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

---
 fs/devpts/inode.c             |   48 +++++++++++++++++++++++++++++++++++++++-
 fs/namespace.c                |   15 ++++++++++++
 include/linux/devpts_fs.h     |    7 ++++++
 include/linux/fs.h            |    2 +
 include/linux/mnt_namespace.h |    2 +
 5 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index d5d5297..ef737af 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -25,6 +25,8 @@
 #include <linux/parser.h>
 #include <linux/fsnotify.h>
 #include <linux/seq_file.h>
+#include <linux/nsproxy.h>
+#include <linux/mnt_namespace.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
 /*
@@ -71,8 +73,32 @@ struct pts_fs_info {
 	struct ida allocated_ptys;
 	struct pts_mount_opts mount_opts;
 	struct dentry *ptmx_dentry;
+	struct kref kref;
 };
 
+struct pts_fs_info *get_devpts(struct pts_fs_info *i)
+{
+	if (i)
+		kref_get(&i->kref);
+	return i;
+}
+
+static void free_pts_fs_info(struct kref *kref)
+{
+	struct pts_fs_info *i = container_of(kref, struct pts_fs_info, kref);
+
+	printk(KERN_NOTICE "%s: freeing a pts_fs_info %lx\n", __func__,
+		(unsigned long) i);
+
+	kfree(i);
+}
+
+void put_devpts(struct pts_fs_info *i)
+{
+	if (i)
+		kref_put(&i->kref, free_pts_fs_info);
+}
+
 static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 {
 	return sb->s_fs_info;
@@ -280,6 +306,9 @@ static void *new_pts_fs_info(void)
 	ida_init(&fsi->allocated_ptys);
 	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
 	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
+	kref_init(&fsi->kref);
+	printk(KERN_NOTICE "%s: created a new pts_fs_info %lx\n", __func__,
+		(unsigned long) fsi);
 
 	return fsi;
 }
@@ -325,6 +354,16 @@ fail:
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 static int compare_init_pts_sb(struct super_block *s, void *p)
 {
+	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+
+	if (ns->devpts) {
+		int ret;
+		rcu_read_lock();
+		ret = ns->devpts == DEVPTS_SB(s);
+		rcu_read_unlock();
+		return ret;
+	}
+
 	if (devpts_mnt)
 		return devpts_mnt->mnt_sb == s;
 	return 0;
@@ -382,7 +421,8 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 		if (error)
 			goto out_undo_sget;
 		s->s_flags |= MS_ACTIVE;
-	}
+	} else
+		get_devpts(DEVPTS_SB(s));
 
 	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
 
@@ -390,6 +430,9 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	if (error)
 		goto out_undo_sget;
 
+	if (opts.newinstance)
+		mnt_set_devpts(DEVPTS_SB(s));
+
 	return dget(s->s_root);
 
 out_undo_sget:
@@ -398,6 +441,7 @@ out_undo_sget:
 }
 
 #else
+
 /*
  * This supports only the legacy single-instance semantics (no
  * multiple-instance semantics)
@@ -413,7 +457,7 @@ static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 
-	kfree(fsi);
+	put_devpts(fsi);
 	kill_litter_super(sb);
 }
 
diff --git a/fs/namespace.c b/fs/namespace.c
index aabfd30..953a972 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -31,6 +31,7 @@
 #include <linux/idr.h>
 #include <linux/fs_struct.h>
 #include <linux/fsnotify.h>
+#include <linux/devpts_fs.h>
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
 #include "pnode.h"
@@ -2458,6 +2459,7 @@ static struct mnt_namespace *dup_mnt_ns(struct mnt_namespace *mnt_ns,
 		p = next_mnt(p, mnt_ns->root);
 		q = next_mnt(q, new_ns->root);
 	}
+	new_ns->devpts = get_devpts(mnt_ns->devpts);
 	up_write(&namespace_sem);
 
 	if (rootmnt)
@@ -2764,6 +2766,7 @@ void put_mnt_ns(struct mnt_namespace *ns)
 	br_write_unlock(vfsmount_lock);
 	up_write(&namespace_sem);
 	release_mounts(&umount_list);
+	put_devpts(ns->devpts);
 	kfree(ns);
 }
 EXPORT_SYMBOL(put_mnt_ns);
@@ -2797,3 +2800,15 @@ bool our_mnt(struct vfsmount *mnt)
 {
 	return check_mnt(mnt);
 }
+
+void mnt_set_devpts(struct pts_fs_info *i)
+{
+	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+
+	down_write(&namespace_sem);
+	put_devpts(ns->devpts);
+	ns->devpts = get_devpts(i);
+	up_write(&namespace_sem);
+}
+
+EXPORT_SYMBOL(mnt_set_devpts);
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 5ce0e5f..4b02091 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -25,6 +25,9 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty);
 struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number);
 /* unlink */
 void devpts_pty_kill(struct tty_struct *tty);
+struct pts_fs_info;
+struct pts_fs_info * get_devpts(struct pts_fs_info *);
+void put_devpts(struct pts_fs_info *);
 
 #else
 
@@ -43,6 +46,10 @@ static inline struct tty_struct *devpts_get_tty(struct inode *pts_inode,
 }
 static inline void devpts_pty_kill(struct tty_struct *tty) { }
 
+struct pts_fs_info { };
+static inline struct pts_fs_info *get_devpts(struct pts_fs_info *i) { return NULL; } ;
+static inline void put_devpts(struct pts_fs_info *) { } ;
+
 #endif
 
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f33826..bfeafaa 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1956,6 +1956,8 @@ extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
+struct pts_fs_info;
+extern void mnt_set_devpts(struct pts_fs_info *i);
 
 extern int current_umask(void);
 
diff --git a/include/linux/mnt_namespace.h b/include/linux/mnt_namespace.h
index 2930485..571641c 100644
--- a/include/linux/mnt_namespace.h
+++ b/include/linux/mnt_namespace.h
@@ -6,12 +6,14 @@
 #include <linux/seq_file.h>
 #include <linux/wait.h>
 
+struct pts_fs_info;
 struct mnt_namespace {
 	atomic_t		count;
 	struct vfsmount *	root;
 	struct list_head	list;
 	wait_queue_head_t poll;
 	int event;
+	struct pts_fs_info	*devpts;
 };
 
 struct proc_mounts {
-- 
1.7.8.3

[RFC] fix devpts mount behavior

[Serge Hallyn]

Multiple devpts support currently behaves in a way userspace would likely
not expect.  You can mount a new devpts instance using '-o newinstance'.
However, if you subsequently mount devpts without '-o newinstance', you
will remount the global initial devpts instance.

As a result, if a container is created with a private devpts, but
has devpts in its fstab, then the container's init will end up mounting
the host's devpts.  This (a) confuses the software 'creating' the
container (which usually uses a pty for consoles), (b) confuses
userspace inside the container (especially if the ptmx from the
newinstance is bind-mounted over /dev/ptmx), and (c) is a security
risk to the host.

I've been playing for the last week with ways to fix this, and in the
end I can see two ways.  One is to pin+cache, on every newinstance devpts
mount, the new pts_fs_info at the current mount namespace.  Then if a task
does

	mount -t devpts -o newinstance d2 /mnt
	mount -t devpts devpts /dev/pts

then the new instance will be mounted at /dev/pts.

Dave Hansen commented that this basically turns the newinstance mount 
into an unshare, and that therefore perhaps we should fully support
a devpts namespace.  I actually like this better as it avoids making
awkward new relations between the mnt_namespace and pts_fs_info, and
leaves a comfy well-known 'unshare' operation between the current and
new behaviors.  However, it is behaviorally a bit awkward.  If a task
simply does

	mount -t devpts -o newinstance d2 /mnt
	mount -t devpts devpts /dev/pts

Then /dev/pts will still get the host's initial instance, but if it
does

	lxc-unshare -s MOUNT -- /bin/bash
	mount -t devpts -o newinstance d2 /mnt
	mount -t devpts devpts /dev/pts

then /dev/pts will have the new instance (in the new bash shell with
the unshared mount ns).

The other problem with this approach is that it consumes one of the
very few clone flags (we have 0x02000000 and maybe 0x00002000
available).

The patch below implements the first approach - though it's broken,
so only meant to show roughly how it might look.  If consensus is
that the second approach is best, I'll happily implement that and
gobble up 0x02000000.

Is there perhaps another option I haven't considered?

Am I wrong in even considering the current behavior bad?

BROKEN PATCH, DON'T RUN IT.

Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>

---
 fs/devpts/inode.c             |   48 +++++++++++++++++++++++++++++++++++++++-
 fs/namespace.c                |   15 ++++++++++++
 include/linux/devpts_fs.h     |    7 ++++++
 include/linux/fs.h            |    2 +
 include/linux/mnt_namespace.h |    2 +
 5 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index d5d5297..ef737af 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -25,6 +25,8 @@
 #include <linux/parser.h>
 #include <linux/fsnotify.h>
 #include <linux/seq_file.h>
+#include <linux/nsproxy.h>
+#include <linux/mnt_namespace.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
 /*
@@ -71,8 +73,32 @@ struct pts_fs_info {
 	struct ida allocated_ptys;
 	struct pts_mount_opts mount_opts;
 	struct dentry *ptmx_dentry;
+	struct kref kref;
 };
 
+struct pts_fs_info *get_devpts(struct pts_fs_info *i)
+{
+	if (i)
+		kref_get(&i->kref);
+	return i;
+}
+
+static void free_pts_fs_info(struct kref *kref)
+{
+	struct pts_fs_info *i = container_of(kref, struct pts_fs_info, kref);
+
+	printk(KERN_NOTICE "%s: freeing a pts_fs_info %lx\n", __func__,
+		(unsigned long) i);
+
+	kfree(i);
+}
+
+void put_devpts(struct pts_fs_info *i)
+{
+	if (i)
+		kref_put(&i->kref, free_pts_fs_info);
+}
+
 static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 {
 	return sb->s_fs_info;
@@ -280,6 +306,9 @@ static void *new_pts_fs_info(void)
 	ida_init(&fsi->allocated_ptys);
 	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
 	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
+	kref_init(&fsi->kref);
+	printk(KERN_NOTICE "%s: created a new pts_fs_info %lx\n", __func__,
+		(unsigned long) fsi);
 
 	return fsi;
 }
@@ -325,6 +354,16 @@ fail:
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 static int compare_init_pts_sb(struct super_block *s, void *p)
 {
+	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+
+	if (ns->devpts) {
+		int ret;
+		rcu_read_lock();
+		ret = ns->devpts == DEVPTS_SB(s);
+		rcu_read_unlock();
+		return ret;
+	}
+
 	if (devpts_mnt)
 		return devpts_mnt->mnt_sb == s;
 	return 0;
@@ -382,7 +421,8 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 		if (error)
 			goto out_undo_sget;
 		s->s_flags |= MS_ACTIVE;
-	}
+	} else
+		get_devpts(DEVPTS_SB(s));
 
 	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
 
@@ -390,6 +430,9 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	if (error)
 		goto out_undo_sget;
 
+	if (opts.newinstance)
+		mnt_set_devpts(DEVPTS_SB(s));
+
 	return dget(s->s_root);
 
 out_undo_sget:
@@ -398,6 +441,7 @@ out_undo_sget:
 }
 
 #else
+
 /*
  * This supports only the legacy single-instance semantics (no
  * multiple-instance semantics)
@@ -413,7 +457,7 @@ static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 
-	kfree(fsi);
+	put_devpts(fsi);
 	kill_litter_super(sb);
 }
 
diff --git a/fs/namespace.c b/fs/namespace.c
index aabfd30..953a972 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -31,6 +31,7 @@
 #include <linux/idr.h>
 #include <linux/fs_struct.h>
 #include <linux/fsnotify.h>
+#include <linux/devpts_fs.h>
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
 #include "pnode.h"
@@ -2458,6 +2459,7 @@ static struct mnt_namespace *dup_mnt_ns(struct mnt_namespace *mnt_ns,
 		p = next_mnt(p, mnt_ns->root);
 		q = next_mnt(q, new_ns->root);
 	}
+	new_ns->devpts = get_devpts(mnt_ns->devpts);
 	up_write(&namespace_sem);
 
 	if (rootmnt)
@@ -2764,6 +2766,7 @@ void put_mnt_ns(struct mnt_namespace *ns)
 	br_write_unlock(vfsmount_lock);
 	up_write(&namespace_sem);
 	release_mounts(&umount_list);
+	put_devpts(ns->devpts);
 	kfree(ns);
 }
 EXPORT_SYMBOL(put_mnt_ns);
@@ -2797,3 +2800,15 @@ bool our_mnt(struct vfsmount *mnt)
 {
 	return check_mnt(mnt);
 }
+
+void mnt_set_devpts(struct pts_fs_info *i)
+{
+	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+
+	down_write(&namespace_sem);
+	put_devpts(ns->devpts);
+	ns->devpts = get_devpts(i);
+	up_write(&namespace_sem);
+}
+
+EXPORT_SYMBOL(mnt_set_devpts);
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 5ce0e5f..4b02091 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -25,6 +25,9 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty);
 struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number);
 /* unlink */
 void devpts_pty_kill(struct tty_struct *tty);
+struct pts_fs_info;
+struct pts_fs_info * get_devpts(struct pts_fs_info *);
+void put_devpts(struct pts_fs_info *);
 
 #else
 
@@ -43,6 +46,10 @@ static inline struct tty_struct *devpts_get_tty(struct inode *pts_inode,
 }
 static inline void devpts_pty_kill(struct tty_struct *tty) { }
 
+struct pts_fs_info { };
+static inline struct pts_fs_info *get_devpts(struct pts_fs_info *i) { return NULL; } ;
+static inline void put_devpts(struct pts_fs_info *) { } ;
+
 #endif
 
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f33826..bfeafaa 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1956,6 +1956,8 @@ extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
+struct pts_fs_info;
+extern void mnt_set_devpts(struct pts_fs_info *i);
 
 extern int current_umask(void);
 
diff --git a/include/linux/mnt_namespace.h b/include/linux/mnt_namespace.h
index 2930485..571641c 100644
--- a/include/linux/mnt_namespace.h
+++ b/include/linux/mnt_namespace.h
@@ -6,12 +6,14 @@
 #include <linux/seq_file.h>
 #include <linux/wait.h>
 
+struct pts_fs_info;
 struct mnt_namespace {
 	atomic_t		count;
 	struct vfsmount *	root;
 	struct list_head	list;
 	wait_queue_head_t poll;
 	int event;
+	struct pts_fs_info	*devpts;
 };
 
 struct proc_mounts {
-- 
1.7.8.3

Re: [RFC] fix devpts mount behavior

[Linus Torvalds]

On Mon, Jan 23, 2012 at 4:05 PM, Serge Hallyn
<serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
>
> I've been playing for the last week with ways to fix this, and in the
> end I can see two ways.

How about a third way:

 - make "newinstance" mandatory (and "-o newinstance" is a no-op) and
forget about the whole issue.

 - if you really want to remount the old global one, you have to use a
bind mount of that original mount instead.

There may be some subtle reason why the above is totally broken and
just fundamnetally wouldn't work, and breaks all existing setups, but
maybe it's worth at least discussing as an option?

Or did I entirely misunderstand the problem?

                      Linus

Re: [RFC] fix devpts mount behavior

[Linus Torvalds]

On Mon, Jan 23, 2012 at 4:05 PM, Serge Hallyn
<serge.hallyn@canonical.com> wrote:
>
> I've been playing for the last week with ways to fix this, and in the
> end I can see two ways.

How about a third way:

 - make "newinstance" mandatory (and "-o newinstance" is a no-op) and
forget about the whole issue.

 - if you really want to remount the old global one, you have to use a
bind mount of that original mount instead.

There may be some subtle reason why the above is totally broken and
just fundamnetally wouldn't work, and breaks all existing setups, but
maybe it's worth at least discussing as an option?

Or did I entirely misunderstand the problem?

                      Linus

Re: [RFC] fix devpts mount behavior

[Serge Hallyn]

Quoting Linus Torvalds (torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org):
> On Mon, Jan 23, 2012 at 4:05 PM, Serge Hallyn
> <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> >
> > I've been playing for the last week with ways to fix this, and in the
> > end I can see two ways.
> 
> How about a third way:
> 
>  - make "newinstance" mandatory (and "-o newinstance" is a no-op) and
> forget about the whole issue.
> 
>  - if you really want to remount the old global one, you have to use a
> bind mount of that original mount instead.
> 
> There may be some subtle reason why the above is totally broken and
> just fundamnetally wouldn't work, and breaks all existing setups, but
> maybe it's worth at least discussing as an option?
> 
> Or did I entirely misunderstand the problem?
> 
>                       Linus

I like it.

The only place that *might* be a problem is if initramsfs does a devpts
mount, and later init blindly mounts tmpfs on /dev and mounts a new
devpts.  But it seems unlikely there would be any open pty's so it
shouldn't really matter.

I could go ahead and test that, say, ubuntu and fedora systems boot fine
with this change.  But of course I can't be sure there is no userspace
out there that won't cope...

-serge

Re: [RFC] fix devpts mount behavior

[Serge Hallyn]

Quoting Linus Torvalds (torvalds@linux-foundation.org):
> On Mon, Jan 23, 2012 at 4:05 PM, Serge Hallyn
> <serge.hallyn@canonical.com> wrote:
> >
> > I've been playing for the last week with ways to fix this, and in the
> > end I can see two ways.
> 
> How about a third way:
> 
>  - make "newinstance" mandatory (and "-o newinstance" is a no-op) and
> forget about the whole issue.
> 
>  - if you really want to remount the old global one, you have to use a
> bind mount of that original mount instead.
> 
> There may be some subtle reason why the above is totally broken and
> just fundamnetally wouldn't work, and breaks all existing setups, but
> maybe it's worth at least discussing as an option?
> 
> Or did I entirely misunderstand the problem?
> 
>                       Linus

I like it.

The only place that *might* be a problem is if initramsfs does a devpts
mount, and later init blindly mounts tmpfs on /dev and mounts a new
devpts.  But it seems unlikely there would be any open pty's so it
shouldn't really matter.

I could go ahead and test that, say, ubuntu and fedora systems boot fine
with this change.  But of course I can't be sure there is no userspace
out there that won't cope...

-serge

Re: [RFC] fix devpts mount behavior

[Al Viro]

On Mon, Jan 23, 2012 at 04:13:49PM -0800, Linus Torvalds wrote:
> On Mon, Jan 23, 2012 at 4:05 PM, Serge Hallyn
> <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> >
> > I've been playing for the last week with ways to fix this, and in the
> > end I can see two ways.
> 
> How about a third way:
> 
>  - make "newinstance" mandatory (and "-o newinstance" is a no-op) and
> forget about the whole issue.
> 
>  - if you really want to remount the old global one, you have to use a
> bind mount of that original mount instead.
> 
> There may be some subtle reason why the above is totally broken and
> just fundamnetally wouldn't work, and breaks all existing setups, but
> maybe it's worth at least discussing as an option?

Hell knows...  That's certainly what I would have prefered, but I wonder
about existing practices along the lines of "mount it in chroot jail
from inside that jail" (as opposed to mount --bind /dev/pts /jail/dev/pts
while preparing it).

Posted patch is too ugly to live and as for the new kind of namespace...
yuck.  We already have far too many :-/

Re: [RFC] fix devpts mount behavior

[Al Viro]

On Mon, Jan 23, 2012 at 04:13:49PM -0800, Linus Torvalds wrote:
> On Mon, Jan 23, 2012 at 4:05 PM, Serge Hallyn
> <serge.hallyn@canonical.com> wrote:
> >
> > I've been playing for the last week with ways to fix this, and in the
> > end I can see two ways.
> 
> How about a third way:
> 
>  - make "newinstance" mandatory (and "-o newinstance" is a no-op) and
> forget about the whole issue.
> 
>  - if you really want to remount the old global one, you have to use a
> bind mount of that original mount instead.
> 
> There may be some subtle reason why the above is totally broken and
> just fundamnetally wouldn't work, and breaks all existing setups, but
> maybe it's worth at least discussing as an option?

Hell knows...  That's certainly what I would have prefered, but I wonder
about existing practices along the lines of "mount it in chroot jail
from inside that jail" (as opposed to mount --bind /dev/pts /jail/dev/pts
while preparing it).

Posted patch is too ugly to live and as for the new kind of namespace...
yuck.  We already have far too many :-/

Re: [RFC] fix devpts mount behavior

[Linus Torvalds]

On Mon, Jan 23, 2012 at 4:25 PM, Serge Hallyn
<serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
>
> The only place that *might* be a problem is if initramsfs does a devpts
> mount, and later init blindly mounts tmpfs on /dev and mounts a new
> devpts.  But it seems unlikely there would be any open pty's so it
> shouldn't really matter.

Right. I think the opportunity for problems should be pretty small.

And it's not like the pty itself wouldn't continue to work - it's just
that programs like /usr/bin/tty wouldn't be able to *find* it.

Although who knows - maybe there is some other subtle interaction.

> I could go ahead and test that, say, ubuntu and fedora systems boot fine
> with this change.  But of course I can't be sure there is no userspace
> out there that won't cope...

I think the only way is to try it out.

If you can test a few distros and at least validate that the approach
isn't *totally* broken, I'll apply it. It's early days in the -rc
series yet, and this does seem to be one of those things that we'd be
better off trying to do early rather than delay.

If it causes problems, we will have to revert it and look for
alternate approaches, but if it just works it would seem to be the
right thing to do.

                          Linus

Re: [RFC] fix devpts mount behavior

[Linus Torvalds]

On Mon, Jan 23, 2012 at 4:25 PM, Serge Hallyn
<serge.hallyn@canonical.com> wrote:
>
> The only place that *might* be a problem is if initramsfs does a devpts
> mount, and later init blindly mounts tmpfs on /dev and mounts a new
> devpts.  But it seems unlikely there would be any open pty's so it
> shouldn't really matter.

Right. I think the opportunity for problems should be pretty small.

And it's not like the pty itself wouldn't continue to work - it's just
that programs like /usr/bin/tty wouldn't be able to *find* it.

Although who knows - maybe there is some other subtle interaction.

> I could go ahead and test that, say, ubuntu and fedora systems boot fine
> with this change.  But of course I can't be sure there is no userspace
> out there that won't cope...

I think the only way is to try it out.

If you can test a few distros and at least validate that the approach
isn't *totally* broken, I'll apply it. It's early days in the -rc
series yet, and this does seem to be one of those things that we'd be
better off trying to do early rather than delay.

If it causes problems, we will have to revert it and look for
alternate approaches, but if it just works it would seem to be the
right thing to do.

                          Linus

Re: [RFC] fix devpts mount behavior

[Al Viro]

On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:

> Right. I think the opportunity for problems should be pretty small.
> 
> And it's not like the pty itself wouldn't continue to work - it's just
> that programs like /usr/bin/tty wouldn't be able to *find* it.
> 
> Although who knows - maybe there is some other subtle interaction.

FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
and it *will* work, refering to the "initial" instance.  That's what
concerns me about the chroot scenarios -
	mknod /jail/dev/ptmx c 5 2
	mkdir /jail/dev/pts
	mount -t devpts /jail/dev/pts
	chroot /jail
works fine right now, but with that change behaviour will be all wrong -
opening /dev/ptmx inside of jail will grab you a pts, all right, but
it will *not* show up in (jail) /dev/pts/* as it does with the current
kernel.

Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
the things will keep working.  However, that will _only_ work for kernels
with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
devpts (which is arguably wrong, BTW)

Re: [RFC] fix devpts mount behavior

[Al Viro]

On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:

> Right. I think the opportunity for problems should be pretty small.
> 
> And it's not like the pty itself wouldn't continue to work - it's just
> that programs like /usr/bin/tty wouldn't be able to *find* it.
> 
> Although who knows - maybe there is some other subtle interaction.

FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
and it *will* work, refering to the "initial" instance.  That's what
concerns me about the chroot scenarios -
	mknod /jail/dev/ptmx c 5 2
	mkdir /jail/dev/pts
	mount -t devpts /jail/dev/pts
	chroot /jail
works fine right now, but with that change behaviour will be all wrong -
opening /dev/ptmx inside of jail will grab you a pts, all right, but
it will *not* show up in (jail) /dev/pts/* as it does with the current
kernel.

Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
the things will keep working.  However, that will _only_ work for kernels
with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
devpts (which is arguably wrong, BTW)

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Al Viro (viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org):
> On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
> 
> > Right. I think the opportunity for problems should be pretty small.
> > 
> > And it's not like the pty itself wouldn't continue to work - it's just
> > that programs like /usr/bin/tty wouldn't be able to *find* it.
> > 
> > Although who knows - maybe there is some other subtle interaction.
> 
> FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> and it *will* work, refering to the "initial" instance.  That's what
> concerns me about the chroot scenarios -
> 	mknod /jail/dev/ptmx c 5 2
> 	mkdir /jail/dev/pts
> 	mount -t devpts /jail/dev/pts
> 	chroot /jail
> works fine right now, but with that change behaviour will be all wrong -
> opening /dev/ptmx inside of jail will grab you a pts, all right, but
> it will *not* show up in (jail) /dev/pts/* as it does with the current
> kernel.
> 
> Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> the things will keep working.  However, that will _only_ work for kernels
> with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> devpts (which is arguably wrong, BTW)

Should /dev/pts/ptmx be created for DEVPTS_MULTIPLE_INSTANCES=n?

-serge

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Al Viro (viro@ZenIV.linux.org.uk):
> On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
> 
> > Right. I think the opportunity for problems should be pretty small.
> > 
> > And it's not like the pty itself wouldn't continue to work - it's just
> > that programs like /usr/bin/tty wouldn't be able to *find* it.
> > 
> > Although who knows - maybe there is some other subtle interaction.
> 
> FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> and it *will* work, refering to the "initial" instance.  That's what
> concerns me about the chroot scenarios -
> 	mknod /jail/dev/ptmx c 5 2
> 	mkdir /jail/dev/pts
> 	mount -t devpts /jail/dev/pts
> 	chroot /jail
> works fine right now, but with that change behaviour will be all wrong -
> opening /dev/ptmx inside of jail will grab you a pts, all right, but
> it will *not* show up in (jail) /dev/pts/* as it does with the current
> kernel.
> 
> Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> the things will keep working.  However, that will _only_ work for kernels
> with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> devpts (which is arguably wrong, BTW)

Should /dev/pts/ptmx be created for DEVPTS_MULTIPLE_INSTANCES=n?

-serge

Re: [RFC] fix devpts mount behavior

[Sukadev Bhattiprolu]

Serge Hallyn [serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting Al Viro (viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org):
| > On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
| > 
| > > Right. I think the opportunity for problems should be pretty small.
| > > 
| > > And it's not like the pty itself wouldn't continue to work - it's just
| > > that programs like /usr/bin/tty wouldn't be able to *find* it.
| > > 
| > > Although who knows - maybe there is some other subtle interaction.
| > 
| > FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
| > and it *will* work, refering to the "initial" instance.  That's what
| > concerns me about the chroot scenarios -
| > 	mknod /jail/dev/ptmx c 5 2
| > 	mkdir /jail/dev/pts
| > 	mount -t devpts /jail/dev/pts
| > 	chroot /jail
| > works fine right now, but with that change behaviour will be all wrong -
| > opening /dev/ptmx inside of jail will grab you a pts, all right, but
| > it will *not* show up in (jail) /dev/pts/* as it does with the current
| > kernel.
| > 
| > Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
| > the things will keep working.  However, that will _only_ work for kernels
| > with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
| > devpts (which is arguably wrong, BTW)
| 
| Should /dev/pts/ptmx be created for DEVPTS_MULTIPLE_INSTANCES=n?

With DEVPTS_MULTIPLE_INSTANCES=n, there is only _one_ (global) instance
right ? Why would we need a 'pts/ptmx' node ? To keep the symlink (i.e
user space scripts) valid for both single and multiple instance cases ?

Sukadev

Re: [RFC] fix devpts mount behavior

[Sukadev Bhattiprolu]

Serge Hallyn [serge@hallyn.com] wrote:
| Quoting Al Viro (viro@ZenIV.linux.org.uk):
| > On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
| > 
| > > Right. I think the opportunity for problems should be pretty small.
| > > 
| > > And it's not like the pty itself wouldn't continue to work - it's just
| > > that programs like /usr/bin/tty wouldn't be able to *find* it.
| > > 
| > > Although who knows - maybe there is some other subtle interaction.
| > 
| > FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
| > and it *will* work, refering to the "initial" instance.  That's what
| > concerns me about the chroot scenarios -
| > 	mknod /jail/dev/ptmx c 5 2
| > 	mkdir /jail/dev/pts
| > 	mount -t devpts /jail/dev/pts
| > 	chroot /jail
| > works fine right now, but with that change behaviour will be all wrong -
| > opening /dev/ptmx inside of jail will grab you a pts, all right, but
| > it will *not* show up in (jail) /dev/pts/* as it does with the current
| > kernel.
| > 
| > Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
| > the things will keep working.  However, that will _only_ work for kernels
| > with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
| > devpts (which is arguably wrong, BTW)
| 
| Should /dev/pts/ptmx be created for DEVPTS_MULTIPLE_INSTANCES=n?

With DEVPTS_MULTIPLE_INSTANCES=n, there is only _one_ (global) instance
right ? Why would we need a 'pts/ptmx' node ? To keep the symlink (i.e
user space scripts) valid for both single and multiple instance cases ?

Sukadev

Re: [RFC] fix devpts mount behavior

[Eric W. Biederman]

Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> writes:

> On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
>
>> Right. I think the opportunity for problems should be pretty small.
>> 
>> And it's not like the pty itself wouldn't continue to work - it's just
>> that programs like /usr/bin/tty wouldn't be able to *find* it.
>> 
>> Although who knows - maybe there is some other subtle interaction.
>
> FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> and it *will* work, refering to the "initial" instance.  That's what
> concerns me about the chroot scenarios -
> 	mknod /jail/dev/ptmx c 5 2
> 	mkdir /jail/dev/pts
> 	mount -t devpts /jail/dev/pts
> 	chroot /jail
> works fine right now, but with that change behaviour will be all wrong -
> opening /dev/ptmx inside of jail will grab you a pts, all right, but
> it will *not* show up in (jail) /dev/pts/* as it does with the current
> kernel.
>
> Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> the things will keep working.  However, that will _only_ work for kernels
> with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> devpts (which is arguably wrong, BTW)

For testing I would recommend looking at the distro chroot build cases.

It looks like relatively recent udev still creates /dev/ptmx and does
not create the symlink.  So we might get into the awkward situation of
/dev/ptmx not matching /dev/pts/ptmx with something as simple as
initramfs mounting /dev/pts and then initscripts mounting /dev/pts.

Eric

Re: [RFC] fix devpts mount behavior

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
>
>> Right. I think the opportunity for problems should be pretty small.
>> 
>> And it's not like the pty itself wouldn't continue to work - it's just
>> that programs like /usr/bin/tty wouldn't be able to *find* it.
>> 
>> Although who knows - maybe there is some other subtle interaction.
>
> FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> and it *will* work, refering to the "initial" instance.  That's what
> concerns me about the chroot scenarios -
> 	mknod /jail/dev/ptmx c 5 2
> 	mkdir /jail/dev/pts
> 	mount -t devpts /jail/dev/pts
> 	chroot /jail
> works fine right now, but with that change behaviour will be all wrong -
> opening /dev/ptmx inside of jail will grab you a pts, all right, but
> it will *not* show up in (jail) /dev/pts/* as it does with the current
> kernel.
>
> Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> the things will keep working.  However, that will _only_ work for kernels
> with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> devpts (which is arguably wrong, BTW)

For testing I would recommend looking at the distro chroot build cases.

It looks like relatively recent udev still creates /dev/ptmx and does
not create the symlink.  So we might get into the awkward situation of
/dev/ptmx not matching /dev/pts/ptmx with something as simple as
initramfs mounting /dev/pts and then initscripts mounting /dev/pts.

Eric

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Serge Hallyn [serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org] wrote:
> | Quoting Al Viro (viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org):
> | > On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
> | > 
> | > > Right. I think the opportunity for problems should be pretty small.
> | > > 
> | > > And it's not like the pty itself wouldn't continue to work - it's just
> | > > that programs like /usr/bin/tty wouldn't be able to *find* it.
> | > > 
> | > > Although who knows - maybe there is some other subtle interaction.
> | > 
> | > FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> | > and it *will* work, refering to the "initial" instance.  That's what
> | > concerns me about the chroot scenarios -
> | > 	mknod /jail/dev/ptmx c 5 2
> | > 	mkdir /jail/dev/pts
> | > 	mount -t devpts /jail/dev/pts
> | > 	chroot /jail
> | > works fine right now, but with that change behaviour will be all wrong -
> | > opening /dev/ptmx inside of jail will grab you a pts, all right, but
> | > it will *not* show up in (jail) /dev/pts/* as it does with the current
> | > kernel.
> | > 
> | > Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> | > the things will keep working.  However, that will _only_ work for kernels
> | > with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> | > devpts (which is arguably wrong, BTW)
> | 
> | Should /dev/pts/ptmx be created for DEVPTS_MULTIPLE_INSTANCES=n?
> 
> With DEVPTS_MULTIPLE_INSTANCES=n, there is only _one_ (global) instance
> right ? Why would we need a 'pts/ptmx' node ? To keep the symlink (i.e
> user space scripts) valid for both single and multiple instance cases ?

Exactly

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> Serge Hallyn [serge@hallyn.com] wrote:
> | Quoting Al Viro (viro@ZenIV.linux.org.uk):
> | > On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
> | > 
> | > > Right. I think the opportunity for problems should be pretty small.
> | > > 
> | > > And it's not like the pty itself wouldn't continue to work - it's just
> | > > that programs like /usr/bin/tty wouldn't be able to *find* it.
> | > > 
> | > > Although who knows - maybe there is some other subtle interaction.
> | > 
> | > FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> | > and it *will* work, refering to the "initial" instance.  That's what
> | > concerns me about the chroot scenarios -
> | > 	mknod /jail/dev/ptmx c 5 2
> | > 	mkdir /jail/dev/pts
> | > 	mount -t devpts /jail/dev/pts
> | > 	chroot /jail
> | > works fine right now, but with that change behaviour will be all wrong -
> | > opening /dev/ptmx inside of jail will grab you a pts, all right, but
> | > it will *not* show up in (jail) /dev/pts/* as it does with the current
> | > kernel.
> | > 
> | > Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> | > the things will keep working.  However, that will _only_ work for kernels
> | > with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> | > devpts (which is arguably wrong, BTW)
> | 
> | Should /dev/pts/ptmx be created for DEVPTS_MULTIPLE_INSTANCES=n?
> 
> With DEVPTS_MULTIPLE_INSTANCES=n, there is only _one_ (global) instance
> right ? Why would we need a 'pts/ptmx' node ? To keep the symlink (i.e
> user space scripts) valid for both single and multiple instance cases ?

Exactly

Re: [RFC] fix devpts mount behavior

[Sukadev Bhattiprolu]

Linus Torvalds [torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org] wrote:
| On Mon, Jan 23, 2012 at 4:25 PM, Serge Hallyn
| <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
| >
| > The only place that *might* be a problem is if initramsfs does a devpts
| > mount, and later init blindly mounts tmpfs on /dev and mounts a new
| > devpts.  But it seems unlikely there would be any open pty's so it
| > shouldn't really matter.
| 
| Right. I think the opportunity for problems should be pretty small.
| 
| And it's not like the pty itself wouldn't continue to work - it's just
| that programs like /usr/bin/tty wouldn't be able to *find* it.
| 
| Although who knows - maybe there is some other subtle interaction.
| 
| > I could go ahead and test that, say, ubuntu and fedora systems boot fine
| > with this change.  But of course I can't be sure there is no userspace
| > out there that won't cope...
| 
| I think the only way is to try it out.
| 
| If you can test a few distros and at least validate that the approach
| isn't *totally* broken, I'll apply it. It's early days in the -rc
| series yet, and this does seem to be one of those things that we'd be
| better off trying to do early rather than delay.

Yes, hopefully the new distros can cope with 'newinstance' as the default
and ones that can't can live with CONFIG_DEVPTS_MULTIPLE_INSTANCES=n.

Sukadev

Re: [RFC] fix devpts mount behavior

[Sukadev Bhattiprolu]

Linus Torvalds [torvalds@linux-foundation.org] wrote:
| On Mon, Jan 23, 2012 at 4:25 PM, Serge Hallyn
| <serge.hallyn@canonical.com> wrote:
| >
| > The only place that *might* be a problem is if initramsfs does a devpts
| > mount, and later init blindly mounts tmpfs on /dev and mounts a new
| > devpts.  But it seems unlikely there would be any open pty's so it
| > shouldn't really matter.
| 
| Right. I think the opportunity for problems should be pretty small.
| 
| And it's not like the pty itself wouldn't continue to work - it's just
| that programs like /usr/bin/tty wouldn't be able to *find* it.
| 
| Although who knows - maybe there is some other subtle interaction.
| 
| > I could go ahead and test that, say, ubuntu and fedora systems boot fine
| > with this change.  But of course I can't be sure there is no userspace
| > out there that won't cope...
| 
| I think the only way is to try it out.
| 
| If you can test a few distros and at least validate that the approach
| isn't *totally* broken, I'll apply it. It's early days in the -rc
| series yet, and this does seem to be one of those things that we'd be
| better off trying to do early rather than delay.

Yes, hopefully the new distros can cope with 'newinstance' as the default
and ones that can't can live with CONFIG_DEVPTS_MULTIPLE_INSTANCES=n.

Sukadev

Re: [RFC] fix devpts mount behavior

[Nick Bowler]

On 2012-01-24 12:55 -0800, Sukadev Bhattiprolu wrote:
> Linus Torvalds [torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org] wrote:
[...]
> | If you can test a few distros and at least validate that the approach
> | isn't *totally* broken, I'll apply it. It's early days in the -rc
> | series yet, and this does seem to be one of those things that we'd be
> | better off trying to do early rather than delay.
> 
> Yes, hopefully the new distros can cope with 'newinstance' as the default
> and ones that can't can live with CONFIG_DEVPTS_MULTIPLE_INSTANCES=n.

What about people who chroot into an "old" distro from a "new" livecd?

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [RFC] fix devpts mount behavior

[Nick Bowler]

On 2012-01-24 12:55 -0800, Sukadev Bhattiprolu wrote:
> Linus Torvalds [torvalds@linux-foundation.org] wrote:
[...]
> | If you can test a few distros and at least validate that the approach
> | isn't *totally* broken, I'll apply it. It's early days in the -rc
> | series yet, and this does seem to be one of those things that we'd be
> | better off trying to do early rather than delay.
> 
> Yes, hopefully the new distros can cope with 'newinstance' as the default
> and ones that can't can live with CONFIG_DEVPTS_MULTIPLE_INSTANCES=n.

What about people who chroot into an "old" distro from a "new" livecd?

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> writes:
> 
> > On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
> >
> >> Right. I think the opportunity for problems should be pretty small.
> >> 
> >> And it's not like the pty itself wouldn't continue to work - it's just
> >> that programs like /usr/bin/tty wouldn't be able to *find* it.
> >> 
> >> Although who knows - maybe there is some other subtle interaction.
> >
> > FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> > and it *will* work, refering to the "initial" instance.  That's what
> > concerns me about the chroot scenarios -
> > 	mknod /jail/dev/ptmx c 5 2
> > 	mkdir /jail/dev/pts
> > 	mount -t devpts /jail/dev/pts
> > 	chroot /jail
> > works fine right now, but with that change behaviour will be all wrong -
> > opening /dev/ptmx inside of jail will grab you a pts, all right, but
> > it will *not* show up in (jail) /dev/pts/* as it does with the current
> > kernel.
> >
> > Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> > the things will keep working.  However, that will _only_ work for kernels
> > with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> > devpts (which is arguably wrong, BTW)
> 
> For testing I would recommend looking at the distro chroot build cases.

Do you have a specific example in mind?  I would expect build chroots
generally don't mount a devpts.

> It looks like relatively recent udev still creates /dev/ptmx and does

Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
entry doesn't stop it.  (this is after I've had an init job replace the
devtmpfs-created ptmx entry with a symlink)

So current distros (well, Ubuntu and Fedora at least) would need to at least
(a) fix udev, (b) change the default devpts mount (done from initramfs) to
add ptmxmode=666, (c) (if not done in udev) create the /dev/ptmx symlink.

For safety I'd recommend creating /dev/pts/ptmx with
DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
setting ptmxmode to 666 as that's what udev does.

> not create the symlink.  So we might get into the awkward situation of
> /dev/ptmx not matching /dev/pts/ptmx with something as simple as
> initramfs mounting /dev/pts and then initscripts mounting /dev/pts.

That shouldn't matter with a symlink, though it is sloppy.

-serge

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xmission.com):
> Al Viro <viro@ZenIV.linux.org.uk> writes:
> 
> > On Mon, Jan 23, 2012 at 04:41:25PM -0800, Linus Torvalds wrote:
> >
> >> Right. I think the opportunity for problems should be pretty small.
> >> 
> >> And it's not like the pty itself wouldn't continue to work - it's just
> >> that programs like /usr/bin/tty wouldn't be able to *find* it.
> >> 
> >> Although who knows - maybe there is some other subtle interaction.
> >
> > FWIW, the subtle and nasty part in all that is  that you can mknod /dev/ptmx
> > and it *will* work, refering to the "initial" instance.  That's what
> > concerns me about the chroot scenarios -
> > 	mknod /jail/dev/ptmx c 5 2
> > 	mkdir /jail/dev/pts
> > 	mount -t devpts /jail/dev/pts
> > 	chroot /jail
> > works fine right now, but with that change behaviour will be all wrong -
> > opening /dev/ptmx inside of jail will grab you a pts, all right, but
> > it will *not* show up in (jail) /dev/pts/* as it does with the current
> > kernel.
> >
> > Note that if you replace that mknod with symlink pts/ptmx /jail/dev/ptmx
> > the things will keep working.  However, that will _only_ work for kernels
> > with DEVPTS_MULTIPLE_INSTANCES - without it you won't get ptmx inside
> > devpts (which is arguably wrong, BTW)
> 
> For testing I would recommend looking at the distro chroot build cases.

Do you have a specific example in mind?  I would expect build chroots
generally don't mount a devpts.

> It looks like relatively recent udev still creates /dev/ptmx and does

Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
entry doesn't stop it.  (this is after I've had an init job replace the
devtmpfs-created ptmx entry with a symlink)

So current distros (well, Ubuntu and Fedora at least) would need to at least
(a) fix udev, (b) change the default devpts mount (done from initramfs) to
add ptmxmode=666, (c) (if not done in udev) create the /dev/ptmx symlink.

For safety I'd recommend creating /dev/pts/ptmx with
DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
setting ptmxmode to 666 as that's what udev does.

> not create the symlink.  So we might get into the awkward situation of
> /dev/ptmx not matching /dev/pts/ptmx with something as simple as
> initramfs mounting /dev/pts and then initscripts mounting /dev/pts.

That shouldn't matter with a symlink, though it is sloppy.

-serge

Re: [RFC] fix devpts mount behavior

[Kay Sievers]

On Tue, Jan 24, 2012 at 23:02, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Eric W. Biederman (ebiederm@xmission.com):

>> It looks like relatively recent udev still creates /dev/ptmx and does
>
> Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
> entry doesn't stop it.  (this is after I've had an init job replace the
> devtmpfs-created ptmx entry with a symlink)

Udev has nothing to do with that. The kernel creates that device node.
Udev does not carry any rules you could remove, to name device nodes,
it only set permissions and creates symlinks to device nodes.

It will never replace a kernel-created device node with a symlink,
there is no way to express that. If you don't want a device node
there, you need to change the kernel, to not export
/sys/class/tty/ptmx/ the way it is today.

> So current distros (well, Ubuntu and Fedora at least) would need to at least
> (a) fix udev,

To do what?

> (b) change the default devpts mount (done from initramfs) to
> add ptmxmode=666,

> (c) (if not done in udev) create the /dev/ptmx symlink.

Udev can only create symlinks to devices the driver-core creates, not
to devices inside a custom filesystem.

> For safety I'd recommend creating /dev/pts/ptmx with
> DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
> setting ptmxmode to 666 as that's what udev does.

The mode for ptmx is set by the kernel itself, and does not even need
udev to do that:
  $ cat /sys/class/tty/ptmx/uevent
  MAJOR=5
  MINOR=2
  DEVNAME=ptmx
  DEVMODE=0666

Kay
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [RFC] fix devpts mount behavior

[Kay Sievers]

On Tue, Jan 24, 2012 at 23:02, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Eric W. Biederman (ebiederm@xmission.com):

>> It looks like relatively recent udev still creates /dev/ptmx and does
>
> Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
> entry doesn't stop it.  (this is after I've had an init job replace the
> devtmpfs-created ptmx entry with a symlink)

Udev has nothing to do with that. The kernel creates that device node.
Udev does not carry any rules you could remove, to name device nodes,
it only set permissions and creates symlinks to device nodes.

It will never replace a kernel-created device node with a symlink,
there is no way to express that. If you don't want a device node
there, you need to change the kernel, to not export
/sys/class/tty/ptmx/ the way it is today.

> So current distros (well, Ubuntu and Fedora at least) would need to at least
> (a) fix udev,

To do what?

> (b) change the default devpts mount (done from initramfs) to
> add ptmxmode=666,

> (c) (if not done in udev) create the /dev/ptmx symlink.

Udev can only create symlinks to devices the driver-core creates, not
to devices inside a custom filesystem.

> For safety I'd recommend creating /dev/pts/ptmx with
> DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
> setting ptmxmode to 666 as that's what udev does.

The mode for ptmx is set by the kernel itself, and does not even need
udev to do that:
  $ cat /sys/class/tty/ptmx/uevent
  MAJOR=5
  MINOR=2
  DEVNAME=ptmx
  DEVMODE=0666

Kay

Re: [RFC] fix devpts mount behavior

[Serge Hallyn]

Quoting Kay Sievers (kay.sievers-tD+1rO4QERM@public.gmane.org):
> On Tue, Jan 24, 2012 at 23:02, Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> wrote:
> > Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> 
> >> It looks like relatively recent udev still creates /dev/ptmx and does
> >
> > Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
> > entry doesn't stop it.  (this is after I've had an init job replace the
> > devtmpfs-created ptmx entry with a symlink)
> 
> Udev has nothing to do with that. The kernel creates that device node.
> Udev does not carry any rules you could remove, to name device nodes,
> it only set permissions and creates symlinks to device nodes.

That's odd, because I was sure I deleted the node after the kernel created
it.

But it sounds like I must have done it wrong.

> It will never replace a kernel-created device node with a symlink,
> there is no way to express that. If you don't want a device node
> there, you need to change the kernel, to not export
> /sys/class/tty/ptmx/ the way it is today.
> 
> > So current distros (well, Ubuntu and Fedora at least) would need to at least
> > (a) fix udev,
> 
> To do what?

Nothing, as I'm sure you're right above  :)

> > (b) change the default devpts mount (done from initramfs) to
> > add ptmxmode=666,
> 
> > (c) (if not done in udev) create the /dev/ptmx symlink.
> 
> Udev can only create symlinks to devices the driver-core creates, not
> to devices inside a custom filesystem.

I see.

> > For safety I'd recommend creating /dev/pts/ptmx with
> > DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
> > setting ptmxmode to 666 as that's what udev does.
> 
> The mode for ptmx is set by the kernel itself, and does not even need
> udev to do that:
>   $ cat /sys/class/tty/ptmx/uevent
>   MAJOR=5
>   MINOR=2
>   DEVNAME=ptmx
>   DEVMODE=0666

That has nothing to do with /dev/pts/ptmx, whose perms are set based on
the '-o ptmxmode=" argument, and default to 000 if not specified.  If
/dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
the /dev/pts/ptmx perms to not be 000, or users won't be able to create
ptys.

-serge

Re: [RFC] fix devpts mount behavior

[Serge Hallyn]

Quoting Kay Sievers (kay.sievers@vrfy.org):
> On Tue, Jan 24, 2012 at 23:02, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> 
> >> It looks like relatively recent udev still creates /dev/ptmx and does
> >
> > Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
> > entry doesn't stop it.  (this is after I've had an init job replace the
> > devtmpfs-created ptmx entry with a symlink)
> 
> Udev has nothing to do with that. The kernel creates that device node.
> Udev does not carry any rules you could remove, to name device nodes,
> it only set permissions and creates symlinks to device nodes.

That's odd, because I was sure I deleted the node after the kernel created
it.

But it sounds like I must have done it wrong.

> It will never replace a kernel-created device node with a symlink,
> there is no way to express that. If you don't want a device node
> there, you need to change the kernel, to not export
> /sys/class/tty/ptmx/ the way it is today.
> 
> > So current distros (well, Ubuntu and Fedora at least) would need to at least
> > (a) fix udev,
> 
> To do what?

Nothing, as I'm sure you're right above  :)

> > (b) change the default devpts mount (done from initramfs) to
> > add ptmxmode=666,
> 
> > (c) (if not done in udev) create the /dev/ptmx symlink.
> 
> Udev can only create symlinks to devices the driver-core creates, not
> to devices inside a custom filesystem.

I see.

> > For safety I'd recommend creating /dev/pts/ptmx with
> > DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
> > setting ptmxmode to 666 as that's what udev does.
> 
> The mode for ptmx is set by the kernel itself, and does not even need
> udev to do that:
>   $ cat /sys/class/tty/ptmx/uevent
>   MAJOR=5
>   MINOR=2
>   DEVNAME=ptmx
>   DEVMODE=0666

That has nothing to do with /dev/pts/ptmx, whose perms are set based on
the '-o ptmxmode=" argument, and default to 000 if not specified.  If
/dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
the /dev/pts/ptmx perms to not be 000, or users won't be able to create
ptys.

-serge

Re: [RFC] fix devpts mount behavior

[Sukadev Bhattiprolu]

Serge Hallyn [serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org] wrote:
| 
| That has nothing to do with /dev/pts/ptmx, whose perms are set based on
| the '-o ptmxmode=" argument, and default to 000 if not specified.  If
| /dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
| the /dev/pts/ptmx perms to not be 000, or users won't be able to create
| ptys.

If we are going to make 'newinstance' the default, maybe the default
ptmx mode could be changed to 0666 in the kernel too ?

Re: [RFC] fix devpts mount behavior

[Sukadev Bhattiprolu]

Serge Hallyn [serge.hallyn@canonical.com] wrote:
| 
| That has nothing to do with /dev/pts/ptmx, whose perms are set based on
| the '-o ptmxmode=" argument, and default to 000 if not specified.  If
| /dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
| the /dev/pts/ptmx perms to not be 000, or users won't be able to create
| ptys.

If we are going to make 'newinstance' the default, maybe the default
ptmx mode could be changed to 0666 in the kernel too ?

Re: [RFC] fix devpts mount behavior

[Kay Sievers]

On Wed, Jan 25, 2012 at 00:16, Serge Hallyn <serge.hallyn@canonical.com> wrote:
> Quoting Kay Sievers (kay.sievers@vrfy.org):
>> On Tue, Jan 24, 2012 at 23:02, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > Quoting Eric W. Biederman (ebiederm@xmission.com):
>>
>> >> It looks like relatively recent udev still creates /dev/ptmx and does
>> >
>> > Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
>> > entry doesn't stop it.  (this is after I've had an init job replace the
>> > devtmpfs-created ptmx entry with a symlink)
>>
>> Udev has nothing to do with that. The kernel creates that device node.
>> Udev does not carry any rules you could remove, to name device nodes,
>> it only set permissions and creates symlinks to device nodes.
>
> That's odd, because I was sure I deleted the node after the kernel created
> it.
>
> But it sounds like I must have done it wrong.

Oh, older udevs re-create it when you run 'udevadm trigger', but only
then, never on its own, there will be no such event. Current udevs
will not do mknod() anymore, never.

>> It will never replace a kernel-created device node with a symlink,
>> there is no way to express that. If you don't want a device node
>> there, you need to change the kernel, to not export
>> /sys/class/tty/ptmx/ the way it is today.
>>
>> > So current distros (well, Ubuntu and Fedora at least) would need to at least
>> > (a) fix udev,
>>
>> To do what?
>
> Nothing, as I'm sure you're right above  :)

:)

>> > (b) change the default devpts mount (done from initramfs) to
>> > add ptmxmode=666,
>>
>> > (c) (if not done in udev) create the /dev/ptmx symlink.
>>
>> Udev can only create symlinks to devices the driver-core creates, not
>> to devices inside a custom filesystem.
>
> I see.
>
>> > For safety I'd recommend creating /dev/pts/ptmx with
>> > DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
>> > setting ptmxmode to 666 as that's what udev does.
>>
>> The mode for ptmx is set by the kernel itself, and does not even need
>> udev to do that:
>>   $ cat /sys/class/tty/ptmx/uevent
>>   MAJOR=5
>>   MINOR=2
>>   DEVNAME=ptmx
>>   DEVMODE=0666
>
> That has nothing to do with /dev/pts/ptmx, whose perms are set based on
> the '-o ptmxmode=" argument, and default to 000 if not specified.

Yeah, right. Just saying that some permissions are set by the kernel
itself these days. I wouldn't be bad if the kernel had the default of
0666 for the devpts fs, would it?

> If /dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
> the /dev/pts/ptmx perms to not be 000, or users won't be able to create
> ptys.

Right. Change the devpts in-kernel default?

We might also thing about changing /sys/class/tty/ptmx/, and have the
kernel create the symlink? The loops through userspace to setup
default kernel stuff are kind of crazy ...

Kay
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [RFC] fix devpts mount behavior

[Kay Sievers]

On Wed, Jan 25, 2012 at 00:16, Serge Hallyn <serge.hallyn@canonical.com> wrote:
> Quoting Kay Sievers (kay.sievers@vrfy.org):
>> On Tue, Jan 24, 2012 at 23:02, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > Quoting Eric W. Biederman (ebiederm@xmission.com):
>>
>> >> It looks like relatively recent udev still creates /dev/ptmx and does
>> >
>> > Boy, it does, and it's stubborn about it.  Removing the /lib/udev/rules.d
>> > entry doesn't stop it.  (this is after I've had an init job replace the
>> > devtmpfs-created ptmx entry with a symlink)
>>
>> Udev has nothing to do with that. The kernel creates that device node.
>> Udev does not carry any rules you could remove, to name device nodes,
>> it only set permissions and creates symlinks to device nodes.
>
> That's odd, because I was sure I deleted the node after the kernel created
> it.
>
> But it sounds like I must have done it wrong.

Oh, older udevs re-create it when you run 'udevadm trigger', but only
then, never on its own, there will be no such event. Current udevs
will not do mknod() anymore, never.

>> It will never replace a kernel-created device node with a symlink,
>> there is no way to express that. If you don't want a device node
>> there, you need to change the kernel, to not export
>> /sys/class/tty/ptmx/ the way it is today.
>>
>> > So current distros (well, Ubuntu and Fedora at least) would need to at least
>> > (a) fix udev,
>>
>> To do what?
>
> Nothing, as I'm sure you're right above  :)

:)

>> > (b) change the default devpts mount (done from initramfs) to
>> > add ptmxmode=666,
>>
>> > (c) (if not done in udev) create the /dev/ptmx symlink.
>>
>> Udev can only create symlinks to devices the driver-core creates, not
>> to devices inside a custom filesystem.
>
> I see.
>
>> > For safety I'd recommend creating /dev/pts/ptmx with
>> > DEVPTS_MULTIPLE_INSTANCES=n (or dropping that support), and by default
>> > setting ptmxmode to 666 as that's what udev does.
>>
>> The mode for ptmx is set by the kernel itself, and does not even need
>> udev to do that:
>>   $ cat /sys/class/tty/ptmx/uevent
>>   MAJOR=5
>>   MINOR=2
>>   DEVNAME=ptmx
>>   DEVMODE=0666
>
> That has nothing to do with /dev/pts/ptmx, whose perms are set based on
> the '-o ptmxmode=" argument, and default to 000 if not specified.

Yeah, right. Just saying that some permissions are set by the kernel
itself these days. I wouldn't be bad if the kernel had the default of
0666 for the devpts fs, would it?

> If /dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
> the /dev/pts/ptmx perms to not be 000, or users won't be able to create
> ptys.

Right. Change the devpts in-kernel default?

We might also thing about changing /sys/class/tty/ptmx/, and have the
kernel create the symlink? The loops through userspace to setup
default kernel stuff are kind of crazy ...

Kay

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Serge Hallyn [serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org] wrote:
> | 
> | That has nothing to do with /dev/pts/ptmx, whose perms are set based on
> | the '-o ptmxmode=" argument, and default to 000 if not specified.  If
> | /dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
> | the /dev/pts/ptmx perms to not be 000, or users won't be able to create
> | ptys.
> 
> If we are going to make 'newinstance' the default, maybe the default
> ptmx mode could be changed to 0666 in the kernel too ?
> 

Right that was what i meant :)

thanks,
-serge

Re: [RFC] fix devpts mount behavior

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> Serge Hallyn [serge.hallyn@canonical.com] wrote:
> | 
> | That has nothing to do with /dev/pts/ptmx, whose perms are set based on
> | the '-o ptmxmode=" argument, and default to 000 if not specified.  If
> | /dev/ptmx is going to be a symlink to /dev/pts/ptmx, then we have to set
> | the /dev/pts/ptmx perms to not be 000, or users won't be able to create
> | ptys.
> 
> If we are going to make 'newinstance' the default, maybe the default
> ptmx mode could be changed to 0666 in the kernel too ?
> 

Right that was what i meant :)

thanks,
-serge

Re: [RFC] fix devpts mount behavior

[Eric W. Biederman]

"Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:

>> For testing I would recommend looking at the distro chroot build cases.
>
> Do you have a specific example in mind?  I would expect build chroots
> generally don't mount a devpts.

I was thinking of mock on fedora.  I think other distros have something
similar.   mock is sufficient to test programs in so it probably has
a /dev/pts somewhere.  Mostly though I was trying to think of cases
where this would matter.

Eric

Re: [RFC] fix devpts mount behavior

[Eric W. Biederman]

"Serge E. Hallyn" <serge@hallyn.com> writes:

>> For testing I would recommend looking at the distro chroot build cases.
>
> Do you have a specific example in mind?  I would expect build chroots
> generally don't mount a devpts.

I was thinking of mock on fedora.  I think other distros have something
similar.   mock is sufficient to test programs in so it probably has
a /dev/pts somewhere.  Mostly though I was trying to think of cases
where this would matter.

Eric

Re: [RFC] fix devpts mount behavior

[Serge Hallyn]

Quoting Kay Sievers (kay.sievers-tD+1rO4QERM@public.gmane.org):
> We might also thing about changing /sys/class/tty/ptmx/, and have the
> kernel create the symlink? The loops through userspace to setup
> default kernel stuff are kind of crazy ...
> 
> Kay

Hi Kay,

sorry, there is something I'm missing here.  Without a target kobj
to link to, how would we go about telling the kernel that /dev/ptmx
should be a symlink to /dev/pts/ptmx?  Is there a way to do that?

thanks,
-serge

Re: [RFC] fix devpts mount behavior

[Serge Hallyn]

Quoting Kay Sievers (kay.sievers@vrfy.org):
> We might also thing about changing /sys/class/tty/ptmx/, and have the
> kernel create the symlink? The loops through userspace to setup
> default kernel stuff are kind of crazy ...
> 
> Kay

Hi Kay,

sorry, there is something I'm missing here.  Without a target kobj
to link to, how would we go about telling the kernel that /dev/ptmx
should be a symlink to /dev/pts/ptmx?  Is there a way to do that?

thanks,
-serge

Re: [RFC] fix devpts mount behavior

[Eric W. Biederman]

Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:

> Quoting Kay Sievers (kay.sievers-tD+1rO4QERM@public.gmane.org):
>> We might also thing about changing /sys/class/tty/ptmx/, and have the
>> kernel create the symlink? The loops through userspace to setup
>> default kernel stuff are kind of crazy ...
>> 
>> Kay
>
> Hi Kay,
>
> sorry, there is something I'm missing here.  Without a target kobj
> to link to, how would we go about telling the kernel that /dev/ptmx
> should be a symlink to /dev/pts/ptmx?  Is there a way to do that?

There is precedent in the /dev/fd symlinks, so I expect you want
to look and see where the configuration for that comes from.

Eric

Re: [RFC] fix devpts mount behavior

[Eric W. Biederman]

Serge Hallyn <serge.hallyn@canonical.com> writes:

> Quoting Kay Sievers (kay.sievers@vrfy.org):
>> We might also thing about changing /sys/class/tty/ptmx/, and have the
>> kernel create the symlink? The loops through userspace to setup
>> default kernel stuff are kind of crazy ...
>> 
>> Kay
>
> Hi Kay,
>
> sorry, there is something I'm missing here.  Without a target kobj
> to link to, how would we go about telling the kernel that /dev/ptmx
> should be a symlink to /dev/pts/ptmx?  Is there a way to do that?

There is precedent in the /dev/fd symlinks, so I expect you want
to look and see where the configuration for that comes from.

Eric

Re: [RFC] fix devpts mount behavior

[Kay Sievers]

On Sat, Jan 28, 2012 at 21:52, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Serge Hallyn <serge.hallyn@canonical.com> writes:
>
>> Quoting Kay Sievers (kay.sievers@vrfy.org):
>>> We might also thing about changing /sys/class/tty/ptmx/, and have the
>>> kernel create the symlink? The loops through userspace to setup
>>> default kernel stuff are kind of crazy ...

>> sorry, there is something I'm missing here.  Without a target kobj
>> to link to, how would we go about telling the kernel that /dev/ptmx
>> should be a symlink to /dev/pts/ptmx?  Is there a way to do that?

There is no support for symlinks in the core at the moment. But I
guess we could just add support for it to the driver core and devtmpfs
to create a symlink in /dev instead of a device node.

The ptmx device would need to pass that information along to the
driver core call when creating the ptmx device.

> There is precedent in the /dev/fd symlinks, so I expect you want
> to look and see where the configuration for that comes from.

That's a static list of stuff created at startup. But it's easier to
do in usersace, because /proc is not configurable or optional for
usual use cases, so we can just do it unconditionally in userspace.
They also can not conflict with device nodes.

Kay
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [RFC] fix devpts mount behavior

[Kay Sievers]

On Sat, Jan 28, 2012 at 21:52, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Serge Hallyn <serge.hallyn@canonical.com> writes:
>
>> Quoting Kay Sievers (kay.sievers@vrfy.org):
>>> We might also thing about changing /sys/class/tty/ptmx/, and have the
>>> kernel create the symlink? The loops through userspace to setup
>>> default kernel stuff are kind of crazy ...

>> sorry, there is something I'm missing here.  Without a target kobj
>> to link to, how would we go about telling the kernel that /dev/ptmx
>> should be a symlink to /dev/pts/ptmx?  Is there a way to do that?

There is no support for symlinks in the core at the moment. But I
guess we could just add support for it to the driver core and devtmpfs
to create a symlink in /dev instead of a device node.

The ptmx device would need to pass that information along to the
driver core call when creating the ptmx device.

> There is precedent in the /dev/fd symlinks, so I expect you want
> to look and see where the configuration for that comes from.

That's a static list of stuff created at startup. But it's easier to
do in usersace, because /proc is not configurable or optional for
usual use cases, so we can just do it unconditionally in userspace.
They also can not conflict with device nodes.

Kay

[PATCH 0/4] devpts: fix devpts mount behavior

[Eric W. Biederman]

This is the restart of an old conversation that started out with Serge
noting that there are problems taking advantage of the devpts newinstance
mount option.  As the conversation progressed it was suggested that the
devpts newinstance mount option should just be removed if possible.
Then the conversation floundered upon what to do about /dev/ptmx.
Making /dev/ptmx a symlink sounds simple, but in practice no one could
actually figure out how to make it happen.

I was playing with this recently and it is not at all hard or
complicated to have opens of ptmx redirect themselves to pts/ptmx.

So I propose we solve this issue by adding a little magic to /dev/ptmx
and making the devpts newinstance mount option historical.

I my limited testing the code just works.

I assume that since devpts is essentially serial this should go through
Greg's tree?

Eric

[PATCH 0/4] devpts: fix devpts mount behavior

[Eric W. Biederman]

This is the restart of an old conversation that started out with Serge
noting that there are problems taking advantage of the devpts newinstance
mount option.  As the conversation progressed it was suggested that the
devpts newinstance mount option should just be removed if possible.
Then the conversation floundered upon what to do about /dev/ptmx.
Making /dev/ptmx a symlink sounds simple, but in practice no one could
actually figure out how to make it happen.

I was playing with this recently and it is not at all hard or
complicated to have opens of ptmx redirect themselves to pts/ptmx.

So I propose we solve this issue by adding a little magic to /dev/ptmx
and making the devpts newinstance mount option historical.

I my limited testing the code just works.

I assume that since devpts is essentially serial this should go through
Greg's tree?

Eric

[PATCH 1/4] devpts: Remove CONFIG_DEVPTS_MULTIPLE_INSTANCES

[Eric W. Biederman]

This compile option just makes the code more complicated and makes
it more difficult to transition to a time when we take advantage
of the new features that DEVPTS_MULTIPLE_INSTANCES allows.

Acked-by: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 Documentation/filesystems/devpts.txt |   36 ++++++++++++---------------------
 drivers/tty/Kconfig                  |   11 ----------
 fs/devpts/inode.c                    |   28 --------------------------
 3 files changed, 13 insertions(+), 62 deletions(-)

diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
index 68dffd8..260cf6d 100644
--- a/Documentation/filesystems/devpts.txt
+++ b/Documentation/filesystems/devpts.txt
@@ -6,33 +6,23 @@ allocated in other instances of devpts.
 To preserve backward compatibility, this support for multiple instances is
 enabled only if:
 
-	- CONFIG_DEVPTS_MULTIPLE_INSTANCES=y, and
 	- '-o newinstance' mount option is specified while mounting devpts
 
 IOW, devpts now supports both single-instance and multi-instance semantics.
 
-If CONFIG_DEVPTS_MULTIPLE_INSTANCES=n, there is no change in behavior and
-this referred to as the "legacy" mode. In this mode, the new mount options
-(-o newinstance and -o ptmxmode) will be ignored with a 'bogus option' message
-on console.
-
-If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and devpts is mounted without the
-'newinstance' option (as in current start-up scripts) the new mount binds
-to the initial kernel mount of devpts. This mode is referred to as the
-'single-instance' mode and the current, single-instance semantics are
-preserved, i.e PTYs are common across the system.
-
-The only difference between this single-instance mode and the legacy mode
-is the presence of new, '/dev/pts/ptmx' node with permissions 0000, which
-can safely be ignored.
-
-If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and 'newinstance' option is specified,
-the mount is considered to be in the multi-instance mode and a new instance
-of the devpts fs is created. Any ptys created in this instance are independent
-of ptys in other instances of devpts. Like in the single-instance mode, the
-/dev/pts/ptmx node is present. To effectively use the multi-instance mode,
-open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using a symlink or
-bind-mount.
+If devpts is mounted without the 'newinstance' option (as in current
+start-up scripts) the new mount binds to the initial kernel mount of
+devpts. This mode is referred to as the 'single-instance' mode and the
+current, single-instance semantics are preserved, i.e PTYs are common
+across the system.
+
+If 'newinstance' option is specified, the mount is considered to be in
+the multi-instance mode and a new instance of the devpts fs is
+created. Any ptys created in this instance are independent of ptys in
+other instances of devpts. Like in the single-instance mode, the
+/dev/pts/ptmx node is present. To effectively use the multi-instance
+mode, open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using
+a symlink or bind-mount.
 
 Eg: A container startup script could do the following:
 
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 830cd62..5e5cd3e 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -109,17 +109,6 @@ config UNIX98_PTYS
 	  All modern Linux systems use the Unix98 ptys.  Say Y unless
 	  you're on an embedded system and want to conserve memory.
 
-config DEVPTS_MULTIPLE_INSTANCES
-	bool "Support multiple instances of devpts"
-	depends on UNIX98_PTYS
-	default n
-	---help---
-	  Enable support for multiple instances of devpts filesystem.
-	  If you want to have isolated PTY namespaces (eg: in containers),
-	  say Y here.  Otherwise, say N. If enabled, each mount of devpts
-	  filesystem with the '-o newinstance' option will create an
-	  independent PTY namespace.
-
 config LEGACY_PTYS
 	bool "Legacy (BSD) PTY support"
 	default y
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 14afbab..5f0d64e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -115,11 +115,9 @@ static const match_table_t tokens = {
 	{Opt_uid, "uid=%u"},
 	{Opt_gid, "gid=%u"},
 	{Opt_mode, "mode=%o"},
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	{Opt_ptmxmode, "ptmxmode=%o"},
 	{Opt_newinstance, "newinstance"},
 	{Opt_max, "max=%d"},
-#endif
 	{Opt_err, NULL}
 };
 
@@ -136,10 +134,8 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 
 static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 {
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
 		return inode->i_sb;
-#endif
 	return devpts_mnt->mnt_sb;
 }
 
@@ -206,7 +202,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 				return -EINVAL;
 			opts->mode = option & S_IALLUGO;
 			break;
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 		case Opt_ptmxmode:
 			if (match_octal(&args[0], &option))
 				return -EINVAL;
@@ -223,7 +218,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 				return -EINVAL;
 			opts->max = option;
 			break;
-#endif
 		default:
 			printk(KERN_ERR "devpts: called with bogus options\n");
 			return -EINVAL;
@@ -233,7 +227,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 	return 0;
 }
 
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 static int mknod_ptmx(struct super_block *sb)
 {
 	int mode;
@@ -291,12 +284,6 @@ static void update_ptmx_mode(struct pts_fs_info *fsi)
 		inode->i_mode = S_IFCHR|fsi->mount_opts.ptmxmode;
 	}
 }
-#else
-static inline void update_ptmx_mode(struct pts_fs_info *fsi)
-{
-       return;
-}
-#endif
 
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
@@ -327,11 +314,9 @@ static int devpts_show_options(struct seq_file *seq, struct dentry *root)
 	if (opts->setgid)
 		seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, opts->gid));
 	seq_printf(seq, ",mode=%03o", opts->mode);
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
 	if (opts->max < NR_UNIX98_PTY_MAX)
 		seq_printf(seq, ",max=%d", opts->max);
-#endif
 
 	return 0;
 }
@@ -392,7 +377,6 @@ fail:
 	return -ENOMEM;
 }
 
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 static int compare_init_pts_sb(struct super_block *s, void *p)
 {
 	if (devpts_mnt)
@@ -467,18 +451,6 @@ out_undo_sget:
 	return ERR_PTR(error);
 }
 
-#else
-/*
- * This supports only the legacy single-instance semantics (no
- * multiple-instance semantics)
- */
-static struct dentry *devpts_mount(struct file_system_type *fs_type, int flags,
-		const char *dev_name, void *data)
-{
-	return mount_single(fs_type, flags, data, devpts_fill_super);
-}
-#endif
-
 static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
-- 
1.7.5.4

[PATCH 1/4] devpts: Remove CONFIG_DEVPTS_MULTIPLE_INSTANCES

[Eric W. Biederman]

This compile option just makes the code more complicated and makes
it more difficult to transition to a time when we take advantage
of the new features that DEVPTS_MULTIPLE_INSTANCES allows.

Acked-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 Documentation/filesystems/devpts.txt |   36 ++++++++++++---------------------
 drivers/tty/Kconfig                  |   11 ----------
 fs/devpts/inode.c                    |   28 --------------------------
 3 files changed, 13 insertions(+), 62 deletions(-)

diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
index 68dffd8..260cf6d 100644
--- a/Documentation/filesystems/devpts.txt
+++ b/Documentation/filesystems/devpts.txt
@@ -6,33 +6,23 @@ allocated in other instances of devpts.
 To preserve backward compatibility, this support for multiple instances is
 enabled only if:
 
-	- CONFIG_DEVPTS_MULTIPLE_INSTANCES=y, and
 	- '-o newinstance' mount option is specified while mounting devpts
 
 IOW, devpts now supports both single-instance and multi-instance semantics.
 
-If CONFIG_DEVPTS_MULTIPLE_INSTANCES=n, there is no change in behavior and
-this referred to as the "legacy" mode. In this mode, the new mount options
-(-o newinstance and -o ptmxmode) will be ignored with a 'bogus option' message
-on console.
-
-If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and devpts is mounted without the
-'newinstance' option (as in current start-up scripts) the new mount binds
-to the initial kernel mount of devpts. This mode is referred to as the
-'single-instance' mode and the current, single-instance semantics are
-preserved, i.e PTYs are common across the system.
-
-The only difference between this single-instance mode and the legacy mode
-is the presence of new, '/dev/pts/ptmx' node with permissions 0000, which
-can safely be ignored.
-
-If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and 'newinstance' option is specified,
-the mount is considered to be in the multi-instance mode and a new instance
-of the devpts fs is created. Any ptys created in this instance are independent
-of ptys in other instances of devpts. Like in the single-instance mode, the
-/dev/pts/ptmx node is present. To effectively use the multi-instance mode,
-open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using a symlink or
-bind-mount.
+If devpts is mounted without the 'newinstance' option (as in current
+start-up scripts) the new mount binds to the initial kernel mount of
+devpts. This mode is referred to as the 'single-instance' mode and the
+current, single-instance semantics are preserved, i.e PTYs are common
+across the system.
+
+If 'newinstance' option is specified, the mount is considered to be in
+the multi-instance mode and a new instance of the devpts fs is
+created. Any ptys created in this instance are independent of ptys in
+other instances of devpts. Like in the single-instance mode, the
+/dev/pts/ptmx node is present. To effectively use the multi-instance
+mode, open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using
+a symlink or bind-mount.
 
 Eg: A container startup script could do the following:
 
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 830cd62..5e5cd3e 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -109,17 +109,6 @@ config UNIX98_PTYS
 	  All modern Linux systems use the Unix98 ptys.  Say Y unless
 	  you're on an embedded system and want to conserve memory.
 
-config DEVPTS_MULTIPLE_INSTANCES
-	bool "Support multiple instances of devpts"
-	depends on UNIX98_PTYS
-	default n
-	---help---
-	  Enable support for multiple instances of devpts filesystem.
-	  If you want to have isolated PTY namespaces (eg: in containers),
-	  say Y here.  Otherwise, say N. If enabled, each mount of devpts
-	  filesystem with the '-o newinstance' option will create an
-	  independent PTY namespace.
-
 config LEGACY_PTYS
 	bool "Legacy (BSD) PTY support"
 	default y
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 14afbab..5f0d64e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -115,11 +115,9 @@ static const match_table_t tokens = {
 	{Opt_uid, "uid=%u"},
 	{Opt_gid, "gid=%u"},
 	{Opt_mode, "mode=%o"},
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	{Opt_ptmxmode, "ptmxmode=%o"},
 	{Opt_newinstance, "newinstance"},
 	{Opt_max, "max=%d"},
-#endif
 	{Opt_err, NULL}
 };
 
@@ -136,10 +134,8 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 
 static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 {
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
 		return inode->i_sb;
-#endif
 	return devpts_mnt->mnt_sb;
 }
 
@@ -206,7 +202,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 				return -EINVAL;
 			opts->mode = option & S_IALLUGO;
 			break;
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 		case Opt_ptmxmode:
 			if (match_octal(&args[0], &option))
 				return -EINVAL;
@@ -223,7 +218,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 				return -EINVAL;
 			opts->max = option;
 			break;
-#endif
 		default:
 			printk(KERN_ERR "devpts: called with bogus options\n");
 			return -EINVAL;
@@ -233,7 +227,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 	return 0;
 }
 
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 static int mknod_ptmx(struct super_block *sb)
 {
 	int mode;
@@ -291,12 +284,6 @@ static void update_ptmx_mode(struct pts_fs_info *fsi)
 		inode->i_mode = S_IFCHR|fsi->mount_opts.ptmxmode;
 	}
 }
-#else
-static inline void update_ptmx_mode(struct pts_fs_info *fsi)
-{
-       return;
-}
-#endif
 
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
@@ -327,11 +314,9 @@ static int devpts_show_options(struct seq_file *seq, struct dentry *root)
 	if (opts->setgid)
 		seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, opts->gid));
 	seq_printf(seq, ",mode=%03o", opts->mode);
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
 	if (opts->max < NR_UNIX98_PTY_MAX)
 		seq_printf(seq, ",max=%d", opts->max);
-#endif
 
 	return 0;
 }
@@ -392,7 +377,6 @@ fail:
 	return -ENOMEM;
 }
 
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 static int compare_init_pts_sb(struct super_block *s, void *p)
 {
 	if (devpts_mnt)
@@ -467,18 +451,6 @@ out_undo_sget:
 	return ERR_PTR(error);
 }
 
-#else
-/*
- * This supports only the legacy single-instance semantics (no
- * multiple-instance semantics)
- */
-static struct dentry *devpts_mount(struct file_system_type *fs_type, int flags,
-		const char *dev_name, void *data)
-{
-	return mount_single(fs_type, flags, data, devpts_fill_super);
-}
-#endif
-
 static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
-- 
1.7.5.4

[PATCH 2/4] devpts: Set the default permissions of /dev/pts/ptmx and /dev/ptmx to 0666

[Eric W. Biederman]

These are difference instances of the same device not so it only
makes sense that they would have the same default permissions.

Move the definitions of DEVPTS_DEEFAULT_PTMX_MODE and PTM_MINOR
into devpts_fs.h so we can use the same definitions throughout.

Acked-by: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 drivers/tty/pty.c         |    6 +++---
 drivers/tty/tty_io.c      |    5 +++--
 fs/devpts/inode.c         |    8 --------
 include/linux/devpts_fs.h |    3 +++
 4 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 5505ffc..6beb7e1 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -722,10 +722,10 @@ static void __init unix98_pty_init(void)
 	ptmx_fops.open = ptmx_open;
 
 	cdev_init(&ptmx_cdev, &ptmx_fops);
-	if (cdev_add(&ptmx_cdev, MKDEV(TTYAUX_MAJOR, 2), 1) ||
-	    register_chrdev_region(MKDEV(TTYAUX_MAJOR, 2), 1, "/dev/ptmx") < 0)
+	if (cdev_add(&ptmx_cdev, MKDEV(TTYAUX_MAJOR, PTMX_MINOR), 1) ||
+	    register_chrdev_region(MKDEV(TTYAUX_MAJOR, PTMX_MINOR), 1, "/dev/ptmx") < 0)
 		panic("Couldn't register /dev/ptmx driver\n");
-	device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 2), NULL, "ptmx");
+	device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, PTMX_MINOR), NULL, "ptmx");
 }
 
 #else
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..6c3a7c2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3314,9 +3314,10 @@ static char *tty_devnode(struct device *dev, umode_t *mode)
 {
 	if (!mode)
 		return NULL;
-	if (dev->devt == MKDEV(TTYAUX_MAJOR, 0) ||
-	    dev->devt == MKDEV(TTYAUX_MAJOR, 2))
+	if (dev->devt == MKDEV(TTYAUX_MAJOR, 0))
 		*mode = 0666;
+	if (dev->devt == MKDEV(TTYAUX_MAJOR, PTMX_MINOR))
+		*mode = DEVPTS_DEFAULT_PTMX_MODE;
 	return NULL;
 }
 
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 5f0d64e..61b54aa 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -27,14 +27,6 @@
 #include <linux/seq_file.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
-/*
- * ptmx is a new node in /dev/pts and will be unused in legacy (single-
- * instance) mode. To prevent surprises in user space, set permissions of
- * ptmx to 0. Use 'chmod' or remount with '-o ptmxmode' to set meaningful
- * permissions.
- */
-#define DEVPTS_DEFAULT_PTMX_MODE 0000
-#define PTMX_MINOR	2
 
 /*
  * sysctl support for setting limits on the number of Unix98 ptys allocated.
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 5ce0e5f..2d539ae 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -15,6 +15,9 @@
 
 #include <linux/errno.h>
 
+#define DEVPTS_DEFAULT_PTMX_MODE 0666
+#define PTMX_MINOR	2
+
 #ifdef CONFIG_UNIX98_PTYS
 
 int devpts_new_index(struct inode *ptmx_inode);
-- 
1.7.5.4

[PATCH 2/4] devpts: Set the default permissions of /dev/pts/ptmx and /dev/ptmx to 0666

[Eric W. Biederman]

These are difference instances of the same device not so it only
makes sense that they would have the same default permissions.

Move the definitions of DEVPTS_DEEFAULT_PTMX_MODE and PTM_MINOR
into devpts_fs.h so we can use the same definitions throughout.

Acked-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 drivers/tty/pty.c         |    6 +++---
 drivers/tty/tty_io.c      |    5 +++--
 fs/devpts/inode.c         |    8 --------
 include/linux/devpts_fs.h |    3 +++
 4 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 5505ffc..6beb7e1 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -722,10 +722,10 @@ static void __init unix98_pty_init(void)
 	ptmx_fops.open = ptmx_open;
 
 	cdev_init(&ptmx_cdev, &ptmx_fops);
-	if (cdev_add(&ptmx_cdev, MKDEV(TTYAUX_MAJOR, 2), 1) ||
-	    register_chrdev_region(MKDEV(TTYAUX_MAJOR, 2), 1, "/dev/ptmx") < 0)
+	if (cdev_add(&ptmx_cdev, MKDEV(TTYAUX_MAJOR, PTMX_MINOR), 1) ||
+	    register_chrdev_region(MKDEV(TTYAUX_MAJOR, PTMX_MINOR), 1, "/dev/ptmx") < 0)
 		panic("Couldn't register /dev/ptmx driver\n");
-	device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 2), NULL, "ptmx");
+	device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, PTMX_MINOR), NULL, "ptmx");
 }
 
 #else
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..6c3a7c2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3314,9 +3314,10 @@ static char *tty_devnode(struct device *dev, umode_t *mode)
 {
 	if (!mode)
 		return NULL;
-	if (dev->devt == MKDEV(TTYAUX_MAJOR, 0) ||
-	    dev->devt == MKDEV(TTYAUX_MAJOR, 2))
+	if (dev->devt == MKDEV(TTYAUX_MAJOR, 0))
 		*mode = 0666;
+	if (dev->devt == MKDEV(TTYAUX_MAJOR, PTMX_MINOR))
+		*mode = DEVPTS_DEFAULT_PTMX_MODE;
 	return NULL;
 }
 
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 5f0d64e..61b54aa 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -27,14 +27,6 @@
 #include <linux/seq_file.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
-/*
- * ptmx is a new node in /dev/pts and will be unused in legacy (single-
- * instance) mode. To prevent surprises in user space, set permissions of
- * ptmx to 0. Use 'chmod' or remount with '-o ptmxmode' to set meaningful
- * permissions.
- */
-#define DEVPTS_DEFAULT_PTMX_MODE 0000
-#define PTMX_MINOR	2
 
 /*
  * sysctl support for setting limits on the number of Unix98 ptys allocated.
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 5ce0e5f..2d539ae 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -15,6 +15,9 @@
 
 #include <linux/errno.h>
 
+#define DEVPTS_DEFAULT_PTMX_MODE 0666
+#define PTMX_MINOR	2
+
 #ifdef CONFIG_UNIX98_PTYS
 
 int devpts_new_index(struct inode *ptmx_inode);
-- 
1.7.5.4

[PATCH 3/4] devpts: Make the newinstance option historical

[Eric W. Biederman]

- Modify opening /dev/ptmx so that it opens /dev/pts/ptmx
  As it appares nearly impossible to get udev to create a symlink
  do this with a little sprinkle of in kernel magic.
- Cleanup the devpts mount code and only leave the code for
  new instance.

Tested on debian and ubuntu without any problems.

Acked-by: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 drivers/tty/pty.c         |    4 +
 fs/devpts/inode.c         |  180 +++++++++++++++++----------------------------
 include/linux/devpts_fs.h |    5 +
 3 files changed, 77 insertions(+), 112 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 6beb7e1..a605074 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -615,6 +615,10 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	int retval;
 	int index;
 
+	inode = devpts_redirect(filp);
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
+
 	nonseekable_open(inode, filp);
 
 	retval = tty_alloc_file(filp);
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 61b54aa..38136ae 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -25,6 +25,7 @@
 #include <linux/parser.h>
 #include <linux/fsnotify.h>
 #include <linux/seq_file.h>
+#include <linux/file.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
 
@@ -94,7 +95,6 @@ struct pts_mount_opts {
 	kgid_t   gid;
 	umode_t mode;
 	umode_t ptmxmode;
-	int newinstance;
 	int max;
 };
 
@@ -116,7 +116,6 @@ static const match_table_t tokens = {
 struct pts_fs_info {
 	struct ida allocated_ptys;
 	struct pts_mount_opts mount_opts;
-	struct dentry *ptmx_dentry;
 };
 
 static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
@@ -157,10 +156,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
 	opts->max     = NR_UNIX98_PTY_MAX;
 
-	/* newinstance makes sense only on initial mount */
-	if (op == PARSE_MOUNT)
-		opts->newinstance = 0;
-
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
 		int token;
@@ -200,9 +195,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 			opts->ptmxmode = option & S_IALLUGO;
 			break;
 		case Opt_newinstance:
-			/* newinstance makes sense only on initial mount */
-			if (op == PARSE_MOUNT)
-				opts->newinstance = 1;
+			/* Historical */
 			break;
 		case Opt_max:
 			if (match_int(&args[0], &option) ||
@@ -229,14 +222,6 @@ static int mknod_ptmx(struct super_block *sb)
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	mutex_lock(&root->d_inode->i_mutex);
-
-	/* If we have already created ptmx node, return */
-	if (fsi->ptmx_dentry) {
-		rc = 0;
-		goto out;
-	}
-
 	dentry = d_alloc_name(root, "ptmx");
 	if (!dentry) {
 		printk(KERN_NOTICE "Unable to alloc dentry for ptmx node\n");
@@ -261,39 +246,17 @@ static int mknod_ptmx(struct super_block *sb)
 
 	d_add(dentry, inode);
 
-	fsi->ptmx_dentry = dentry;
 	rc = 0;
 out:
-	mutex_unlock(&root->d_inode->i_mutex);
 	return rc;
 }
 
-static void update_ptmx_mode(struct pts_fs_info *fsi)
-{
-	struct inode *inode;
-	if (fsi->ptmx_dentry) {
-		inode = fsi->ptmx_dentry->d_inode;
-		inode->i_mode = S_IFCHR|fsi->mount_opts.ptmxmode;
-	}
-}
-
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
-	int err;
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	err = parse_mount_options(data, PARSE_REMOUNT, opts);
-
-	/*
-	 * parse_mount_options() restores options to default values
-	 * before parsing and may have changed ptmxmode. So, update the
-	 * mode in the inode too. Bogus options don't fail the remount,
-	 * so do this even on error return.
-	 */
-	update_ptmx_mode(fsi);
-
-	return err;
+	return  parse_mount_options(data, PARSE_REMOUNT, opts);
 }
 
 static int devpts_show_options(struct seq_file *seq, struct dentry *root)
@@ -319,7 +282,7 @@ static const struct super_operations devpts_sops = {
 	.show_options	= devpts_show_options,
 };
 
-static void *new_pts_fs_info(void)
+static struct pts_fs_info *new_pts_fs_info(void)
 {
 	struct pts_fs_info *fsi;
 
@@ -338,6 +301,8 @@ static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
 	struct inode *inode;
+	struct pts_fs_info *fsi;
+	int error = -ENOMEM;
 
 	s->s_blocksize = 1024;
 	s->s_blocksize_bits = 10;
@@ -345,13 +310,18 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &devpts_sops;
 	s->s_time_gran = 1;
 
-	s->s_fs_info = new_pts_fs_info();
-	if (!s->s_fs_info)
+	fsi = new_pts_fs_info();
+	s->s_fs_info = fsi;
+	if (!fsi)
 		goto fail;
 
+	error = parse_mount_options(data, PARSE_MOUNT, &fsi->mount_opts);
+	if (error)
+		goto fail_kfree;
+
 	inode = new_inode(s);
 	if (!inode)
-		goto fail;
+		goto fail_kfree;
 	inode->i_ino = 1;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_mode = S_IFDIR | S_IRUGO | S_IXUGO | S_IWUSR;
@@ -360,87 +330,40 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	set_nlink(inode, 2);
 
 	s->s_root = d_make_root(inode);
-	if (s->s_root)
-		return 0;
+	if (!s->s_root) {
+		printk(KERN_ERR "devpts: get root dentry failed\n");
+		goto fail_iput;
+	}
 
-	printk(KERN_ERR "devpts: get root dentry failed\n");
+	error = mknod_ptmx(s);
+	if (error)
+		goto fail_put_root;
+
+	return 0;
 
+fail_put_root:
+	dput(s->s_root);
+	s->s_root = NULL;
+fail_iput:
+	iput(inode);
+fail_kfree:
+	kfree(fsi);
 fail:
 	return -ENOMEM;
 }
 
-static int compare_init_pts_sb(struct super_block *s, void *p)
-{
-	if (devpts_mnt)
-		return devpts_mnt->mnt_sb == s;
-	return 0;
-}
 
 /*
  * devpts_mount()
  *
- *     If the '-o newinstance' mount option was specified, mount a new
- *     (private) instance of devpts.  PTYs created in this instance are
- *     independent of the PTYs in other devpts instances.
- *
- *     If the '-o newinstance' option was not specified, mount/remount the
- *     initial kernel mount of devpts.  This type of mount gives the
- *     legacy, single-instance semantics.
- *
- *     The 'newinstance' option is needed to support multiple namespace
- *     semantics in devpts while preserving backward compatibility of the
- *     current 'single-namespace' semantics. i.e all mounts of devpts
- *     without the 'newinstance' mount option should bind to the initial
- *     kernel mount, like mount_single().
- *
- *     Mounts with 'newinstance' option create a new, private namespace.
+ *     Mount a new (private) instance of devpts.  PTYs created in this
+ *     instance are independent of the PTYs in other devpts instances.
  *
- *     NOTE:
- *
- *     For single-mount semantics, devpts cannot use mount_single(),
- *     because mount_single()/sget() find and use the super-block from
- *     the most recent mount of devpts. But that recent mount may be a
- *     'newinstance' mount and mount_single() would pick the newinstance
- *     super-block instead of the initial super-block.
  */
 static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
-	int error;
-	struct pts_mount_opts opts;
-	struct super_block *s;
-
-	error = parse_mount_options(data, PARSE_MOUNT, &opts);
-	if (error)
-		return ERR_PTR(error);
-
-	if (opts.newinstance)
-		s = sget(fs_type, NULL, set_anon_super, flags, NULL);
-	else
-		s = sget(fs_type, compare_init_pts_sb, set_anon_super, flags,
-			 NULL);
-
-	if (IS_ERR(s))
-		return ERR_CAST(s);
-
-	if (!s->s_root) {
-		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
-		if (error)
-			goto out_undo_sget;
-		s->s_flags |= MS_ACTIVE;
-	}
-
-	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
-
-	error = mknod_ptmx(s);
-	if (error)
-		goto out_undo_sget;
-
-	return dget(s->s_root);
-
-out_undo_sget:
-	deactivate_locked_super(s);
-	return ERR_PTR(error);
+	return mount_nodev(fs_type, flags, data, devpts_fill_super);
 }
 
 static void devpts_kill_sb(struct super_block *sb)
@@ -457,6 +380,40 @@ static struct file_system_type devpts_fs_type = {
 	.kill_sb	= devpts_kill_sb,
 };
 
+struct inode *devpts_redirect(struct file *filp)
+{
+	struct inode *inode;
+	struct file *filp2;
+
+	/* Is the inode already a devpts inode? */
+	inode = filp->f_dentry->d_inode;
+	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
+		goto out;
+
+	/* Is f_dentry->d_parent usable? */
+	inode = ERR_PTR(-ENODEV);
+	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
+		goto out;
+
+	/* Is there a devpts inode we can use instead? */
+	
+	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
+			       "pts/ptmx", O_PATH);
+	if (!IS_ERR(filp2)) {
+		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
+			struct path old;
+			old = filp->f_path;
+			filp->f_path = filp2->f_path;
+			inode = filp->f_dentry->d_inode;
+			path_get(&filp->f_path);
+			path_put(&old);
+		}
+		fput(filp2);
+	}
+out:
+	return inode;
+}
+
 /*
  * The normal naming convention is simply /dev/pts/<number>; this conforms
  * to the System V naming convention
@@ -474,8 +431,7 @@ retry:
 		return -ENOMEM;
 
 	mutex_lock(&allocated_ptys_lock);
-	if (pty_count >= pty_limit -
-			(fsi->mount_opts.newinstance ? pty_reserve : 0)) {
+	if (pty_count >= pty_limit - pty_reserve) {
 		mutex_unlock(&allocated_ptys_lock);
 		return -ENOSPC;
 	}
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 2d539ae..fe2b41f 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -20,6 +20,7 @@
 
 #ifdef CONFIG_UNIX98_PTYS
 
+struct inode *devpts_redirect(struct file *filp);
 int devpts_new_index(struct inode *ptmx_inode);
 void devpts_kill_index(struct inode *ptmx_inode, int idx);
 /* mknod in devpts */
@@ -32,6 +33,10 @@ void devpts_pty_kill(struct tty_struct *tty);
 #else
 
 /* Dummy stubs in the no-pty case */
+static inline struct inode *devpts_redirect(struct file *filp)
+{
+	return ERR_PTR(-ENODEV);
+}
 static inline int devpts_new_index(struct inode *ptmx_inode) { return -EINVAL; }
 static inline void devpts_kill_index(struct inode *ptmx_inode, int idx) { }
 static inline int devpts_pty_new(struct inode *ptmx_inode,
-- 
1.7.5.4

[PATCH 3/4] devpts: Make the newinstance option historical

[Eric W. Biederman]

- Modify opening /dev/ptmx so that it opens /dev/pts/ptmx
  As it appares nearly impossible to get udev to create a symlink
  do this with a little sprinkle of in kernel magic.
- Cleanup the devpts mount code and only leave the code for
  new instance.

Tested on debian and ubuntu without any problems.

Acked-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 drivers/tty/pty.c         |    4 +
 fs/devpts/inode.c         |  180 +++++++++++++++++----------------------------
 include/linux/devpts_fs.h |    5 +
 3 files changed, 77 insertions(+), 112 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 6beb7e1..a605074 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -615,6 +615,10 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	int retval;
 	int index;
 
+	inode = devpts_redirect(filp);
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
+
 	nonseekable_open(inode, filp);
 
 	retval = tty_alloc_file(filp);
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 61b54aa..38136ae 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -25,6 +25,7 @@
 #include <linux/parser.h>
 #include <linux/fsnotify.h>
 #include <linux/seq_file.h>
+#include <linux/file.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
 
@@ -94,7 +95,6 @@ struct pts_mount_opts {
 	kgid_t   gid;
 	umode_t mode;
 	umode_t ptmxmode;
-	int newinstance;
 	int max;
 };
 
@@ -116,7 +116,6 @@ static const match_table_t tokens = {
 struct pts_fs_info {
 	struct ida allocated_ptys;
 	struct pts_mount_opts mount_opts;
-	struct dentry *ptmx_dentry;
 };
 
 static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
@@ -157,10 +156,6 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
 	opts->max     = NR_UNIX98_PTY_MAX;
 
-	/* newinstance makes sense only on initial mount */
-	if (op == PARSE_MOUNT)
-		opts->newinstance = 0;
-
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
 		int token;
@@ -200,9 +195,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 			opts->ptmxmode = option & S_IALLUGO;
 			break;
 		case Opt_newinstance:
-			/* newinstance makes sense only on initial mount */
-			if (op == PARSE_MOUNT)
-				opts->newinstance = 1;
+			/* Historical */
 			break;
 		case Opt_max:
 			if (match_int(&args[0], &option) ||
@@ -229,14 +222,6 @@ static int mknod_ptmx(struct super_block *sb)
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	mutex_lock(&root->d_inode->i_mutex);
-
-	/* If we have already created ptmx node, return */
-	if (fsi->ptmx_dentry) {
-		rc = 0;
-		goto out;
-	}
-
 	dentry = d_alloc_name(root, "ptmx");
 	if (!dentry) {
 		printk(KERN_NOTICE "Unable to alloc dentry for ptmx node\n");
@@ -261,39 +246,17 @@ static int mknod_ptmx(struct super_block *sb)
 
 	d_add(dentry, inode);
 
-	fsi->ptmx_dentry = dentry;
 	rc = 0;
 out:
-	mutex_unlock(&root->d_inode->i_mutex);
 	return rc;
 }
 
-static void update_ptmx_mode(struct pts_fs_info *fsi)
-{
-	struct inode *inode;
-	if (fsi->ptmx_dentry) {
-		inode = fsi->ptmx_dentry->d_inode;
-		inode->i_mode = S_IFCHR|fsi->mount_opts.ptmxmode;
-	}
-}
-
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
-	int err;
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	err = parse_mount_options(data, PARSE_REMOUNT, opts);
-
-	/*
-	 * parse_mount_options() restores options to default values
-	 * before parsing and may have changed ptmxmode. So, update the
-	 * mode in the inode too. Bogus options don't fail the remount,
-	 * so do this even on error return.
-	 */
-	update_ptmx_mode(fsi);
-
-	return err;
+	return  parse_mount_options(data, PARSE_REMOUNT, opts);
 }
 
 static int devpts_show_options(struct seq_file *seq, struct dentry *root)
@@ -319,7 +282,7 @@ static const struct super_operations devpts_sops = {
 	.show_options	= devpts_show_options,
 };
 
-static void *new_pts_fs_info(void)
+static struct pts_fs_info *new_pts_fs_info(void)
 {
 	struct pts_fs_info *fsi;
 
@@ -338,6 +301,8 @@ static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
 	struct inode *inode;
+	struct pts_fs_info *fsi;
+	int error = -ENOMEM;
 
 	s->s_blocksize = 1024;
 	s->s_blocksize_bits = 10;
@@ -345,13 +310,18 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &devpts_sops;
 	s->s_time_gran = 1;
 
-	s->s_fs_info = new_pts_fs_info();
-	if (!s->s_fs_info)
+	fsi = new_pts_fs_info();
+	s->s_fs_info = fsi;
+	if (!fsi)
 		goto fail;
 
+	error = parse_mount_options(data, PARSE_MOUNT, &fsi->mount_opts);
+	if (error)
+		goto fail_kfree;
+
 	inode = new_inode(s);
 	if (!inode)
-		goto fail;
+		goto fail_kfree;
 	inode->i_ino = 1;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_mode = S_IFDIR | S_IRUGO | S_IXUGO | S_IWUSR;
@@ -360,87 +330,40 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	set_nlink(inode, 2);
 
 	s->s_root = d_make_root(inode);
-	if (s->s_root)
-		return 0;
+	if (!s->s_root) {
+		printk(KERN_ERR "devpts: get root dentry failed\n");
+		goto fail_iput;
+	}
 
-	printk(KERN_ERR "devpts: get root dentry failed\n");
+	error = mknod_ptmx(s);
+	if (error)
+		goto fail_put_root;
+
+	return 0;
 
+fail_put_root:
+	dput(s->s_root);
+	s->s_root = NULL;
+fail_iput:
+	iput(inode);
+fail_kfree:
+	kfree(fsi);
 fail:
 	return -ENOMEM;
 }
 
-static int compare_init_pts_sb(struct super_block *s, void *p)
-{
-	if (devpts_mnt)
-		return devpts_mnt->mnt_sb == s;
-	return 0;
-}
 
 /*
  * devpts_mount()
  *
- *     If the '-o newinstance' mount option was specified, mount a new
- *     (private) instance of devpts.  PTYs created in this instance are
- *     independent of the PTYs in other devpts instances.
- *
- *     If the '-o newinstance' option was not specified, mount/remount the
- *     initial kernel mount of devpts.  This type of mount gives the
- *     legacy, single-instance semantics.
- *
- *     The 'newinstance' option is needed to support multiple namespace
- *     semantics in devpts while preserving backward compatibility of the
- *     current 'single-namespace' semantics. i.e all mounts of devpts
- *     without the 'newinstance' mount option should bind to the initial
- *     kernel mount, like mount_single().
- *
- *     Mounts with 'newinstance' option create a new, private namespace.
+ *     Mount a new (private) instance of devpts.  PTYs created in this
+ *     instance are independent of the PTYs in other devpts instances.
  *
- *     NOTE:
- *
- *     For single-mount semantics, devpts cannot use mount_single(),
- *     because mount_single()/sget() find and use the super-block from
- *     the most recent mount of devpts. But that recent mount may be a
- *     'newinstance' mount and mount_single() would pick the newinstance
- *     super-block instead of the initial super-block.
  */
 static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
-	int error;
-	struct pts_mount_opts opts;
-	struct super_block *s;
-
-	error = parse_mount_options(data, PARSE_MOUNT, &opts);
-	if (error)
-		return ERR_PTR(error);
-
-	if (opts.newinstance)
-		s = sget(fs_type, NULL, set_anon_super, flags, NULL);
-	else
-		s = sget(fs_type, compare_init_pts_sb, set_anon_super, flags,
-			 NULL);
-
-	if (IS_ERR(s))
-		return ERR_CAST(s);
-
-	if (!s->s_root) {
-		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
-		if (error)
-			goto out_undo_sget;
-		s->s_flags |= MS_ACTIVE;
-	}
-
-	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
-
-	error = mknod_ptmx(s);
-	if (error)
-		goto out_undo_sget;
-
-	return dget(s->s_root);
-
-out_undo_sget:
-	deactivate_locked_super(s);
-	return ERR_PTR(error);
+	return mount_nodev(fs_type, flags, data, devpts_fill_super);
 }
 
 static void devpts_kill_sb(struct super_block *sb)
@@ -457,6 +380,40 @@ static struct file_system_type devpts_fs_type = {
 	.kill_sb	= devpts_kill_sb,
 };
 
+struct inode *devpts_redirect(struct file *filp)
+{
+	struct inode *inode;
+	struct file *filp2;
+
+	/* Is the inode already a devpts inode? */
+	inode = filp->f_dentry->d_inode;
+	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
+		goto out;
+
+	/* Is f_dentry->d_parent usable? */
+	inode = ERR_PTR(-ENODEV);
+	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
+		goto out;
+
+	/* Is there a devpts inode we can use instead? */
+	
+	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
+			       "pts/ptmx", O_PATH);
+	if (!IS_ERR(filp2)) {
+		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
+			struct path old;
+			old = filp->f_path;
+			filp->f_path = filp2->f_path;
+			inode = filp->f_dentry->d_inode;
+			path_get(&filp->f_path);
+			path_put(&old);
+		}
+		fput(filp2);
+	}
+out:
+	return inode;
+}
+
 /*
  * The normal naming convention is simply /dev/pts/<number>; this conforms
  * to the System V naming convention
@@ -474,8 +431,7 @@ retry:
 		return -ENOMEM;
 
 	mutex_lock(&allocated_ptys_lock);
-	if (pty_count >= pty_limit -
-			(fsi->mount_opts.newinstance ? pty_reserve : 0)) {
+	if (pty_count >= pty_limit - pty_reserve) {
 		mutex_unlock(&allocated_ptys_lock);
 		return -ENOSPC;
 	}
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 2d539ae..fe2b41f 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -20,6 +20,7 @@
 
 #ifdef CONFIG_UNIX98_PTYS
 
+struct inode *devpts_redirect(struct file *filp);
 int devpts_new_index(struct inode *ptmx_inode);
 void devpts_kill_index(struct inode *ptmx_inode, int idx);
 /* mknod in devpts */
@@ -32,6 +33,10 @@ void devpts_pty_kill(struct tty_struct *tty);
 #else
 
 /* Dummy stubs in the no-pty case */
+static inline struct inode *devpts_redirect(struct file *filp)
+{
+	return ERR_PTR(-ENODEV);
+}
 static inline int devpts_new_index(struct inode *ptmx_inode) { return -EINVAL; }
 static inline void devpts_kill_index(struct inode *ptmx_inode, int idx) { }
 static inline int devpts_pty_new(struct inode *ptmx_inode,
-- 
1.7.5.4

[PATCH 4/4] devpts: Update the documentation

[Eric W. Biederman]

Document the updated state of devpts.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 Documentation/filesystems/devpts.txt |   88 ++++++++-------------------------
 1 files changed, 22 insertions(+), 66 deletions(-)

diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
index 260cf6d..02f2806 100644
--- a/Documentation/filesystems/devpts.txt
+++ b/Documentation/filesystems/devpts.txt
@@ -3,26 +3,17 @@ To support containers, we now allow multiple instances of devpts filesystem,
 such that indices of ptys allocated in one instance are independent of indices
 allocated in other instances of devpts.
 
-To preserve backward compatibility, this support for multiple instances is
-enabled only if:
-
-	- '-o newinstance' mount option is specified while mounting devpts
-
-IOW, devpts now supports both single-instance and multi-instance semantics.
-
-If devpts is mounted without the 'newinstance' option (as in current
-start-up scripts) the new mount binds to the initial kernel mount of
-devpts. This mode is referred to as the 'single-instance' mode and the
-current, single-instance semantics are preserved, i.e PTYs are common
-across the system.
-
-If 'newinstance' option is specified, the mount is considered to be in
-the multi-instance mode and a new instance of the devpts fs is
-created. Any ptys created in this instance are independent of ptys in
-other instances of devpts. Like in the single-instance mode, the
-/dev/pts/ptmx node is present. To effectively use the multi-instance
-mode, open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using
-a symlink or bind-mount.
+Initially devpts would only support creating multiple instances of itself
+when passed the newinstance mount option.  The newinstance mount option was
+found to be unnecessary and is now recognized but ignored because all mounts
+of devpts create a new instance devpts.
+
+when devpts is mounted a new instance of the devpts fs is created. Any
+ptys created in this instance are independent of ptys in other
+instances of devpts.  The /dev/pts/ptmx node is present.  Opening the
+classic /dev/ptmx device node redirects to /dev/pts/ptmx (if present).
+Creating /dev/ptmx as a symlink to or bind mount of /dev/pts/ptmx is
+preferable to make it explicit what is going on.
 
 Eg: A container startup script could do the following:
 
@@ -44,14 +35,13 @@ the original mount of /dev/pts.
 User-space changes
 ------------------
 
-In multi-instance mode (i.e '-o newinstance' mount option is specified at least
-once), following user-space issues should be noted.
+The following user-space issues should be noted.
 
-1. If -o newinstance mount option is never used, /dev/pts/ptmx can be ignored
+1. If devpts is only mounted once on /dev/pts /dev/pts/ptmx can be ignored
    and no change is needed to system-startup scripts.
 
-2. To effectively use multi-instance mode (i.e -o newinstance is specified)
-   administrators or startup scripts should "redirect" open of /dev/ptmx to
+2. To effectively use multiple instances of devpts administrators or
+   startup scripts should "redirect" open of /dev/ptmx to
    /dev/pts/ptmx using either a bind mount or symlink.
 
 	$ mount -t devpts -o newinstance devpts /dev/pts
@@ -64,21 +54,17 @@ once), following user-space issues should be noted.
    or
 	$ mount -o bind /dev/pts/ptmx /dev/ptmx
 
+   Opening of ptmx attempts to remove the need for this by acting
+   like a relative symlink to pts/ptmx but this does not work if
+   devpts and /dev/ptmx are not in the typical locations.
+
 3. The '/dev/ptmx -> pts/ptmx' symlink is the preferred method since it
    enables better error-reporting and treats both single-instance and
    multi-instance mounts similarly.
 
-   But this method requires that system-startup scripts set the mode of
-   /dev/pts/ptmx correctly (default mode is 0000). The scripts can set the
-   mode by, either
-
-   	- adding ptmxmode mount option to devpts entry in /etc/fstab, or
-	- using 'chmod 0666 /dev/pts/ptmx'
-
-4. If multi-instance mode mount is needed for containers, but the system
-   startup scripts have not yet been updated, container-startup scripts
-   should bind mount /dev/ptmx to /dev/pts/ptmx to avoid breaking single-
-   instance mounts.
+4. If the system startup scripts have not yet been updated,
+   container-startup scripts should bind mount /dev/ptmx to
+   /dev/pts/ptmx.
 
    Or, in general, container-startup scripts should use:
 
@@ -90,33 +76,3 @@ once), following user-space issues should be noted.
    When all devpts mounts are multi-instance, /dev/ptmx can permanently be
    a symlink to pts/ptmx and the bind mount can be ignored.
 
-5. A multi-instance mount that is not accompanied by the /dev/ptmx to
-   /dev/pts/ptmx redirection would result in an unusable/unreachable pty.
-
-	mount -t devpts -o newinstance lxcpts /dev/pts
-
-   immediately followed by:
-
-	open("/dev/ptmx")
-
-    would create a pty, say /dev/pts/7, in the initial kernel mount.
-    But /dev/pts/7 would be invisible in the new mount.
-
-6. The permissions for /dev/pts/ptmx node should be specified when mounting
-   /dev/pts, using the '-o ptmxmode=%o' mount option (default is 0000).
-
-	mount -t devpts -o newinstance -o ptmxmode=0644 devpts /dev/pts
-
-   The permissions can be later be changed as usual with 'chmod'.
-
-	chmod 666 /dev/pts/ptmx
-
-7. A mount of devpts without the 'newinstance' option results in binding to
-   initial kernel mount.  This behavior while preserving legacy semantics,
-   does not provide strict isolation in a container environment. i.e by
-   mounting devpts without the 'newinstance' option, a container could
-   get visibility into the 'host' or root container's devpts.
-   
-   To workaround this and have strict isolation, all mounts of devpts,
-   including the mount in the root container, should use the newinstance
-   option.
-- 
1.7.5.4

[PATCH 4/4] devpts: Update the documentation

[Eric W. Biederman]

Document the updated state of devpts.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 Documentation/filesystems/devpts.txt |   88 ++++++++-------------------------
 1 files changed, 22 insertions(+), 66 deletions(-)

diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
index 260cf6d..02f2806 100644
--- a/Documentation/filesystems/devpts.txt
+++ b/Documentation/filesystems/devpts.txt
@@ -3,26 +3,17 @@ To support containers, we now allow multiple instances of devpts filesystem,
 such that indices of ptys allocated in one instance are independent of indices
 allocated in other instances of devpts.
 
-To preserve backward compatibility, this support for multiple instances is
-enabled only if:
-
-	- '-o newinstance' mount option is specified while mounting devpts
-
-IOW, devpts now supports both single-instance and multi-instance semantics.
-
-If devpts is mounted without the 'newinstance' option (as in current
-start-up scripts) the new mount binds to the initial kernel mount of
-devpts. This mode is referred to as the 'single-instance' mode and the
-current, single-instance semantics are preserved, i.e PTYs are common
-across the system.
-
-If 'newinstance' option is specified, the mount is considered to be in
-the multi-instance mode and a new instance of the devpts fs is
-created. Any ptys created in this instance are independent of ptys in
-other instances of devpts. Like in the single-instance mode, the
-/dev/pts/ptmx node is present. To effectively use the multi-instance
-mode, open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using
-a symlink or bind-mount.
+Initially devpts would only support creating multiple instances of itself
+when passed the newinstance mount option.  The newinstance mount option was
+found to be unnecessary and is now recognized but ignored because all mounts
+of devpts create a new instance devpts.
+
+when devpts is mounted a new instance of the devpts fs is created. Any
+ptys created in this instance are independent of ptys in other
+instances of devpts.  The /dev/pts/ptmx node is present.  Opening the
+classic /dev/ptmx device node redirects to /dev/pts/ptmx (if present).
+Creating /dev/ptmx as a symlink to or bind mount of /dev/pts/ptmx is
+preferable to make it explicit what is going on.
 
 Eg: A container startup script could do the following:
 
@@ -44,14 +35,13 @@ the original mount of /dev/pts.
 User-space changes
 ------------------
 
-In multi-instance mode (i.e '-o newinstance' mount option is specified at least
-once), following user-space issues should be noted.
+The following user-space issues should be noted.
 
-1. If -o newinstance mount option is never used, /dev/pts/ptmx can be ignored
+1. If devpts is only mounted once on /dev/pts /dev/pts/ptmx can be ignored
    and no change is needed to system-startup scripts.
 
-2. To effectively use multi-instance mode (i.e -o newinstance is specified)
-   administrators or startup scripts should "redirect" open of /dev/ptmx to
+2. To effectively use multiple instances of devpts administrators or
+   startup scripts should "redirect" open of /dev/ptmx to
    /dev/pts/ptmx using either a bind mount or symlink.
 
 	$ mount -t devpts -o newinstance devpts /dev/pts
@@ -64,21 +54,17 @@ once), following user-space issues should be noted.
    or
 	$ mount -o bind /dev/pts/ptmx /dev/ptmx
 
+   Opening of ptmx attempts to remove the need for this by acting
+   like a relative symlink to pts/ptmx but this does not work if
+   devpts and /dev/ptmx are not in the typical locations.
+
 3. The '/dev/ptmx -> pts/ptmx' symlink is the preferred method since it
    enables better error-reporting and treats both single-instance and
    multi-instance mounts similarly.
 
-   But this method requires that system-startup scripts set the mode of
-   /dev/pts/ptmx correctly (default mode is 0000). The scripts can set the
-   mode by, either
-
-   	- adding ptmxmode mount option to devpts entry in /etc/fstab, or
-	- using 'chmod 0666 /dev/pts/ptmx'
-
-4. If multi-instance mode mount is needed for containers, but the system
-   startup scripts have not yet been updated, container-startup scripts
-   should bind mount /dev/ptmx to /dev/pts/ptmx to avoid breaking single-
-   instance mounts.
+4. If the system startup scripts have not yet been updated,
+   container-startup scripts should bind mount /dev/ptmx to
+   /dev/pts/ptmx.
 
    Or, in general, container-startup scripts should use:
 
@@ -90,33 +76,3 @@ once), following user-space issues should be noted.
    When all devpts mounts are multi-instance, /dev/ptmx can permanently be
    a symlink to pts/ptmx and the bind mount can be ignored.
 
-5. A multi-instance mount that is not accompanied by the /dev/ptmx to
-   /dev/pts/ptmx redirection would result in an unusable/unreachable pty.
-
-	mount -t devpts -o newinstance lxcpts /dev/pts
-
-   immediately followed by:
-
-	open("/dev/ptmx")
-
-    would create a pty, say /dev/pts/7, in the initial kernel mount.
-    But /dev/pts/7 would be invisible in the new mount.
-
-6. The permissions for /dev/pts/ptmx node should be specified when mounting
-   /dev/pts, using the '-o ptmxmode=%o' mount option (default is 0000).
-
-	mount -t devpts -o newinstance -o ptmxmode=0644 devpts /dev/pts
-
-   The permissions can be later be changed as usual with 'chmod'.
-
-	chmod 666 /dev/pts/ptmx
-
-7. A mount of devpts without the 'newinstance' option results in binding to
-   initial kernel mount.  This behavior while preserving legacy semantics,
-   does not provide strict isolation in a container environment. i.e by
-   mounting devpts without the 'newinstance' option, a container could
-   get visibility into the 'host' or root container's devpts.
-   
-   To workaround this and have strict isolation, all mounts of devpts,
-   including the mount in the root container, should use the newinstance
-   option.
-- 
1.7.5.4

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sat, Sep 22, 2012 at 08:50:44PM -0700, Eric W. Biederman wrote:

> +struct inode *devpts_redirect(struct file *filp)
> +{
> +	struct inode *inode;
> +	struct file *filp2;
> +
> +	/* Is the inode already a devpts inode? */
> +	inode = filp->f_dentry->d_inode;
> +	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
> +		goto out;
> +
> +	/* Is f_dentry->d_parent usable? */
> +	inode = ERR_PTR(-ENODEV);
> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> +		goto out;
> +
> +	/* Is there a devpts inode we can use instead? */
> +	
> +	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
> +			       "pts/ptmx", O_PATH);
> +	if (!IS_ERR(filp2)) {
> +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
> +			struct path old;
> +			old = filp->f_path;
> +			filp->f_path = filp2->f_path;
> +			inode = filp->f_dentry->d_inode;
> +			path_get(&filp->f_path);
> +			path_put(&old);

You are welcome to supply an analysis of the reasons why ->open() pulling
such tricks will not break all kinds of code in VFS.
> +		}
> +		fput(filp2);

... starting with "what happens when some joker binds /dev/ptmx on
/dev/pts/ptmx"

NAK.  This violates asserts made by VFS (namely, that ->f_path is not
changed since dentry_open() has set it and until __fput() rips the thing
out) *and* by your own code (attack mentioned above, just from looking
at it for a minute).  Far too brittle...

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sat, Sep 22, 2012 at 08:50:44PM -0700, Eric W. Biederman wrote:

> +struct inode *devpts_redirect(struct file *filp)
> +{
> +	struct inode *inode;
> +	struct file *filp2;
> +
> +	/* Is the inode already a devpts inode? */
> +	inode = filp->f_dentry->d_inode;
> +	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
> +		goto out;
> +
> +	/* Is f_dentry->d_parent usable? */
> +	inode = ERR_PTR(-ENODEV);
> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> +		goto out;
> +
> +	/* Is there a devpts inode we can use instead? */
> +	
> +	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
> +			       "pts/ptmx", O_PATH);
> +	if (!IS_ERR(filp2)) {
> +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
> +			struct path old;
> +			old = filp->f_path;
> +			filp->f_path = filp2->f_path;
> +			inode = filp->f_dentry->d_inode;
> +			path_get(&filp->f_path);
> +			path_put(&old);

You are welcome to supply an analysis of the reasons why ->open() pulling
such tricks will not break all kinds of code in VFS.
> +		}
> +		fput(filp2);

... starting with "what happens when some joker binds /dev/ptmx on
/dev/pts/ptmx"

NAK.  This violates asserts made by VFS (namely, that ->f_path is not
changed since dentry_open() has set it and until __fput() rips the thing
out) *and* by your own code (attack mentioned above, just from looking
at it for a minute).  Far too brittle...

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sun, Sep 23, 2012 at 05:19:06AM +0100, Al Viro wrote:
> On Sat, Sep 22, 2012 at 08:50:44PM -0700, Eric W. Biederman wrote:
> 
> > +struct inode *devpts_redirect(struct file *filp)
> > +{
> > +	struct inode *inode;
> > +	struct file *filp2;
> > +
> > +	/* Is the inode already a devpts inode? */
> > +	inode = filp->f_dentry->d_inode;
> > +	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
> > +		goto out;
> > +
> > +	/* Is f_dentry->d_parent usable? */
> > +	inode = ERR_PTR(-ENODEV);
> > +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> > +		goto out;
> > +
> > +	/* Is there a devpts inode we can use instead? */
> > +	
> > +	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
> > +			       "pts/ptmx", O_PATH);
> > +	if (!IS_ERR(filp2)) {
> > +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
> > +			struct path old;
> > +			old = filp->f_path;
> > +			filp->f_path = filp2->f_path;
> > +			inode = filp->f_dentry->d_inode;
> > +			path_get(&filp->f_path);
> > +			path_put(&old);
> 
> You are welcome to supply an analysis of the reasons why ->open() pulling
> such tricks will not break all kinds of code in VFS.
> > +		}
> > +		fput(filp2);
> 
> ... starting with "what happens when some joker binds /dev/ptmx on
> /dev/pts/ptmx"

Or does mknod /tmp/ptmx c 5 2; mkdir /tmp/pts; ln /tmp/ptmx /tmp/pts/ptmx,
for that matter...

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sun, Sep 23, 2012 at 05:19:06AM +0100, Al Viro wrote:
> On Sat, Sep 22, 2012 at 08:50:44PM -0700, Eric W. Biederman wrote:
> 
> > +struct inode *devpts_redirect(struct file *filp)
> > +{
> > +	struct inode *inode;
> > +	struct file *filp2;
> > +
> > +	/* Is the inode already a devpts inode? */
> > +	inode = filp->f_dentry->d_inode;
> > +	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
> > +		goto out;
> > +
> > +	/* Is f_dentry->d_parent usable? */
> > +	inode = ERR_PTR(-ENODEV);
> > +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> > +		goto out;
> > +
> > +	/* Is there a devpts inode we can use instead? */
> > +	
> > +	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
> > +			       "pts/ptmx", O_PATH);
> > +	if (!IS_ERR(filp2)) {
> > +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
> > +			struct path old;
> > +			old = filp->f_path;
> > +			filp->f_path = filp2->f_path;
> > +			inode = filp->f_dentry->d_inode;
> > +			path_get(&filp->f_path);
> > +			path_put(&old);
> 
> You are welcome to supply an analysis of the reasons why ->open() pulling
> such tricks will not break all kinds of code in VFS.
> > +		}
> > +		fput(filp2);
> 
> ... starting with "what happens when some joker binds /dev/ptmx on
> /dev/pts/ptmx"

Or does mknod /tmp/ptmx c 5 2; mkdir /tmp/pts; ln /tmp/ptmx /tmp/pts/ptmx,
for that matter...

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Eric W. Biederman]

Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> writes:

> On Sat, Sep 22, 2012 at 08:50:44PM -0700, Eric W. Biederman wrote:
>
>> +struct inode *devpts_redirect(struct file *filp)
>> +{
>> +	struct inode *inode;
>> +	struct file *filp2;
>> +
>> +	/* Is the inode already a devpts inode? */
>> +	inode = filp->f_dentry->d_inode;
>> +	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>> +		goto out;
>> +
>> +	/* Is f_dentry->d_parent usable? */
>> +	inode = ERR_PTR(-ENODEV);
>> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
>> +		goto out;
>> +
>> +	/* Is there a devpts inode we can use instead? */
>> +	
>> +	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
>> +			       "pts/ptmx", O_PATH);
>> +	if (!IS_ERR(filp2)) {
>> +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
>> +			struct path old;
>> +			old = filp->f_path;
>> +			filp->f_path = filp2->f_path;
>> +			inode = filp->f_dentry->d_inode;
>> +			path_get(&filp->f_path);
>> +			path_put(&old);
>
> You are welcome to supply an analysis of the reasons why ->open() pulling
> such tricks will not break all kinds of code in VFS.


>> +		}
>> +		fput(filp2);
>
> ... starting with "what happens when some joker binds /dev/ptmx on
> /dev/pts/ptmx"

The test:
>> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
kicks in and no redirection is performed.


> Or does mknod /tmp/ptmx c 5 2; mkdir /tmp/pts; ln /tmp/ptmx /tmp/pts/ptmx,
> for that matter...

The test:
>> +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {n
fails and no redirection is performed.

> NAK.  This violates asserts made by VFS (namely, that ->f_path is not
> changed since dentry_open() has set it and until __fput() rips the thing
> out) *and* by your own code (attack mentioned above, just from looking
> at it for a minute).  Far too brittle...

This code seems much more robust than your quick analysis.

But if the constraint that the path in struct file must not be
changed between dentry_open and __fput that is doable to it just
needs a little more reorganizing of the data structures.  It is
definitely not a fundamental limitation.

Eric

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Sat, Sep 22, 2012 at 08:50:44PM -0700, Eric W. Biederman wrote:
>
>> +struct inode *devpts_redirect(struct file *filp)
>> +{
>> +	struct inode *inode;
>> +	struct file *filp2;
>> +
>> +	/* Is the inode already a devpts inode? */
>> +	inode = filp->f_dentry->d_inode;
>> +	if (filp->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>> +		goto out;
>> +
>> +	/* Is f_dentry->d_parent usable? */
>> +	inode = ERR_PTR(-ENODEV);
>> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
>> +		goto out;
>> +
>> +	/* Is there a devpts inode we can use instead? */
>> +	
>> +	filp2 = file_open_root(filp->f_dentry->d_parent, filp->f_vfsmnt,
>> +			       "pts/ptmx", O_PATH);
>> +	if (!IS_ERR(filp2)) {
>> +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {
>> +			struct path old;
>> +			old = filp->f_path;
>> +			filp->f_path = filp2->f_path;
>> +			inode = filp->f_dentry->d_inode;
>> +			path_get(&filp->f_path);
>> +			path_put(&old);
>
> You are welcome to supply an analysis of the reasons why ->open() pulling
> such tricks will not break all kinds of code in VFS.


>> +		}
>> +		fput(filp2);
>
> ... starting with "what happens when some joker binds /dev/ptmx on
> /dev/pts/ptmx"

The test:
>> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
kicks in and no redirection is performed.


> Or does mknod /tmp/ptmx c 5 2; mkdir /tmp/pts; ln /tmp/ptmx /tmp/pts/ptmx,
> for that matter...

The test:
>> +		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {n
fails and no redirection is performed.

> NAK.  This violates asserts made by VFS (namely, that ->f_path is not
> changed since dentry_open() has set it and until __fput() rips the thing
> out) *and* by your own code (attack mentioned above, just from looking
> at it for a minute).  Far too brittle...

This code seems much more robust than your quick analysis.

But if the constraint that the path in struct file must not be
changed between dentry_open and __fput that is doable to it just
needs a little more reorganizing of the data structures.  It is
definitely not a fundamental limitation.

Eric

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sat, Sep 22, 2012 at 10:59:04PM -0700, Eric W. Biederman wrote:

> The test:
> >> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> kicks in and no redirection is performed.

Umm...  OK, after the first round of recursion.  Unless you bind /something/pts
on /something.  Or simply create a symlink.  Hell, if static /dev is on the
same fs as /tmp, it can even be done by unpriveleged attacker -
mkdir /tmp/pts
ln /dev/ptmx /tmp
ln -s /tmp/ptmx /tmp/pts/ptmx
exec </tmp/ptmx
and enjoy the stack overflow in kernel mode.  It's not particulary common
setup, of course, but I think it demonstrates that you are playing with
fire...

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sat, Sep 22, 2012 at 10:59:04PM -0700, Eric W. Biederman wrote:

> The test:
> >> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> kicks in and no redirection is performed.

Umm...  OK, after the first round of recursion.  Unless you bind /something/pts
on /something.  Or simply create a symlink.  Hell, if static /dev is on the
same fs as /tmp, it can even be done by unpriveleged attacker -
mkdir /tmp/pts
ln /dev/ptmx /tmp
ln -s /tmp/ptmx /tmp/pts/ptmx
exec </tmp/ptmx
and enjoy the stack overflow in kernel mode.  It's not particulary common
setup, of course, but I think it demonstrates that you are playing with
fire...

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sun, Sep 23, 2012 at 07:30:38AM +0100, Al Viro wrote:
> On Sat, Sep 22, 2012 at 10:59:04PM -0700, Eric W. Biederman wrote:
> 
> > The test:
> > >> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> > kicks in and no redirection is performed.
> 
> Umm...  OK, after the first round of recursion.  Unless you bind /something/pts
> on /something.

Other way round, of course...  E.g.
mount --bind /dev /dev/pts
mount --bind /dev /dev/pts/pts
etc. enough times to overflow the kernel stack on recursion.

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Al Viro]

On Sun, Sep 23, 2012 at 07:30:38AM +0100, Al Viro wrote:
> On Sat, Sep 22, 2012 at 10:59:04PM -0700, Eric W. Biederman wrote:
> 
> > The test:
> > >> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
> > kicks in and no redirection is performed.
> 
> Umm...  OK, after the first round of recursion.  Unless you bind /something/pts
> on /something.

Other way round, of course...  E.g.
mount --bind /dev /dev/pts
mount --bind /dev /dev/pts/pts
etc. enough times to overflow the kernel stack on recursion.

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Eric W. Biederman]

Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> writes:

> On Sun, Sep 23, 2012 at 07:30:38AM +0100, Al Viro wrote:
>> On Sat, Sep 22, 2012 at 10:59:04PM -0700, Eric W. Biederman wrote:
>> 
>> > The test:
>> > >> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
>> > kicks in and no redirection is performed.
>> 
>> Umm...  OK, after the first round of recursion.  Unless you bind /something/pts
>> on /something.
>
> Other way round, of course...  E.g.
> mount --bind /dev /dev/pts
> mount --bind /dev /dev/pts/pts
> etc. enough times to overflow the kernel stack on recursion.

Except of course I gave the wrong explanation.

I did the open with O_PATH.  Since O_PATH never calls open recursion
is impossible.

Eric

Re: [PATCH 3/4] devpts: Make the newinstance option historical

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Sun, Sep 23, 2012 at 07:30:38AM +0100, Al Viro wrote:
>> On Sat, Sep 22, 2012 at 10:59:04PM -0700, Eric W. Biederman wrote:
>> 
>> > The test:
>> > >> +	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)
>> > kicks in and no redirection is performed.
>> 
>> Umm...  OK, after the first round of recursion.  Unless you bind /something/pts
>> on /something.
>
> Other way round, of course...  E.g.
> mount --bind /dev /dev/pts
> mount --bind /dev /dev/pts/pts
> etc. enough times to overflow the kernel stack on recursion.

Except of course I gave the wrong explanation.

I did the open with O_PATH.  Since O_PATH never calls open recursion
is impossible.

Eric

Re: [PATCH 0/4] devpts: fix devpts mount behavior

[H. Peter Anvin]

On 09/22/2012 08:47 PM, Eric W. Biederman wrote:
> Making /dev/ptmx a symlink sounds simple, but in practice no one could
> actually figure out how to make it happen.

This feels particularly ridiculous since udev now requires devtmpfs (as 
in it will no longer work without it), which means this is all done in 
the kernel, so we should just be able to fix this in the kernel.

Greg, Kay, any idea how to make ptmx show up in devtmpfs as a symlink to 
pts/pmtx instead of as a device node?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/4] devpts: fix devpts mount behavior

[H. Peter Anvin]

On 09/22/2012 08:47 PM, Eric W. Biederman wrote:
> Making /dev/ptmx a symlink sounds simple, but in practice no one could
> actually figure out how to make it happen.

This feels particularly ridiculous since udev now requires devtmpfs (as 
in it will no longer work without it), which means this is all done in 
the kernel, so we should just be able to fix this in the kernel.

Greg, Kay, any idea how to make ptmx show up in devtmpfs as a symlink to 
pts/pmtx instead of as a device node?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/4] devpts: fix devpts mount behavior

[Eric W. Biederman]

"H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> writes:

> On 09/22/2012 08:47 PM, Eric W. Biederman wrote:
>> Making /dev/ptmx a symlink sounds simple, but in practice no one could
>> actually figure out how to make it happen.
>
> This feels particularly ridiculous since udev now requires devtmpfs
> (as in it will no longer work without it), which means this is all
> done in the kernel, so we should just be able to fix this in the
> kernel.

Some versions of udev require devtmpfs.  At best we could have something
conditional in the kernel that does this.

> Greg, Kay, any idea how to make ptmx show up in devtmpfs as a symlink
> to pts/pmtx instead of as a device node?

The conversation stalled in January with that very question.

What I am proposing doesn't conflict with any effort like that.

Eric

Re: [PATCH 0/4] devpts: fix devpts mount behavior

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 09/22/2012 08:47 PM, Eric W. Biederman wrote:
>> Making /dev/ptmx a symlink sounds simple, but in practice no one could
>> actually figure out how to make it happen.
>
> This feels particularly ridiculous since udev now requires devtmpfs
> (as in it will no longer work without it), which means this is all
> done in the kernel, so we should just be able to fix this in the
> kernel.

Some versions of udev require devtmpfs.  At best we could have something
conditional in the kernel that does this.

> Greg, Kay, any idea how to make ptmx show up in devtmpfs as a symlink
> to pts/pmtx instead of as a device node?

The conversation stalled in January with that very question.

What I am proposing doesn't conflict with any effort like that.

Eric

Re: [PATCH 0/4] devpts: fix devpts mount behavior

[H. Peter Anvin]

On 09/23/2012 10:42 AM, Eric W. Biederman wrote:
>
>> Greg, Kay, any idea how to make ptmx show up in devtmpfs as a symlink
>> to pts/pmtx instead of as a device node?
>
> The conversation stalled in January with that very question.
>
> What I am proposing doesn't conflict with any effort like that.
>

I just looked at the archive, and it apparently really comes down to 
"noone wants to take responsibility for it".  The real problem appears 
to be that with devtmpfs device name policy is actually split between 
user space and kernel, which is the worst possible configuration.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/4] devpts: fix devpts mount behavior

[H. Peter Anvin]

On 09/23/2012 10:42 AM, Eric W. Biederman wrote:
>
>> Greg, Kay, any idea how to make ptmx show up in devtmpfs as a symlink
>> to pts/pmtx instead of as a device node?
>
> The conversation stalled in January with that very question.
>
> What I am proposing doesn't conflict with any effort like that.
>

I just looked at the archive, and it apparently really comes down to 
"noone wants to take responsibility for it".  The real problem appears 
to be that with devtmpfs device name policy is actually split between 
user space and kernel, which is the worst possible configuration.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.