fix devpts mount behavior

I've been playing for the last week with ways to fix this, and in the end I can see two ways.

Quoted 3 lines · Serge Hallyn

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

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

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.

Quoted 6 lines · Linus Torvalds

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)

Serge Hallyn [[email protected]] wrote:

| Quoting Al Viro ([email protected]):
| > 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 ?

Quoted 6 lines · Linus Torvalds

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)

Quoted 21 lines · Al Viro

Quoted 6 lines · Linus Torvalds

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.

Quoted 4 lines

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.

Quoted 1 line · Eric W. Biederman

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.

Quoted 4 lines · Serge E. Hallyn

Quoted 1 line · Eric W. Biederman

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

Quoted 3 lines

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.

Quoted 2 lines

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

To do what?

Quoted 4 lines

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

Quoted 3 lines

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

Serge Hallyn [[email protected]] 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 ?

Quoted 9 lines · Kay Sievers

Quoted 7 lines · Serge E. Hallyn

Quoted 1 line · Eric W. Biederman

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)

Quoted 3 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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  :)

Quoted 7 lines

Quoted 4 lines

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

Quoted 11 lines

Quoted 3 lines

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.

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

Quoted 4 lines · Kay Sievers

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

Quoted 1 line

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?

Quoted 8 lines · Serge Hallyn

Quoted 3 lines · Kay Sievers

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 precedent in the /dev/fd symlinks, so I expect you want to look and see where the configuration for that comes from.

fs/devpts/inode.c +17 -0

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

fs/devpts/inode.c +17 -0

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

Quoted 17 lines · Eric W. Biederman

fs/devpts/inode.c +17 -0

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

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 409

Quoted 10 lines

fs/devpts/inode.c +10 -0

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

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 411

Quoted 2 lines

fs/devpts/inode.c +2 -0

+		}
+		fput(filp2);

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

Quoted 17 lines · Eric W. Biederman

fs/devpts/inode.c +17 -0

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

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 409

Quoted 10 lines

fs/devpts/inode.c +10 -0

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

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 411

Quoted 2 lines

fs/devpts/inode.c +2 -0

+		}
+		fput(filp2);

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

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 395

Quoted 1 line

fs/devpts/inode.c +1 -0

+	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)

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

Review

Quoted 1 line

(quoted diff) +1 -0

+		if (filp2->f_dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC) {n

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

The test:

Quoted 1 line

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 395

Quoted 1 line

fs/devpts/inode.c +1 -0

+	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)

kicks in and no redirection is performed.

Quoted 3 lines · Eric W. Biederman

The test:

Quoted 1 line

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 395

Quoted 1 line

fs/devpts/inode.c +1 -0

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

Quoted 6 lines · Al Viro

Quoted 3 lines · Eric W. Biederman

The test:

Quoted 1 line

fs/devpts/inode.c static struct file_system_type devpts_fs_type = { · line 395

Quoted 1 line

fs/devpts/inode.c +1 -0

+	if (filp->f_vfsmnt->mnt_root == filp->f_dentry)

kicks in and no redirection is performed.

Quoted 2 lines

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.

Making /dev/ptmx a symlink sounds simple, but in practice no one could actually figure out how to make it happen.

Quoted 2 lines · Eric W. Biederman

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?

Quoted 3 lines

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.

Quoted 1 line

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.

Linus Torvalds [torvalds-de/[email protected]] 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.

Quoted 3 lines · Serge Hallyn

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?