Some simple helper patches to enable implementation of multiple PTY

(or device) namespaces.
	[PATCH 1/4]: Propagate error code from devpts_pty_new

	[PATCH 2/4]: Factor out PTY index allocation

	[PATCH 3/4]: Move devpts globals into init_pts_ns

	[PATCH 3/4]: Enable multiple mounts of /dev/pts
This patchset is based on earlier versions developed by Serge Hallyn

and Matt Helsley.

--

Suka Stop.
The first two patches appear to just be cleanups and as such should be

able to stand on their own.  Mentioning that you found these

opportunities while working on your pts namespace is fine.  Justifying

the cleanups this way is not.
When you mentioned you intended to just resend the cleanups this is not

at all what I thought you were about.  So please just get the first two

patches in a form that stands by themselves.
The pts namespace as designed is not acceptable.
The problem you are trying to solve with the pts namespace is real.
So what we need is a device namespace and possibly and incremental path

to get there.  A device namespace would abstract the device number to

device mapping for all devices.  A safe incremental path would disable

all device number to device mappings for process in that namespace.  So

all functionality would be gone, then it would enable certain mappings

and certain pieces of the functionality if the code in the kernel is not

clean enough that we can do it all in one go.
We need to see that path.  Only then can we take patches that add

namespace specific goo.  The pattern I am proposing has worked quite

well for the network namespace.  Meanwhile the uid namespace which

follows the pattern you seem to be following now seems does not look to

be completed any time soon.
So send your clean up patches and then let's architect this thing so we

are really talking about a namespace.  Then we can update devpts to

capture the device namespace on mount and do it's work in a namespace

specific manner.
Eric
--

Since the issue of PTY namespaces came up (and was rejected) back in

April, I have thought a little bit about changing ptys to be tied

directly into a devpts instance.  devpts would then be a "normal"

filesystem, which can be mounted multiple times (or not at all).  pty's

would then become private to a devpts instance.
This is what it would appear would have to change, and I'd like to get

people's feeing for the user-space impact:
1. /dev/ptmx would have to change to a symlink, ptmx -> pts/ptmx.

2. Permissions on /dev/ptmx would not be persistent, and would have to

    be set via devpts mount options (unless they're 0666 root.tty, which

    would presumably be the default.)

3. The /proc/sys/kernel/pty limit would be global; a per-filesystem

    limit could be added on top or instead (presumably via a filesystem

    mount options.)
I worry #1 would have substantial user-space impact, but I don't see a

way around it, since there would be no obvious way to associate

/dev/ptmx with a filesystem.
	-hpa

--

Sorry, I thought we were going with a complete device namespace - since that

would address other devices as well and would avoid the following user-space

issue. 
I guess this issue came up in OLS recently and have been looking into this

again.  I have some helper patches to explore multiple mounts of devpts
Sukadev

--

Making them symlinks is asking for trouble because some code does go

around using stat and the like and tools like MAKEDEV have definite ideas.
For /dev/tty the definition is precisely that it is your controlling

tty. No reference to namespace and a task whose controlling tty is in a

different namespace should still open the controlling tty not some

parallel in another universe when you open /dev/tty.
If you want to make sure the controlling tty is in the right namespace

that can be done in userspace when transferring control into a namespace.
/dev/tty and /dev/ptmx already primarily operate by identifying a device

and switching the work to that. Actually putting a bit of namespace logic

in the driver code so the actual files stay as expected (device nodes

etc) seems a *lot* simpler than trying to do symlink hacks.
Alan

--

Alan Cox [alan@lxorguk.ukuu.org.uk] wrote:

| > > 1. /dev/ptmx would have to change to a symlink, ptmx -> pts/ptmx.

| >

| > IIRC, /dev/tty also needs a similar symlink.

|

| Making them symlinks is asking for trouble because some code does go

| around using stat and the like and tools like MAKEDEV have definite ideas.

|

| For /dev/tty the definition is precisely that it is your controlling

| tty. No reference to namespace and a task whose controlling tty is in a

| different namespace should still open the controlling tty not some

| parallel in another universe when you open /dev/tty.
Well, I thought the problem was something like this:
If /dev/pts/1 is the controlling terminal and there are multiple mounts

of devpts, when we open /dev/tty, kernel would somehow have to find the

right instance of devpts.
When init_dev() calls devpts_get_tty(), it would need to specify the devpts

instance. So tty_open() of "/dev/tty" would somehow have to find it based on

the /dev/tty inode (which could be in ext3).
(I thought the issue was similar with /dev/ptmx, ptmx allocates a new

index, /dev/tty accesses an existing index, but both need to somehow

find the right pts instance -no ?)
|

| If you want to make sure the controlling tty is in the right namespace

| that can be done in userspace when transferring control into a namespace.

| In many cases I doubt that is even what is wanted.

|

| > > 2. Permissions on /dev/ptmx would not be persistent, and would have to

| > >    be set via devpts mount options (unless they're 0666 root.tty, which

| > >    would presumably be the default.)

| > > 3. The /proc/sys/kernel/pty limit would be global; a per-filesystem

| > >    limit could be added on top or instead (presumably via a filesystem

| > >    mount options.)

| > >

| > > I worry #1 would have substantial user-space impact, but I don't see a way

| > > around it, since there would be no obvious way to associate /dev/ptmx w...
[ message continues ]

current->tty - it is a direct internal reference.
Alan

--

Why?  I do not believe that is correct.
	-hpa
--

On Debian based systems /dev/ptmx is 0666 root.root.  But the gid of tty
Hmm. Several possibilities:

- Change the filesysteme name and the old name remains usable with

  /dev/ptmx.

  Problem: You could mount the filesystem with the old name within a

  container.

- Make the first mounted one special.

  Problem: Will not survive a umount/mount cycle. But this would be not

  the case anyway.
But you are sure, none of them is a pretty solution.
Bastian
--

I'm a soldier, not a diplomat.  I can only tell the truth.

		-- Kirk, "Errand of Mercy", stardate 3198.9

--

Are your worries just about replacing what is now a normal file with a

symlink, and the behavioral changes that come with that?  
I wonder if using a bind mount for the file would be more robust.  We

wouldn't, of course, be able to do it persistently, but I bet it would

be something we could count on udev to do for us.
-- Dave
--

No, I'm concerned about the changes needed for udev and setup scripts.
	-hpa

--

Not on my boxen.  However, devpts tends to be mounted by initscripts,

which is where one would naturally do that.

--

It took me a minute to figure out what you were offended by, but I see,
(you know perfectly well that we're holding off on any further real

uidns work until netns is complete and you have time to partake in the
Sounds reasonable to me.
thanks,

-serge

--

*boggle*
Care to explain how that "namespace" is different from devpts instance?

IOW, why the devil do you guys ignore Occam's Razor?
Frankly, this nonsense has gone far enough; I can buy the need to compensate

for shitty APIs (sockets, non-fs-based-IPC, etc.), but devpts *is* *a*

*fucking* *filesystem*.  Already.  And as such it's already present in

normal, real, we-really-shouldn't-have-any-other-if-not-for-ancient-stupidity

namespace.
Why not simply allow independent instances of devpts and be done with that?

--

In particular:
/dev/ptmx can be a symlink ptmx -> pts/ptmx, and we add a ptmx instance

inside the devpts filesystem.  Each devpts filesystem is responsible for

its own pool of ptys, with own numbering, etc.
This does mean that entries in /dev/pts are more than just plain device

nodes, which they are now (you can cp -a a device node from /dev/pts

into another filesystem and it will still "just work"), but I doubt this

actually matters to anyone.  If anyone cares, now I guess would be a

good time to speak up.
	-hpa

--

Agreed.   That is another legitimate path.  And if all you care about is

isolation and not dealing with the general class of problems with the

global device number to device mapping that is sane.  I know we have

several other virtual devices that we tend to care about but ptys are

the real world pain point.
Further I don't see any conflict with the generalizing devpts in this

manner (so you only see a subset of the ptys) and then later adding a

namespace that deals with the whole device number to device mapping.
Eric
--

Thinking about it further, allowing this restriction would also allow a

whole lot of cleanups inside the pty setup, since it would eliminate the

need to do a separate lookup to find the corresponding devpts entry in

pty_open().  The benefit here comes from the closer coupling between the

pty and the devpts filesystem and isn't at all related to namespaces,

but it's a very nice side benefit.
	-hpa

--

Minor correction: the lookup is actually in init_dev() in tty_io.c; I'm

specifically referring to devpts_get_tty().
	-hpa

--

Any measurable performance impact when not using these kinds of namespaces?
	-hpa

--

From: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Subject:[PATCH 4/4]: Enable multiple mounts of /dev/pts
To support multiple PTY namespaces, allow multiple mounts of /dev/pts, once

within each PTY namespace.
This patch removes the get_sb_single() in devpts_get_sb() and uses test and

set sb interfaces to allow remounting /dev/pts.
Changelog [v4]:

	- Split-off the simpler changes of moving global=variables into

	  'pts_namespace' to previous patch.
Changelog [v3]:

	- Removed some unnecessary comments from devpts_set_sb()
Changelog [v2]:
	- (Pavel Emelianov/Serge Hallyn) Remove reference to pts_ns from

	  sb->s_fs_info to fix the circular reference (/dev/pts is not

	  unmounted unless the pts_ns is destroyed, so we don't need a

	  reference to the pts_ns).
Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>

---

 fs/devpts/inode.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++---

 1 file changed, 59 insertions(+), 3 deletions(-)
Index: 2.6.25-rc8-mm1/fs/devpts/inode.c

===================================================================

--- 2.6.25-rc8-mm1.orig/fs/devpts/inode.c	2008-04-12 10:10:33.000000000 -0700

+++ 2.6.25-rc8-mm1/fs/devpts/inode.c	2008-04-12 10:10:38.000000000 -0700

@@ -154,17 +154,73 @@ fail:

 	return -ENOMEM;

 }
+/*

+ * We use test and set super-block operations to help determine whether we

+ * need a new super-block for this namespace. get_sb() walks the list of

+ * existing devpts supers, comparing them with the @data ptr. Since we

+ * passed 'current's namespace as the @data pointer we can compare the

+ * namespace pointer in the super-block's 's_fs_info'.  If the test is

+ * TRUE then get_sb() returns a new active reference to the super block.

+ * Otherwise, it helps us build an active reference to a new one.

+ */

+

+static int devpts_test_sb(struct super_block *sb, void *data)

+{

+	return sb->s_f...
[ message continues ]

Matt, Serge, please sign-off on this version.

---

From: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Subject: [PATCH 3/4]: Move devpts globals into init_pts_ns
Move devpts global variables 'allocated_ptys' and 'devpts_mnt' into a new

'pts_namespace' and remove the 'devpts_root'.
Changelog:

	- Split these relatively simpler changes off from the patch that

	  supports remounting devpts.
Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>

---

 fs/devpts/inode.c         |   84 ++++++++++++++++++++++++++++++++--------------

 include/linux/devpts_fs.h |   10 +++++

 2 files changed, 70 insertions(+), 24 deletions(-)
Index: 2.6.25-rc8-mm1/fs/devpts/inode.c

===================================================================

--- 2.6.25-rc8-mm1.orig/fs/devpts/inode.c	2008-04-11 10:12:09.000000000 -0700

+++ 2.6.25-rc8-mm1/fs/devpts/inode.c	2008-04-12 10:10:33.000000000 -0700

@@ -28,12 +28,8 @@

 #define DEVPTS_DEFAULT_MODE 0600
 extern int pty_limit;			/* Config limit on Unix98 ptys */

-static DEFINE_IDR(allocated_ptys);

 static DECLARE_MUTEX(allocated_ptys_lock);
-static struct vfsmount *devpts_mnt;

-static struct dentry *devpts_root;

-

 static struct {

 	int setuid;

 	int setgid;

@@ -54,6 +50,14 @@ static match_table_t tokens = {

 	{Opt_err, NULL}

 };
+struct pts_namespace init_pts_ns = {

+	.kref = {

+		.refcount = ATOMIC_INIT(2),

+	},

+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),

+	.mnt = NULL,

+};

+

 static int devpts_remount(struct super_block *sb, int *flags, char *data)

 {

 	char *p;

@@ -140,7 +144,7 @@ devpts_fill_super(struct super_block *s,

 	inode->i_fop = &simple_dir_operations;

 	inode->i_nlink = 2;
-	devpts_root = s->s_root = d_alloc_root(inode);

+	s->s_root = d_alloc_root(inode);

 	if (s->s_root)

 		return 0;
@@ -168,10 +172,9 @@ static struct file_system_type devpts_fs

  * to the System V naming convention

  */
-static struct dentry *get_node(int num)

+static struct dentry *get_node(stru...
[ message continues ]

From: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Subject: [PATCH 2/4]: Factor out PTY index allocation
Factor out the code used to allocate/free a pts index into new interfaces,

devpts_new_index() and devpts_kill_index().  This localizes the external

data structures used in managing the pts indices.
Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Signed-off-by: Serge Hallyn<serue@us.ibm.com>

Signed-off-by: Matt Helsley<matthltc@us.ibm.com>
---

 drivers/char/tty_io.c     |   40 ++++++----------------------------------

 fs/devpts/inode.c         |   42 +++++++++++++++++++++++++++++++++++++++++-

 include/linux/devpts_fs.h |    4 ++++

 3 files changed, 51 insertions(+), 35 deletions(-)
Index: 2.6.25-rc5-mm1/include/linux/devpts_fs.h

===================================================================

--- 2.6.25-rc5-mm1.orig/include/linux/devpts_fs.h	2008-03-24 20:04:07.000000000 -0700

+++ 2.6.25-rc5-mm1/include/linux/devpts_fs.h	2008-03-24 20:04:26.000000000 -0700

@@ -17,6 +17,8 @@
 #ifdef CONFIG_UNIX98_PTYS
+int devpts_new_index(void);

+void devpts_kill_index(int idx);

 int devpts_pty_new(struct tty_struct *tty);      /* mknod in devpts */

 struct tty_struct *devpts_get_tty(int number);	 /* get tty structure */

 void devpts_pty_kill(int number);		 /* unlink */

@@ -24,6 +26,8 @@ void devpts_pty_kill(int number);		 /* u

 #else
 /* Dummy stubs in the no-pty case */

+static inline int devpts_new_index(void) { return -EINVAL; }

+static inline void devpts_kill_index(int idx) { }

 static inline int devpts_pty_new(struct tty_struct *tty) { return -EINVAL; }

 static inline struct tty_struct *devpts_get_tty(int number) { return NULL; }

 static inline void devpts_pty_kill(int number) { }

Index: 2.6.25-rc5-mm1/drivers/char/tty_io.c

===================================================================

--- 2.6.25-rc5-mm1.orig/drivers/char/tty_io.c	2008-03-24 20:04:07.000000000 -0700

+++ 2.6.25-rc5-mm1/drivers/char/tty_io.c	2008-03-24 20:04:26.000000000...
[ message continues ]

From: Sukadev Bhattiprolu <sukadev@us.ibm.com>

Subject: [PATCH 1/4]: Propagate error code from devpts_pty_new
Have ptmx_open() propagate any error code returned by devpts_pty_new()

(which returns either 0 or -ENOMEM anyway).
Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>

---

 drivers/char/tty_io.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
Index: 2.6.25-rc8-mm1/drivers/char/tty_io.c

===================================================================

--- 2.6.25-rc8-mm1.orig/drivers/char/tty_io.c	2008-04-07 14:49:56.000000000 -0700

+++ 2.6.25-rc8-mm1/drivers/char/tty_io.c	2008-04-09 13:54:00.000000000 -0700

@@ -2835,8 +2835,8 @@ static int ptmx_open(struct inode *inode

 	filp->private_data = tty;

 	file_move(filp, &tty->tty_files);
-	retval = -ENOMEM;

-	if (devpts_pty_new(tty->link))

+	retval = devpts_pty_new(tty->link);

+	if (retval)

 		goto out1;
 	check_tty_count(tty, "tty_open");

--