Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1371063 > unrolled thread
| Started by | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| First post | 2016-04-05 02:20 +0200 |
| Last post | 2016-04-11 22:20 +0200 |
| Articles | 20 on this page of 70 — 7 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 00/13] devpts: New instances for every mount ebiederm@xmission.com (Eric W. Biederman) - 2016-04-05 02:20 +0200
[PATCH 04/13] devpts: Stop rolling devpts_remount by hand in devpts_mount "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 05/13] devpts: Fail early (if appropriate) on overmount "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 08/13] devpts: Make devpts_kill_sb safe if fsi is NULL "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 03/13] devpts: Cleanup newinstance parsing "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 06/13] devpts: Use the same default mode for both /dev/ptmx and dev/pts/ptmx "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 09/13] devpts: Move the creation of /dev/pts/ptmx into fill_super "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 02/13] devpts: More obvious check for the system devpts in pty allocation "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 13/13] devpts: Kill the DEVPTS_MULTIPLE_INSTANCE config option "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 07/13] devpts: Move parse_mount_options into fill_super "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 03:50 +0200
[PATCH 12/13] devpts: Always return a distinct instance when mounting "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 04:00 +0200
[PATCH 11/13] vfs: Implement mount_super_once "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 04:00 +0200
[PATCH 10/13] devpts: Simplify devpts_mount by using mount_nodev "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-05 04:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-05 05:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-05 05:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-08 21:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-07 18:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-08 21:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-08 21:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-08 22:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Andy Lutomirski <luto@amacapital.net> - 2016-04-08 22:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-08 23:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Andy Lutomirski <luto@amacapital.net> - 2016-04-09 00:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-09 00:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-09 01:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-04-09 15:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-09 16:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-09 17:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-10 00:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-10 02:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-10 02:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-10 02:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Andy Lutomirski <luto@amacapital.net> - 2016-04-10 02:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-11 17:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-12 03:40 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Andy Lutomirski <luto@amacapital.net> - 2016-04-11 22:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-11 22:30 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-12 01:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-12 02:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-12 02:30 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-12 03:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-12 03:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-12 03:40 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-12 03:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-12 03:40 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-12 04:30 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-11 22:30 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Andy Lutomirski <luto@amacapital.net> - 2016-04-12 19:50 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-12 20:20 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup "H. Peter Anvin" <hpa@zytor.com> - 2016-04-12 21:20 +0200
[PATCH 01/16] devpts: Attempting to get it right ebiederm@xmission.com (Eric W. Biederman) - 2016-04-15 17:50 +0200
[PATCH 08/16] devpts: Stop rolling devpts_remount by hand in devpts_mount "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 05/16] vfs: Allow unlink, and rename on expirable file mounts "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 04/16] devpts: Teach /dev/ptmx to automount the appropriate devpts via path lookup "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 09/16] devpts: Fail early (if appropriate) on overmount "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 10/16] devpts: Move parse_mount_options into fill_super "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 06/16] devpts: More obvious check for the system devpts in pty allocation "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 01/16] devpts: Use the same default mode for both /dev/ptmx and dev/pts/ptmx "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 17:50 +0200
[PATCH 12/16] devpts: Move the creation of /dev/pts/ptmx into fill_super "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 18:00 +0200
[PATCH 11/16] devpts: Make devpts_kill_sb safe if fsi is NULL "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 18:00 +0200
[PATCH 16/16] devpts: Kill the DEVPTS_MULTIPLE_INSTANCE config option "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 18:00 +0200
[PATCH 15/16] devpts: Always return a distinct instance when mounting "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 18:00 +0200
[PATCH 13/16] devpts: Simplify devpts_mount by using mount_nodev "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 18:00 +0200
[PATCH 14/16] vfs: Implement mount_super_once "Eric W. Biederman" <ebiederm@xmission.com> - 2016-04-15 18:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-12 02:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-12 02:10 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-12 02:40 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-12 03:00 +0200
Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup ebiederm@xmission.com (Eric W. Biederman) - 2016-04-11 22:20 +0200
Page 1 of 4 [1] 2 3 4 Next page →
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2016-04-05 02:20 +0200 |
| Subject | [PATCH 00/13] devpts: New instances for every mount |
| Message-ID | <rkmSS-2Qr-13@gated-at.bofh.it> |
To recap the situation for those who have not been following closely.
There are programs such as xen-create-image that run as root and setup
a chroot environment with:
"mknod dev/ptmx c 5 2"
"mkdir dev/pts"
"mount -t devpts none dev/pts"
Which mostly works but stomps the mount options of the system /dev/pts.
In particular the options of "gid=5,mode=620" are lost resulting in a
situation where creating a new pty by opening /dev/ptmx results in
that pty having the wrong permissions.
Some distributions have been working around this problem by continuing
to install a setuid root pt_chown binary that will be called by glibc
to fix the permissions.
This solution isn't too scary as long as there is only one instance
of devpts but add a second instance of devpts and it becomes possible
to trick the setuid root pt_chown binary into operating on the wrong
files and directories.
The following patchset attempts to dig use out of this mess by carefully
chaning devpts in a way that does not induce any userspace regressions.
My expectation is that userspace developers will love us as it makes
their problems go away, and kernel developers will not be happy with the
changes because what is required to preserve backwards compatibility is
not what anyone would have designed with a clean sheet implementation.
To dig our selves out of this mess it has been generally agreed that it
makes sense for each mount of devpts to result in a separate instance of
devpts. That works for ensuring that a new mount of devpts does not
stomp the permissions of another mount of devpts but then programs such
as xen-create-image break as they expect opening of /dev/ptmx to create
new ptys.
The problem of /dev/ptmx needing to create ptys on different instances
of devpts can be resolved by performing a path based look up from the
"/dev/ptmx" node to find the devpts filesystem.
How we get each mount of devpts to result in a distinct instance of
devpts matters. The kernel treats the system instance of devpts
specially and to maintain backwards compatibility that needs to be
preserved.
As there must be a system instance of devpts the code continues in it's
technique of mounting the system instance of devpts internally and then
exporting it userspace when devpts is mounted under the right
circumstances.
There is one pattern in userspace where an intial ramdisk mounts devpts
then unmounts devpts and the usual system startup scripts then mount
devpts on /dev/pts. I believe Centos5 and Openwrt use this pattern.
There is another pattern of userspace where devpts is mounted in the
initial ramdisk and then "mount --move" is used to move it onto the
final /dev/pts. However devpts remains listed in /etc/fstab and later
in the boot a "mount -a" honors that listing starts mounting devpts on
top of itself. Fails but only after succeeding in changing the mount
options. I believe this is the pattenr that Centos6 uses.
Then there is the question of which permissions checks should apply when
/dev/ptmx is opened. The way it works in v4.6-rc1 is that a pty on the
system instance of devtps can be created either by opening /dev/ptmx or
/dev/pts/ptmx (which happens to reside on the system instance. The only
restriction are the normal unix permission of opening those files. A
pty on a non-system instance of devpts must be created by opening the
ptmx file on the non-systme devpts instance. The default permisions for
/dev/ptmx are 0666 and for /dev/pts/ptmx 0000. Today for non-system
instances of devpts the permission on the ptmx file are always changed to
something else (typically 0666), and typically on the system instance
the permission of ptmx on devpts are ignoreed and left at 0000.
Which leaves the question of what to do about cases such as
xen-create-image where the new /dev/ptmx path based lookup is exercised
to find the mount of devpts. In that case out of an abundance of
caution I require the code to verify that the opener also has permission
to open the ptmx file on the non-system instance of devpts. To make
that work I have changed the default permissions on the ptmx file for
all non-system instance of devpts to 0666. Where the permission check
comes in as useful is on any system where a non-system instance of
devpts and has permissions on it's ptmx file that are not 0666. If that
was not done a simple bind mount of the /dev/ptmx device node would
allow overriding the policy of who is allowed to create ptys in an
instance of devpts.
As you can see above my guiding principle this round has been be very
careful to keep existing userspace working. I have tested this code on
as wide a range of distributions as I could to look for intesting
behavior. Those I managed to get setup and running in a vm for testing
are: on openwrt-15.05, centos5, centos6, centos7, debian-6.0.2,
debian-7.9, debian-8.2, ubuntu-14.04.3, ubuntu-15.10, fedora23, magia-5,
mint-17.3, opensuse-42.1, slackware-14.1, gentoo-20151225 (13.0?),
archlinux-2015-12-01.
As I could not find an image of Android that was easy to get running
in a VM, so I audited the code to see what Android does. Unlike
reports earlier in this conversation Android does not use a shell
script. Android has a daemon that listens on netlink for device events,
consults it's policy data base and creates the device node in a tmpfs
instance mounted on /dev according to policy (assuming the policy allows
that device node). Furthermore at the start of that daemon devpts is
mounted exactly once. Which thankfully means Android poses no special
problems for this patchset.
I have also run xen-create-image on debian 8.2 (where it was easily
installed with apt-get) and confirmed that without these changes it
stomps the mount options of devpts and with these changes it only uses
atypical mount options on a separate instance of devpts.
The first change in this series adds magic to /dev/ptmx. The rest of
the changes deal with all of the little issues needed to ensure that
every mount of devpts is a distinct instance.
Eric W. Biederman (13):
devpts: Teach /dev/ptmx to find the associated devpts via path lookup
devpts: More obvious check for the system devpts in pty allocation
devpts: Cleanup newinstance parsing
devpts: Stop rolling devpts_remount by hand in devpts_mount
devpts: Fail early (if appropriate) on overmount
devpts: Use the same default mode for both /dev/ptmx and dev/pts/ptmx
devpts: Move parse_mount_options into fill_super
devpts: Make devpts_kill_sb safe if fsi is NULL
devpts: Move the creation of /dev/pts/ptmx into fill_super
devpts: Simplify devpts_mount by using mount_nodev
vfs: Implement mount_super_once
devpts: Always return a distinct instance when mounting
devpts: Kill the DEVPTS_MULTIPLE_INSTANCE config option
Documentation/filesystems/devpts.txt | 122 +++++++----------
drivers/tty/Kconfig | 11 --
drivers/tty/pty.c | 4 +
drivers/tty/tty_io.c | 5 +-
fs/devpts/inode.c | 250 ++++++++++++++++++++---------------
fs/namei.c | 64 +++++++--
fs/namespace.c | 8 ++
fs/open.c | 19 +++
fs/super.c | 12 ++
include/linux/devpts_fs.h | 5 +
include/linux/fs.h | 5 +
include/linux/namei.h | 3 +
12 files changed, 302 insertions(+), 206 deletions(-)
This code is also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git devpts-for-testing
Eric
[toc] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 04/13] devpts: Stop rolling devpts_remount by hand in devpts_mount |
| Message-ID | <rkohY-3Gi-5@gated-at.bofh.it> |
| In reply to | #1371063 |
Just use devpts_remount and by doing so ensuring that ptxmode
actually get propogated to /dev/pts/ptmx on the initial mount
of devpts.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c3d53d2f7c3e..14be886f987c 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -338,12 +338,6 @@ static int mknod_ptmx(struct super_block *sb)
inode_lock(d_inode(root));
- /* If we have already created ptmx node, return */
- if (fsi->ptmx_dentry) {
- rc = 0;
- goto out;
- }
-
dentry = d_alloc_name(root, "ptmx");
if (!dentry) {
pr_err("Unable to alloc dentry for ptmx node\n");
@@ -552,16 +546,20 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
if (error)
goto out_undo_sget;
- s->s_flags |= MS_ACTIVE;
- }
- error = parse_mount_options(data, &DEVPTS_SB(s)->mount_opts);
- if (error)
- goto out_undo_sget;
+ error = parse_mount_options(data, &DEVPTS_SB(s)->mount_opts);
+ if (error)
+ goto out_undo_sget;
+
+ error = mknod_ptmx(s);
+ if (error)
+ goto out_undo_sget;
- error = mknod_ptmx(s);
- if (error)
- goto out_undo_sget;
+ s->s_flags |= MS_ACTIVE;
+ } else {
+ /* Match mount_single ignore errors on remount */
+ devpts_remount(s, &flags, data);
+ }
return dget(s->s_root);
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 05/13] devpts: Fail early (if appropriate) on overmount |
| Message-ID | <rkohY-3Gi-1@gated-at.bofh.it> |
| In reply to | #1371063 |
Update the vfs with a may_overmount superblock operation, allowing
devpts to fail early if the primary mount of devpts is going to be
mounted on top of itself.
This change is in preparation for each mount of devpts being distinct
from every other mount of devpts. To maintain a backward compatible
notion of a primary mount of devpts we need overmounts of the mount to
fail (as they do now), which requires a little vfs support so the case
can be detected.
Cause failed over mounts of devpts to go through the devpts remount
path. This already happens as overmounts have previously been detected
late, and it looks like CentOS 6 may actually depend on this behavior
to allow changing devpts mount options by placing them in /etc/fstab.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 15 +++++++++++++++
fs/namespace.c | 8 ++++++++
include/linux/fs.h | 1 +
3 files changed, 24 insertions(+)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 14be886f987c..cb0cc4e33c3f 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -427,10 +427,25 @@ static int devpts_show_options(struct seq_file *seq, struct dentry *root)
return 0;
}
+bool devpts_may_overmount(struct super_block *sb,
+ int flags, const char *dev_name, void *data)
+{
+ if ((sb == devpts_mnt->mnt_sb) &&
+ (current_user_ns() == &init_user_ns) &&
+ !parse_newinstance(data)) {
+ down_write(&sb->s_umount);
+ devpts_remount(sb, &flags, data);
+ up_write(&sb->s_umount);
+ return false;
+ }
+ return true;
+}
+
static const struct super_operations devpts_sops = {
.statfs = simple_statfs,
.remount_fs = devpts_remount,
.show_options = devpts_show_options,
+ .may_overmount = devpts_may_overmount,
};
static void *new_pts_fs_info(void)
diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691b4355..90bbbabfe3c9 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2386,6 +2386,7 @@ static int do_new_mount(struct path *path, const char *fstype, int flags,
{
struct file_system_type *type;
struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns;
+ struct super_block *path_sb;
struct vfsmount *mnt;
int err;
@@ -2414,6 +2415,13 @@ static int do_new_mount(struct path *path, const char *fstype, int flags,
}
}
+ path_sb = path->mnt->mnt_sb;
+ if ((path_sb->s_type == type) &&
+ (path->mnt->mnt_root == path->dentry) &&
+ path_sb->s_op->may_overmount &&
+ !path_sb->s_op->may_overmount(path_sb, flags, name, data))
+ return -EBUSY;
+
mnt = vfs_kern_mount(type, flags, name, data);
if (!IS_ERR(mnt) && (type->fs_flags & FS_HAS_SUBTYPE) &&
!mnt->mnt_sb->s_subtype)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 045bbfe2ecfc..02a980bfad5c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1755,6 +1755,7 @@ struct super_operations {
struct shrink_control *);
long (*free_cached_objects)(struct super_block *,
struct shrink_control *);
+ bool (*may_overmount)(struct super_block *, int, const char *, void *);
};
/*
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 08/13] devpts: Make devpts_kill_sb safe if fsi is NULL |
| Message-ID | <rkohY-3Gi-17@gated-at.bofh.it> |
| In reply to | #1371063 |
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 46d633ab16c7..266a7b7501a6 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -597,7 +597,8 @@ static void devpts_kill_sb(struct super_block *sb)
{
struct pts_fs_info *fsi = DEVPTS_SB(sb);
- ida_destroy(&fsi->allocated_ptys);
+ if (fsi)
+ ida_destroy(&fsi->allocated_ptys);
kfree(fsi);
kill_litter_super(sb);
}
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 03/13] devpts: Cleanup newinstance parsing |
| Message-ID | <rkohZ-3Gi-23@gated-at.bofh.it> |
| In reply to | #1371063 |
Add a dedicated parsing routing for newinstance that does not modify
data, so parsing out newinstance can be separate from the parsing of
the other mount options.
Allways pass as data to parse_mount_options the filesystem specific
portion of the super_block that holds the mount options.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 44 ++++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 9f22c959d1f7..c3d53d2f7c3e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -105,7 +105,6 @@ struct pts_mount_opts {
kgid_t gid;
umode_t mode;
umode_t ptmxmode;
- int newinstance;
int max;
};
@@ -228,19 +227,27 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
return devpts_mnt->mnt_sb;
}
-#define PARSE_MOUNT 0
-#define PARSE_REMOUNT 1
+static bool parse_newinstance(const char *data)
+{
+ while (data) {
+ const char *p = strchr(data, ',');
+ size_t len = p ? p - data : strlen(data);
+ if ((len == 11) && (memcmp(data, "newinstance", 11) == 0)) {
+ return true;
+ }
+ data = p ? p + 1 : NULL;
+ }
+ return false;
+}
/*
* parse_mount_options():
* Set @opts to mount options specified in @data. If an option is not
- * specified in @data, set it to its default value. The exception is
- * 'newinstance' option which can only be set/cleared on a mount (i.e.
- * cannot be changed during remount).
+ * specified in @data, set it to its default value.
*
* Note: @data may be NULL (in which case all options are set to default).
*/
-static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
+static int parse_mount_options(char *data, struct pts_mount_opts *opts)
{
char *p;
kuid_t uid;
@@ -254,10 +261,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;
@@ -298,9 +301,6 @@ 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;
break;
case Opt_max:
if (match_int(&args[0], &option) ||
@@ -399,7 +399,7 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
struct pts_mount_opts *opts = &fsi->mount_opts;
sync_filesystem(sb);
- err = parse_mount_options(data, PARSE_REMOUNT, opts);
+ err = parse_mount_options(data, opts);
/*
* parse_mount_options() restores options to default values
@@ -528,20 +528,18 @@ 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;
+ bool newinstance;
- error = parse_mount_options(data, PARSE_MOUNT, &opts);
- if (error)
- return ERR_PTR(error);
+ newinstance = parse_newinstance(data);
/* Require newinstance for all user namespace mounts to ensure
* the mount options are not changed.
*/
- if ((current_user_ns() != &init_user_ns) && !opts.newinstance)
+ if ((current_user_ns() != &init_user_ns) && !newinstance)
return ERR_PTR(-EINVAL);
- if (opts.newinstance)
+ if (newinstance)
s = sget(fs_type, NULL, set_anon_super, flags, NULL);
else
s = sget(fs_type, compare_init_pts_sb, set_anon_super, flags,
@@ -557,7 +555,9 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
s->s_flags |= MS_ACTIVE;
}
- memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
+ error = parse_mount_options(data, &DEVPTS_SB(s)->mount_opts);
+ if (error)
+ goto out_undo_sget;
error = mknod_ptmx(s);
if (error)
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 06/13] devpts: Use the same default mode for both /dev/ptmx and dev/pts/ptmx |
| Message-ID | <rkohZ-3Gi-21@gated-at.bofh.it> |
| In reply to | #1371063 |
The default mode is 0666 on /dev/ptmx so we really don't gain anything
except a bunch of hassle by not having the default mode on
/dev/pts/ptmx also being 0666.
Leave the default of ptmxmode at 0000 on the system mount of devpts to
avoid the off chance that it would open an security hole on existing
setups that change the permissions of devpts.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
drivers/tty/tty_io.c | 5 +++--
fs/devpts/inode.c | 11 +++--------
include/linux/devpts_fs.h | 3 +++
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8d26ed79bb4c..438b2209ea41 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3597,9 +3597,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 cb0cc4e33c3f..80c78bb472a9 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -30,14 +30,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.
@@ -260,6 +252,9 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
opts->mode = DEVPTS_DEFAULT_MODE;
opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
opts->max = NR_UNIX98_PTY_MAX;
+ if (!devpts_mnt ||
+ (&DEVPTS_SB(devpts_mnt->mnt_sb)->mount_opts == opts))
+ opts->ptmxmode = 0;
while ((p = strsep(&data, ",")) != NULL) {
substring_t args[MAX_OPT_ARGS];
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 260190690674..80908db52667 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
struct inode *devpts_ptmx(struct inode *inode, struct file *filp);
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rkohY-3Gi-3@gated-at.bofh.it> |
| In reply to | #1371063 |
This is in preparation for forcing each mount of devpts to be a
distinct filesystem. The goal of this change is to have as few user
visible changes from the kernel today as possible.
On each open of /dev/ptmx look at the relative path pts and see if
devpts is mounted there.
If the filesystem found via the path lookup is the system devpts
do exactly what we do today.
If the filesystem found is not the system devpts make it appear
to the rest of the system that the /dev/pts/ptmx node was opened.
This includes respecting the permission checks of /dev/pts/ptmx
and updating the file's path to point to /dev/pts/ptmx.
In practice I expect the permission checks are a non-issue as the
permissions on /dev/ptmx and /dev/pts/ptmx are always 0666. If
someone happens to change the permission on /dev/pts/ptmx is seems
important to honor them for the sake of backwards compatibility. As
/dev/ptmx can be bind mounted to set next to any devpts filesystem not
honoring the permissions would provide a nice permission bypass under
the right circumstances if we did not check the permissions.
Similarly reflect the instance of devpts that was opened in f_path so
that we preserve the only way available today to test if someone is
attempting to confuse a pty creating program.
This winds up using 3 new vfs helpers path_parent, path_pts, and
update_file_path.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
drivers/tty/pty.c | 4 +++
fs/devpts/inode.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
fs/namei.c | 64 ++++++++++++++++++++++++++++++-------
fs/open.c | 19 +++++++++++
include/linux/devpts_fs.h | 2 ++
include/linux/fs.h | 2 ++
include/linux/namei.h | 3 ++
7 files changed, 164 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e16a49b507ef..557858ef00f5 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -725,6 +725,10 @@ static int ptmx_open(struct inode *inode, struct file *filp)
int retval;
int index;
+ inode = devpts_ptmx(inode, filp);
+ if (IS_ERR(inode))
+ return PTR_ERR(inode);
+
nonseekable_open(inode, filp);
/* We refuse fsnotify events on ptmx, since it's a shared resource */
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 655f21f99160..c14d51795577 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -17,6 +17,7 @@
#include <linux/fs.h>
#include <linux/sched.h>
#include <linux/namei.h>
+#include <linux/fs_struct.h>
#include <linux/slab.h>
#include <linux/mount.h>
#include <linux/tty.h>
@@ -136,6 +137,86 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
return sb->s_fs_info;
}
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+static int devpts_path_ptmx(struct file *filp)
+{
+ struct pts_fs_info *fsi;
+ struct path root, path;
+ struct dentry *old;
+ int err = -ENOENT;
+ int ret;
+
+ /* Can the pts filesystem be found with a path walk? */
+ path = filp->f_path;
+ path_get(&path);
+ get_fs_root(current->fs, &root);
+ ret = path_parent(&root, &path);
+ path_put(&root);
+ if (ret != 1)
+ goto fail;
+
+ /* Remember the result of this permission check for later */
+ ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
+ if (path_pts(&path))
+ goto fail;
+
+ /* Is path the root of a devpts filesystem? */
+ if ((path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) ||
+ (path.mnt->mnt_root != path.mnt->mnt_sb->s_root))
+ goto fail;
+ fsi = DEVPTS_SB(path.mnt->mnt_sb);
+
+ /* Get out if the path walk resulted in the default devpts instance */
+ if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
+ goto fail;
+
+ /* Don't allow bypassing the existing /dev/pts/ptmx permission check */
+ err = ret;
+ if (!err)
+ err = inode_permission(path.dentry->d_inode, MAY_EXEC);
+ if (!err)
+ err = inode_permission(fsi->ptmx_dentry->d_inode,
+ ACC_MODE(filp->f_flags));
+ if (err)
+ goto fail;
+
+ /* Advance path to the ptmx dentry */
+ old = path.dentry;
+ path.dentry = dget(fsi->ptmx_dentry);
+ dput(old);
+
+ /* Make it look like /dev/pts/ptmx was opened */
+ err = update_file_path(filp, &path);
+ if (err)
+ goto fail;
+
+ return 0;
+fail:
+ path_put(&path);
+ return err;
+}
+#else
+static inline int devpts_path_ptmx(struct file *filp)
+{
+ return -ENOENT;
+}
+#endif
+
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
+{
+ int err;
+ if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
+ return inode;
+
+ err = devpts_path_ptmx(filp);
+ if (err == 0)
+ return filp->f_inode;
+ if (err != -ENOENT)
+ return ERR_PTR(err);
+
+ return inode;
+}
+
static inline struct super_block *pts_sb_from_inode(struct inode *inode)
{
#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
diff --git a/fs/namei.c b/fs/namei.c
index 794f81dce766..afb5137ca199 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1415,29 +1415,41 @@ static void follow_mount(struct path *path)
}
}
-static int follow_dotdot(struct nameidata *nd)
+int path_parent(struct path *root, struct path *path)
{
+ int ret = 0;
+
while(1) {
- struct dentry *old = nd->path.dentry;
+ struct dentry *old = path->dentry;
- if (nd->path.dentry == nd->root.dentry &&
- nd->path.mnt == nd->root.mnt) {
+ if (old == root->dentry &&
+ path->mnt == root->mnt) {
break;
}
- if (nd->path.dentry != nd->path.mnt->mnt_root) {
+ if (old != path->mnt->mnt_root) {
/* rare case of legitimate dget_parent()... */
- nd->path.dentry = dget_parent(nd->path.dentry);
+ path->dentry = dget_parent(path->dentry);
dput(old);
- if (unlikely(!path_connected(&nd->path)))
+ if (unlikely(!path_connected(path)))
return -ENOENT;
+ ret = 1;
break;
}
- if (!follow_up(&nd->path))
+ if (!follow_up(path))
break;
}
- follow_mount(&nd->path);
- nd->inode = nd->path.dentry->d_inode;
- return 0;
+ follow_mount(path);
+ return ret;
+}
+
+static int follow_dotdot(struct nameidata *nd)
+{
+ int ret = path_parent(&nd->root, &nd->path);
+ if (ret >= 0) {
+ ret = 0;
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ return ret;
}
/*
@@ -2374,6 +2386,36 @@ struct dentry *lookup_one_len_unlocked(const char *name,
}
EXPORT_SYMBOL(lookup_one_len_unlocked);
+#ifdef CONFIG_UNIX98_PTYS
+int path_pts(struct path *path)
+{
+ struct dentry *child, *parent = path->dentry;
+ struct qstr this;
+
+ if (!d_can_lookup(parent))
+ return -ENOENT;
+
+ this.name = "pts";
+ this.len = 3;
+ this.hash = full_name_hash(this.name, this.len);
+ if (parent->d_flags & DCACHE_OP_HASH) {
+ int err = parent->d_op->d_hash(parent, &this);
+ if (err < 0)
+ return err;
+ }
+ inode_lock(parent->d_inode);
+ child = d_lookup(parent, &this);
+ inode_unlock(parent->d_inode);
+ if (!child)
+ return -ENOENT;
+
+ path->dentry = child;
+ dput(parent);
+ follow_mount(path);
+ return 0;
+}
+#endif
+
int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
struct path *path, int *empty)
{
diff --git a/fs/open.c b/fs/open.c
index 17cb6b1dab75..e1ed78fa474b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -679,6 +679,24 @@ int open_check_o_direct(struct file *f)
return 0;
}
+int update_file_path(struct file *filp, struct path *path)
+{
+ /* Only valid during f_op->open, and even in open use very carefully */
+ struct path old;
+ struct inode *inode;
+
+ if (filp->f_mode & FMODE_WRITER)
+ return -EINVAL;
+
+ old = filp->f_path;
+ inode = path->dentry->d_inode;
+ filp->f_path = *path;
+ filp->f_inode = inode;
+ filp->f_mapping = inode->i_mapping;
+ path_put(&old);
+ return 0;
+}
+
static int do_dentry_open(struct file *f,
struct inode *inode,
int (*open)(struct inode *, struct file *),
@@ -736,6 +754,7 @@ static int do_dentry_open(struct file *f,
error = open(inode, f);
if (error)
goto cleanup_all;
+ inode = f->f_inode;
}
if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
i_readcount_inc(inode);
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index e0ee0b3000b2..260190690674 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -17,6 +17,8 @@
#ifdef CONFIG_UNIX98_PTYS
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp);
+
int devpts_new_index(struct inode *ptmx_inode);
void devpts_kill_index(struct inode *ptmx_inode, int idx);
void devpts_add_ref(struct inode *ptmx_inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 14a97194b34b..045bbfe2ecfc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2272,6 +2272,8 @@ extern struct filename *getname(const char __user *);
extern struct filename *getname_kernel(const char *);
extern void putname(struct filename *name);
+extern int update_file_path(struct file *filp, struct path *path);
+
enum {
FILE_CREATED = 1,
FILE_OPENED = 2
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 77d01700daf7..647d97a4869b 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -45,6 +45,9 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_ROOT 0x2000
#define LOOKUP_EMPTY 0x4000
+extern int path_parent(struct path *root, struct path *path);
+extern int path_pts(struct path *path);
+
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
static inline int user_path_at(int dfd, const char __user *name, unsigned flags,
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 09/13] devpts: Move the creation of /dev/pts/ptmx into fill_super |
| Message-ID | <rkohZ-3Gi-37@gated-at.bofh.it> |
| In reply to | #1371090 |
The code makes more sense here and things are just clearer.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 266a7b7501a6..5356a68863b8 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -379,6 +379,10 @@ static inline void update_ptmx_mode(struct pts_fs_info *fsi)
{
return;
}
+static inline int mknod_ptmx(struct super_block *sb)
+{
+ return 0;
+}
#endif
static int devpts_remount(struct super_block *sb, int *flags, char *data)
@@ -491,11 +495,19 @@ 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) {
+ pr_err("get root dentry failed\n");
+ goto fail;
+ }
- pr_err("get root dentry failed\n");
+ error = mknod_ptmx(s);
+ if (error)
+ goto fail_dput;
+ return 0;
+fail_dput:
+ dput(s->s_root);
+ s->s_root = NULL;
fail:
return error;
}
@@ -564,10 +576,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
if (error)
goto out_undo_sget;
- error = mknod_ptmx(s);
- if (error)
- goto out_undo_sget;
-
s->s_flags |= MS_ACTIVE;
} else {
/* Match mount_single ignore errors on remount */
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 02/13] devpts: More obvious check for the system devpts in pty allocation |
| Message-ID | <rkoi0-3Gi-55@gated-at.bofh.it> |
| In reply to | #1371090 |
This makes the logic of the test clearer, and removes a confusing
use of opts.newinstance, which allows later changes not to worry
if newinstance is set on the primary mount of devpts or not.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c14d51795577..9f22c959d1f7 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -622,7 +622,7 @@ retry:
mutex_lock(&allocated_ptys_lock);
if (pty_count >= pty_limit -
- (fsi->mount_opts.newinstance ? pty_reserve : 0)) {
+ ((devpts_mnt->mnt_sb == sb) ? pty_reserve : 0)) {
mutex_unlock(&allocated_ptys_lock);
return -ENOSPC;
}
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 13/13] devpts: Kill the DEVPTS_MULTIPLE_INSTANCE config option |
| Message-ID | <rkoi0-3Gi-59@gated-at.bofh.it> |
| In reply to | #1371090 |
Retain the code that was previously enabled with
DEVPTS_MULTIPLE_INSTANCES and remove the config option, and kill the
little bit of code that existed only when DEVPTS_MULTIPLE_INSTANCES
was not selected. With the recently updated semantics userspace
actively depends on having multiple instances of devpts for correct
operation.
Having each mount of devpts return a distinct instance ensures that
user space will not accidentally stomp gid or mode devpts options, of
the primary system devpts, by mounting devpts in a chroot environment.
A guarantee that userspace will not stomp attributes of system devpts
removes the need for a setuid root pt_chown executable.
Running a userspace without DEVPTS_MULTIPLE_INSTANCES that has depends
on the current behavior and has removed a setuid root pt_chown
exectuable will allow the system devpts gid and mode options to be
stomped breaking userspace. Which makes the ability to disable the
code previously selected by DEVPTS_MULTIPLE_INSTANCES actively wrong.
The size increase by always using the DEVPTS_MULTIPLE_INSTANCE
path is minimal, and the code is much easier to maintain and
use without having two different code paths to worry about.
The documentation has been updated to relfect this change.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
Documentation/filesystems/devpts.txt | 122 ++++++++++++++---------------------
drivers/tty/Kconfig | 11 ----
fs/devpts/inode.c | 41 ------------
3 files changed, 47 insertions(+), 127 deletions(-)
diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
index 30d2fcb32f72..984b645ac341 100644
--- a/Documentation/filesystems/devpts.txt
+++ b/Documentation/filesystems/devpts.txt
@@ -1,38 +1,32 @@
-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:
-
- - 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.
+Each mount of the devpts filesystem is now a distinct instance from
+all other mounts of devpts. Mount options and indicies of ptys
+allocated in one instance are independent of indices allocated in
+other instances of devpts.
+
+If devpts is mounted without the 'newinstance' option (as in current
+start-up scripts) and if the initial kernel mount of devpts has not
+been exported to userspace the new mount binds to the initial kernel
+mount of devpts.
+
+If devpts is mounted with the 'newinstance' option or the initial
+internal mount of devpts has already been mounted a new instance of
+the devpts filesystem is created. Any ptys created in this instance
+are independent of the ptys created in other instances of devpts, and
+the initial permissions of those ptys are independent from the initial
+permissions from any other instance of the devpts filesystem.
+
+Ideally people will make use of the /dev/pts/ptmx device node to
+create ptys on the devpts filesystem. This can be done by updating
+userspace, bind mounting /dev/pts/ptmx onto /dev/ptmx or making
+/dev/ptmx a symlink to /dev/pts/ptmx.
+
+To be seemlessly backwards compatible an open of /dev/ptmx will look
+to see if the name pts in the same directory is the root directory of
+a devpts filesystem. If that is the case and it is not the initial
+instance of devpts /dev/pts/ptmx is opened. Otherwise the initial
+instance of devpts is opened. In older kernels /dev/ptmx did not
+perform this redirection.
Eg: A container startup script could do the following:
@@ -60,44 +54,34 @@ Per-instance limit could be set by adding mount option "max=<count>".
This feature was added in kernel 3.4 together with sysctl kernel.pty.reserve.
In kernels older than 3.4 sysctl kernel.pty.max works as per-instance limit.
-User-space changes
-------------------
+What user-space needs to do
+----------------------------
-In multi-instance mode (i.e '-o newinstance' mount option is specified at least
-once), following user-space issues should be noted.
+1. If the devpts filesystem is only mounted once /dev/pts/ptmx can be
+ ignored and no change is needed to system-startup scripts.
-1. If -o newinstance mount option is never used, /dev/pts/ptmx can be ignored
- and no change is needed to system-startup scripts.
+2. For best results userspace libraries and applications should be updated
+ to try opening /dev/pts/ptmx before /dev/ptmx, as /dev/pts/ptmx is less
+ ambiguous and higher performance.
-2. To effectively use multi-instance mode (i.e -o newinstance is specified)
- administrators or startup scripts should "redirect" open of /dev/ptmx to
- /dev/pts/ptmx using either a bind mount or symlink.
+3. To effectively use a new instance of devpts open of /dev/ptmx should
+ be redirected to /dev/pts/ptmx using either a bind mount or symlink.
$ mount -t devpts -o newinstance devpts /dev/pts
followed by either
- $ rm /dev/ptmx
$ ln -s pts/ptmx /dev/ptmx
$ chmod 666 /dev/pts/ptmx
or
$ mount -o bind /dev/pts/ptmx /dev/ptmx
-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
+4. The '/dev/ptmx -> pts/ptmx' symlink is the preferred method since it
+ enables better error-reporting and treats all cases the same.
- - 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.
+5. If the system startup scripts do not create /dev/ptmx as a symlink,
+ container-startup scripts should bind mount /dev/ptmx to /dev/pts/ptmx
+ to avoid breaking the rest of the system.
Or, in general, container-startup scripts should use:
@@ -106,11 +90,9 @@ once), following user-space issues should be noted.
mount -o bind /dev/pts/ptmx /dev/ptmx
fi
- 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.
+6. A mount of devpts that is not accompanied by the /dev/ptmx to
+ /dev/pts/ptmx redirection will result in an unusable/unreachable pty,
+ on older kernels.
mount -t devpts -o newinstance lxcpts /dev/pts
@@ -121,21 +103,11 @@ once), following user-space issues should be noted.
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).
+7. The permissions for /dev/pts/ptmx node should be specified when mounting
+ /dev/pts, using the '-o ptmxmode=%o' mount option (default is 0666 was 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.
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 82c4d2e45319..95103054c0e4 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -120,17 +120,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 f86fae8dac0b..9bed39d89cb3 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -109,11 +109,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}
};
@@ -128,7 +126,6 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
return sb->s_fs_info;
}
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
static int devpts_path_ptmx(struct file *filp)
{
struct pts_fs_info *fsi;
@@ -186,12 +183,6 @@ fail:
path_put(&path);
return err;
}
-#else
-static inline int devpts_path_ptmx(struct file *filp)
-{
- return -ENOENT;
-}
-#endif
struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
{
@@ -210,10 +201,8 @@ struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
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
if (!devpts_mnt)
return NULL;
return devpts_mnt->mnt_sb;
@@ -289,7 +278,6 @@ static int parse_mount_options(char *data, 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;
@@ -303,7 +291,6 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
return -EINVAL;
opts->max = option;
break;
-#endif
default:
pr_err("called with bogus options\n");
return -EINVAL;
@@ -313,7 +300,6 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
return 0;
}
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
static int mknod_ptmx(struct super_block *sb)
{
int mode;
@@ -374,16 +360,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;
-}
-static inline int mknod_ptmx(struct super_block *sb)
-{
- return 0;
-}
-#endif
static int devpts_remount(struct super_block *sb, int *flags, char *data)
{
@@ -417,11 +393,9 @@ static int devpts_show_options(struct seq_file *seq, struct dentry *root)
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;
}
@@ -512,7 +486,6 @@ fail:
return error;
}
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
/*
* devpts_mount()
*
@@ -565,18 +538,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
return root;
}
-#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);
@@ -591,9 +552,7 @@ static struct file_system_type devpts_fs_type = {
.name = "devpts",
.mount = devpts_mount,
.kill_sb = devpts_kill_sb,
-#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
.fs_flags = FS_USERNS_MOUNT | FS_USERNS_DEV_MOUNT,
-#endif
};
/*
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 03:50 +0200 |
| Subject | [PATCH 07/13] devpts: Move parse_mount_options into fill_super |
| Message-ID | <rkoi0-3Gi-65@gated-at.bofh.it> |
| In reply to | #1371090 |
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 80c78bb472a9..46d633ab16c7 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -462,6 +462,7 @@ static int
devpts_fill_super(struct super_block *s, void *data, int silent)
{
struct inode *inode;
+ int error;
s->s_blocksize = 1024;
s->s_blocksize_bits = 10;
@@ -469,10 +470,16 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
s->s_op = &devpts_sops;
s->s_time_gran = 1;
+ error = -ENOMEM;
s->s_fs_info = new_pts_fs_info();
if (!s->s_fs_info)
goto fail;
+ error = parse_mount_options(data, &DEVPTS_SB(s)->mount_opts);
+ if (error)
+ goto fail;
+
+ error = -ENOMEM;
inode = new_inode(s);
if (!inode)
goto fail;
@@ -490,7 +497,7 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
pr_err("get root dentry failed\n");
fail:
- return -ENOMEM;
+ return error;
}
#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
@@ -557,10 +564,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
if (error)
goto out_undo_sget;
- error = parse_mount_options(data, &DEVPTS_SB(s)->mount_opts);
- if (error)
- goto out_undo_sget;
-
error = mknod_ptmx(s);
if (error)
goto out_undo_sget;
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 04:00 +0200 |
| Subject | [PATCH 12/13] devpts: Always return a distinct instance when mounting |
| Message-ID | <rkorE-3JL-5@gated-at.bofh.it> |
| In reply to | #1371090 |
When devpts is mounted and the newinstance flag is not passed the code
first checks to see if the system devpts instance has been exported to
userspace. If it has not the system devpts instance is returned
otherwise a fresh instance of devpts is allocated and returned.
If newinstance is passed a fresh devpts instance is always returned.
Combined with the earlier work to cause mounts of devpts to fail
if devpts is mounted over itself, this ensures that the system devpts
is mounted on /dev/pts on all of the systems I have tested.
This has been verified to work properly on openwrt-15.05, centos5,
centos6, centos7, debian-6.0.2, debian-7.9, debian-8.2, ubuntu-14.04.3,
ubuntu-15.10, fedora23, magia-5, mint-17.3, opensuse-42.1, slackware-14.1,
gentoo-20151225 (13.0?), archlinux-2015-12-01
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 33 ++++++++++++---------------------
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 7b4fe0d4018d..f86fae8dac0b 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -513,13 +513,6 @@ fail:
}
#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
-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()
*
@@ -550,28 +543,26 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
static struct dentry *devpts_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
- struct super_block *s;
+ struct dentry *root;
bool newinstance;
newinstance = parse_newinstance(data);
+ if (flags & MS_KERNMOUNT)
+ newinstance = true;
- /* Require newinstance for all user namespace mounts to ensure
+ /* Force newinstance for all user namespace mounts to ensure
* the mount options are not changed.
*/
- if ((current_user_ns() != &init_user_ns) && !newinstance)
- return ERR_PTR(-EINVAL);
-
- if (newinstance)
- return mount_nodev(fs_type, flags, data, devpts_fill_super);
-
- s = sget(fs_type, compare_init_pts_sb, set_anon_super, flags, NULL);
- if (IS_ERR(s))
- return ERR_CAST(s);
+ if (current_user_ns() != &init_user_ns)
+ newinstance = true;
- /* Match mount_single ignore errors on remount */
- devpts_remount(s, &flags, data);
+ root = NULL;
+ if (!newinstance)
+ root = mount_super_once(devpts_mnt->mnt_sb, flags, data);
+ if (IS_ERR_OR_NULL(root))
+ root = mount_nodev(fs_type, flags, data, devpts_fill_super);
- return dget(s->s_root);
+ return root;
}
#else
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 04:00 +0200 |
| Subject | [PATCH 11/13] vfs: Implement mount_super_once |
| Message-ID | <rkorE-3JL-7@gated-at.bofh.it> |
| In reply to | #1371090 |
The devpts filesystem has a notion of a system or primary instance of
devpts. To retain the notion of a primary system instance of devpts
the code needs a way to allow userspace to mount the internally
mounted instance of devpts when it is not currently mounted by
userspace. The new helper mount_super_once allows that.
Ideally mount_super_once would ignore still referenced lazy unmounts,
but in testing I was not able to find an existing distribution that
cared. Since no one actually cares I did not try and solve the
formidable challenges of adding a test to ignore still referenced lazy
mounts in a race free way.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/super.c | 12 ++++++++++++
include/linux/fs.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/fs/super.c b/fs/super.c
index 74914b1bae70..98a569412036 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1101,6 +1101,18 @@ struct dentry *mount_single(struct file_system_type *fs_type,
}
EXPORT_SYMBOL(mount_single);
+struct dentry *mount_super_once(struct super_block *sb, int flags, void *data)
+{
+ /* Allow mounting the specified superblock by userspace if there
+ * are not any existing userspace mounts of it.
+ */
+ if (atomic_cmpxchg(&sb->s_active, 1, 2) != 1)
+ return ERR_PTR(-EBUSY);
+ down_write(&sb->s_umount);
+ do_remount_sb(sb, flags, data, 0);
+ return dget(sb->s_root);
+}
+
struct dentry *
mount_fs(struct file_system_type *type, int flags, const char *name, void *data)
{
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 02a980bfad5c..515f874a6907 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2002,6 +2002,8 @@ extern struct dentry *mount_single(struct file_system_type *fs_type,
extern struct dentry *mount_nodev(struct file_system_type *fs_type,
int flags, void *data,
int (*fill_super)(struct super_block *, void *, int));
+extern struct dentry *mount_super_once(struct super_block *sb,
+ int flags, void *data);
extern struct dentry *mount_subtree(struct vfsmount *mnt, const char *path);
void generic_shutdown_super(struct super_block *sb);
void kill_block_super(struct super_block *sb);
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | "Eric W. Biederman" <ebiederm@xmission.com> |
|---|---|
| Date | 2016-04-05 04:00 +0200 |
| Subject | [PATCH 10/13] devpts: Simplify devpts_mount by using mount_nodev |
| Message-ID | <rkorF-3JL-21@gated-at.bofh.it> |
| In reply to | #1371090 |
Now that all of the work of setting up a superblock has been moved to
devpts_fill_super simplify devpts_mount by calling mount_nodev instead
of rolling mount_nodev by hand.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/devpts/inode.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 5356a68863b8..7b4fe0d4018d 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -550,7 +550,6 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
static struct dentry *devpts_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
- int error;
struct super_block *s;
bool newinstance;
@@ -563,30 +562,16 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
return ERR_PTR(-EINVAL);
if (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);
+ return mount_nodev(fs_type, flags, data, devpts_fill_super);
+ 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;
- } else {
- /* Match mount_single ignore errors on remount */
- devpts_remount(s, &flags, data);
- }
+ /* Match mount_single ignore errors on remount */
+ devpts_remount(s, &flags, data);
return dget(s->s_root);
-
-out_undo_sget:
- deactivate_locked_super(s);
- return ERR_PTR(error);
}
#else
--
2.6.3
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-04-05 05:00 +0200 |
| Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rkpnI-4zp-11@gated-at.bofh.it> |
| In reply to | #1371090 |
On Mon, Apr 04, 2016 at 08:29:17PM -0500, Eric W. Biederman wrote:
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +static int devpts_path_ptmx(struct file *filp)
> +{
> + struct pts_fs_info *fsi;
> + struct path root, path;
> + struct dentry *old;
> + int err = -ENOENT;
> + int ret;
> +
> + /* Can the pts filesystem be found with a path walk? */
> + path = filp->f_path;
> + path_get(&path);
> + get_fs_root(current->fs, &root);
> + ret = path_parent(&root, &path);
> + path_put(&root);
> + if (ret != 1)
> + goto fail;
That, I take it, is a lookup for .. and buggering off if it fails *or* if
we had been in caller's root or something that overmount it? Not that the
latter had been possible - root is a directory and can be overmounted only
by another such, and we are called from ->open() of a device node.
> + /* Remember the result of this permission check for later */
> + ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
> + if (path_pts(&path))
> + goto fail;
Egads, man - you've just introduced a special function for looking up
something named "pts" in a given directory!
The reason not to use kern_path() would be what, the fact that it doesn't
allow starting at given location? So let's make a variant that would - and
rather than bothering with RCU, just go for something like (completely
untested)
/* on success overwrite *path with the result of walk; do _not_ drop the
reference to old contents - let the caller arrange that */
int kern_path_relative(struct path *path, const char *s, int flags)
{
int err;
struct nameidata nd = {.path = *path};
struct filename *name;
if (!*s || *s == '/' || flags & (LOOKUP_ROOT | LOOKUP_RCU))
return -EINVAL;
name = getname_kernel(s);
if (IS_ERR(name))
return PTR_ERR(name);
set_nameidata(&nd, AT_FDCWD, name);
nd.last_type = LAST_ROOT;
nd.flags = flags | LOOKUP_REVAL | LOOKUP_JUMPED | LOOKUP_PARENT;
nd.m_seq = read_seqbegin(&mount_lock);
path_get(&nd.path);
nd.inode = nd.path.dentry->d_inode;
while (!(err = link_path_walk(s, &nd))
&& ((err = lookup_last(&nd)) > 0)) {
s = trailing_symlink(&nd);
if (IS_ERR(s)) {
err = PTR_ERR(s);
break;
}
}
if (!err)
err = complete_walk(&nd);
if (!err && flags & LOOKUP_DIRECTORY)
if (!d_can_lookup(nd.path.dentry))
err = -ENOTDIR;
if (!err) {
*path = nd.path;
nd.path.mnt = NULL;
nd.path.dentry = NULL;
}
terminate_walk(&nd);
restore_nameidata();
putname(name);
return err;
}
and use it as
path = filp->f_path;
err = kern_path_relative(&path, "../pts", LOOKUP_DIRECTORY);
if (err)
return err;
/* from here on we need to path_put() it */
if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
goto fail;
/* must be its root; no other directories on that puppy */
> + fsi = DEVPTS_SB(path.mnt->mnt_sb);
> +
> + /* Get out if the path walk resulted in the default devpts instance */
> + if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
> + goto fail;
> +
> + /* Don't allow bypassing the existing /dev/pts/ptmx permission check */
err = inode_permission(path.dentry->d_inode, MAY_EXEC);
if (err)
goto fail;
err = inode_permission(fsi->ptmx_dentry->d_inode,
ACC_MODE(filp->f_flags));
if (err)
goto fail;
> + /* Advance path to the ptmx dentry */
> + old = path.dentry;
> + path.dentry = dget(fsi->ptmx_dentry);
> + dput(old);
> +
> + /* Make it look like /dev/pts/ptmx was opened */
> + err = update_file_path(filp, &path);
> + if (err)
> + goto fail;
> +
> + return 0;
> +fail:
> + path_put(&path);
> + return err;
> +}
> +#else
> +static inline int devpts_path_ptmx(struct file *filp)
> +{
> + return -ENOENT;
> +}
> +#endif
> +
> +struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
> +{
> + int err;
> + if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> + return inode;
> +
> + err = devpts_path_ptmx(filp);
> + if (err == 0)
> + return filp->f_inode;
> + if (err != -ENOENT)
> + return ERR_PTR(err);
> +
> + return inode;
> +}
Umm... I'm not sure it makes for good calling conventions - the caller can
do inode = file_inode(filp) just as well, so why not simply return 0 or -E...?
"return inode;" cases become simply return 0...
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1415,29 +1415,41 @@ static void follow_mount(struct path *path)
> }
> }
>
> -static int follow_dotdot(struct nameidata *nd)
> +int path_parent(struct path *root, struct path *path)
Please, don't.
> +#ifdef CONFIG_UNIX98_PTYS
> +int path_pts(struct path *path)
Fuck, no.
> index 17cb6b1dab75..e1ed78fa474b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -679,6 +679,24 @@ int open_check_o_direct(struct file *f)
> return 0;
> }
>
> +int update_file_path(struct file *filp, struct path *path)
> +{
> + /* Only valid during f_op->open, and even in open use very carefully */
> + struct path old;
> + struct inode *inode;
> +
> + if (filp->f_mode & FMODE_WRITER)
> + return -EINVAL;
That really needs to be commented.
> + old = filp->f_path;
> + inode = path->dentry->d_inode;
> + filp->f_path = *path;
> + filp->f_inode = inode;
> + filp->f_mapping = inode->i_mapping;
> + path_put(&old);
> + return 0;
> +}
> +
> static int do_dentry_open(struct file *f,
> struct inode *inode,
> int (*open)(struct inode *, struct file *),
> @@ -736,6 +754,7 @@ static int do_dentry_open(struct file *f,
> error = open(inode, f);
> if (error)
> goto cleanup_all;
> + inode = f->f_inode;
> }
> if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
> i_readcount_inc(inode);
BTW, have you looked through the callers of dentry_open()? It can hit that
case as well...
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-04-05 05:10 +0200 |
| Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rkpxn-4WF-5@gated-at.bofh.it> |
| In reply to | #1371147 |
On Tue, Apr 05, 2016 at 03:54:25AM +0100, Al Viro wrote: > That, I take it, is a lookup for .. and buggering off if it fails *or* if > we had been in caller's root or something that overmount it? Not that the > latter had been possible - root is a directory and can be overmounted only > by another such, and we are called from ->open() of a device node. > > > + /* Remember the result of this permission check for later */ > > + ret = inode_permission(path.dentry->d_inode, MAY_EXEC); > > + if (path_pts(&path)) > > + goto fail; > > Egads, man - you've just introduced a special function for looking up > something named "pts" in a given directory! > > The reason not to use kern_path() would be what, the fact that it doesn't > allow starting at given location? So let's make a variant that would - and > rather than bothering with RCU, just go for something like (completely > untested) Ah... Right, that would demand exec permissions on the starting point. Still, this is incredibly ugly ;-/ I'll try to come up with something more tolerable, but this "path_pts" thing is too ugly to live. Seriously.
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2016-04-08 21:10 +0200 |
| Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rlJX4-7e-29@gated-at.bofh.it> |
| In reply to | #1371152 |
Al Viro <viro@ZenIV.linux.org.uk> writes: > On Tue, Apr 05, 2016 at 03:54:25AM +0100, Al Viro wrote: > >> That, I take it, is a lookup for .. and buggering off if it fails *or* if >> we had been in caller's root or something that overmount it? Not that the >> latter had been possible - root is a directory and can be overmounted only >> by another such, and we are called from ->open() of a device node. >> >> > + /* Remember the result of this permission check for later */ >> > + ret = inode_permission(path.dentry->d_inode, MAY_EXEC); >> > + if (path_pts(&path)) >> > + goto fail; >> >> Egads, man - you've just introduced a special function for looking up >> something named "pts" in a given directory! >> >> The reason not to use kern_path() would be what, the fact that it doesn't >> allow starting at given location? So let's make a variant that would - and >> rather than bothering with RCU, just go for something like (completely >> untested) > > Ah... Right, that would demand exec permissions on the starting point. > Still, this is incredibly ugly ;-/ I'll try to come up with something > more tolerable, but this "path_pts" thing is too ugly to live. > Seriously. Given that I can think of no other reason than this special case to ever want to use this code. I figured having something incredibily special case and obviously so was the way to go. Then at least no one would mistake it for a general purpose facility. Eric
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-04-07 18:10 +0200 |
| Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rlkFl-6uI-37@gated-at.bofh.it> |
| In reply to | #1371090 |
On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> In practice I expect the permission checks are a non-issue as the
> permissions on /dev/ptmx and /dev/pts/ptmx are always 0666.
So I think this is still entirely wrongheaded, and thinking about the
problem the wrong way around.
The issue is *not* that the "permissions on /dev/ptmx and
/dev/pts/ptmx are always 0666". Not at all.
The permissions of /dev/ptmx and /dev/pts/ptmx are simply *irrelevant*.
We're not interested in opening /dev/ptmx. We are interested in
looking up *which* ptmx that pty is associated with.
Those are two totally different issues.
We never opened /dev./ptmx before either, and we never ever cared
about the permiossions of it. We just hardcoded which superblock we
were using, regardless of those permissions.
We should basically continue to do the exact same thing. We don't care
about the permissions of the ptmx entry, and we're not even interested
in opening it (it's sufficient to just find the "pts" subdirectory),
we are _purely_ asking "which superblock/mount am I associated with".
In other words, we *could* do this by doing some insane parsing of
/proc/mounts, but that would be stupid.
My point is, talking about permissions of these nodes is _wrong_. It's
actively misleading. It is exactly the wrong thing to do, because it
confuses people into thinking that we somehow care, and that we
somehow open the new node. We don't. We're opening the *old* pathname,
the one whose permissions we already checked when we walked it, and
we're just looking up the pts directory so that we don't hardcode
which set of pty's we're talking about.
So I think the part of the patch where you check permissions is wrong.
I think the part of the commit message where you talk about this is
confused.
You should make this about looking up the superblock, and explicitly
talk about how this is *not* about permissions.
So get rid of all the pointless "inode_permission()" crap. We already
checked that by virtue of us opening "/dev/ptmx". THOSE permissions
matter, but they were already done. Now we're just saying "ok, the
user has a right to open the ptmx node, now _which_ devpts is that
ptmx node for?"
So also get rid of this:
+ /* Advance path to the ptmx dentry */
+ old = path.dentry;
+ path.dentry = dget(fsi->ptmx_dentry);
+ dput(old);
entirely. It's wrong. It's entirely pointless. We don't even care
about "what does pts/ptmx point to". We care about "which superblock
do we get when we look up the "pts/" subdirectory in the dentry cache
for this user (without permissions)"/
So get rid of all the pathname games. Just save the superblock pointer
in file->f_private or somewhere like that, and make it really clear
that what we are doing is making "/dev/ptmx" work sanely! The user is
not looking up "/dev/pts/ptmx". They are looking up "/dev/ptmx".
See the difference?
Linus
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2016-04-08 21:10 +0200 |
| Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rlJX4-7e-27@gated-at.bofh.it> |
| In reply to | #1373511 |
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> In practice I expect the permission checks are a non-issue as the
>> permissions on /dev/ptmx and /dev/pts/ptmx are always 0666.
>
> So I think this is still entirely wrongheaded, and thinking about the
> problem the wrong way around.
No. You are missing my concern.
My concern is that I suspect someone somewhere has created a chroot
environment. That chroot environment has devpts mounted with
"-o newinstance" and has set the permissions of /dev/pts/ptmx such
that only users in that container can create ptys on that instance
of devpts.
Being a mischevious user outside the container I can create a new user
namespace and a new mount namespace and bind mount our new and improved
version of /dev/ptmx right next to the chroot's /dev/pts mount.
Then because the permissions on /dev/ptmx are different than on the
chroots /dev/pts/ptmx I can create ptys that I could not have before
hand. Bypassing the existing permissions.
Given that concern under the rule we don't break userspace we have to
check the permissions of /dev/pts/ptmx when we are creating a new pty,
on a instance of devpts that was created with newinstance.
Short of saying we simply don't care about such users I don't see a way
we can allow bypassing the existing permission check.
Now I do think we can remove the permission check altogether. At this
point POSIX does not even require the existince of any files or device
nodes, and FreeBSD proves out that users of ptys don't care by
implementing a dedicated system call to create ptys that does contain
any permission checks. So only the small set of linux specific
chroot/container creating applications might care. As the permissions
were not in any way the focus of this patchset I choose not to tackle
a possible user visible change like this.
>
> So get rid of all the pointless "inode_permission()" crap. We already
> checked that by virtue of us opening "/dev/ptmx". THOSE permissions
> matter, but they were already done. Now we're just saying "ok, the
> user has a right to open the ptmx node, now _which_ devpts is that
> ptmx node for?"
I wish I could in conscience do that. But unless we decide that
permission are irrelevant we are adding a permission bypass for an
existing operation. Typically that is called a security bug. I am not
comfortable doing that unless we simply decide we don't care.
If we decide we don't care I will add a patch at the front of the
patchset that implements don't care before all of the rest of this.
> So also get rid of this:
>
> + /* Advance path to the ptmx dentry */
> + old = path.dentry;
> + path.dentry = dget(fsi->ptmx_dentry);
> + dput(old);
>
> entirely. It's wrong. It's entirely pointless. We don't even care
> about "what does pts/ptmx point to". We care about "which superblock
> do we get when we look up the "pts/" subdirectory in the dentry cache
> for this user (without permissions)"/
Actually it is not pointless. There is a second issue in all of this.
Right now it is possible to confuse the pt_chown setuid root binary
about which instance of devpts it should be calling chmod on. I am not
certain it even ensures it is calling chown on a devpts entry. It is
just hard coded paths today.
Right now userspace does something like:
masterfd = posix_openpt(O_RDWR);
grantpt(masterfd);
char *slave_name = ptsname(masterfd);
slavefd = open(slave_name);
Furthemore invoked by grantpt execs the pt_chown binary which does:
int pty_number;
ioctl(masterfd, TIOCGPTN, &pty_number)
sprintf(slave_name, "/dev/pty/%u", pty_number);
chown(slave_name, some_uid, some_gid);
It would be very nice if we could have a way to close the races
in this mess and allow a program like pt_chown to actually only affect
the pty it cares about. There is only one way I know to implement this
in a backwards compatible way and that is to have
readlink("/proc/self/fd/${masterfd}"), stat("/proc/self/fd/${masterfd}",
and open("/proc/self/fd/${masterfd}") talk about "/dev/pts/ptmx"
for a non-system instance of devpts
That will at least allow ptsname to find the proper instance of devpts.
I suppose it becomes hameless if grantpt stops calling a setuid root
exectuable if the connection between the master and the slave pty
gets confused and pt_chown, does not exist anymore. But it feels
wrong to allow userspace no way to ask the question which mounted
instance of devpts does this masterfd belong to. Especially
as readlink("/proc/self/fd/${masterfd}") is the natural way to ask that.
If anyone has a better idea on how userspace should connect the master
pty file descriptor the slave file descriptor, I would be willing to
implement that instead.
> So get rid of all the pathname games. Just save the superblock pointer
> in file->f_private or somewhere like that, and make it really clear
> that what we are doing is making "/dev/ptmx" work sanely! The user is
> not looking up "/dev/pts/ptmx". They are looking up "/dev/ptmx".
>
> See the difference?
The change that introduces devpts_add_ref devpts_del_ref takes care of
all of the needed reference counting of the super block level.
Apparently the interactions with /dev/tty require the tty's to have a
reference to the superblock.
The natural place to store the superblock in tty->driver_data. I even
took a look at that before posting my patches. Unfortunately there
is at least devpts_pty_kill that actually need the slave inode. So for
the slave side there does not appear to be a location where we can just
use the superblock. Furthermore most of the methods between the master
side and the slave side are shared so having tty->driver_data be an
inode pointer in one place and super_block pointer in another is tricky
to implement.
Given that it was all crazy weird and goofy and I was have a very hard
time tracing it, I figured the better part of valour was not attempting
to change code I was having trouble tracing.
That is the only reason why I do not refer to the super_block directly
from the tty layer. I actually have patches that get as far as killing
pts_sb_from_inode.
Or in summary the decisions you question most I have made not for
implementation reasons but for user space api reasons. The permission
check because I don't want to break existing userspace, and the
change of file path to allow the natural way to ask the question which
devpts does this master file descriptor belong to.
I am open to better ideas.
Eric
p.s. In the long term I think we should just update glibc and the
handful of ther libraries that care to prefer to use /dev/pts/ptmx if it
is available over /dev/ptmx and then make /dev/ptmx and all of the hacks
for supporting it a configuration option which in a decade or so can be
turned off by default.
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-04-08 21:10 +0200 |
| Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| Message-ID | <rlJX5-7e-49@gated-at.bofh.it> |
| In reply to | #1374430 |
On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Given that concern under the rule we don't break userspace we have to
> check the permissions of /dev/pts/ptmx when we are creating a new pty,
> on a instance of devpts that was created with newinstance.
The rule is that we don't break existing installations.
If somebody has root and installs a "ptmx" node in an existing mount
space next to a pts subdirectory, that's not a security issue, nor is
it going to break any existing installation.
The whole point of the patch is that yes, we change semantics. A
change of semantics means that people will see situations where the
behavior is different. But that's not "breaking user space", that's
just "ok, you can see a difference".
Linus
[toc] | [prev] | [next] | [standalone]
Page 1 of 4 [1] 2 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web