Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
Konstantin Khlebnikov writes: > On Wed, Apr 20, 2016 at 5:55 PM, Eric W. Biederman > wrote: >> Linus Torvalds writes: >> >>> On Tue, Apr 19, 2016 at 9:36 PM, Konstantin Khlebnikov >>> wrote: On Wed, Apr 20, 2016 at 6:04 AM, Eric W. Biederman > > The kernel.pty.reserve sysctl is neutered with no way currently > implemented to be able to use the reserved ptys. I think we could convert this into reserve for init user namespace, ssh in host will work even if containers eaten all ptys. >>> >>> Yes. That's basically how it effectively worked before (ie everything >>> but the initial non-newinstance devpts mount would be limited to the >>> non-reserved numbers). >>> >>> We required the non-init namespaces to do a newinstance mount, so the >>> whole test for "newinstance" was effectively the same thing as just >>> checking for the init namespace from a security standpoint. >>> >>> And in fact, rewriting it in that form (ie checking for init_ns) would >>> just make it much more obvious what the intent it. >> >> How does this sound. >> >> When mounting a devpts filesystem. We look at the caller (aka current) >> and if we are in the initial mount namespace set a flag in fsi that >> allows that instance of devpts to draw into the reserve pool. > > Maybe just check current user namespace when task opens /dev/ptmx? > > IIRR now check looks like: count < limit - (newinstance ? reserved : 0). > So, it will be count < limit - (current_in_init_userns ? 0 : newinstance). Looking at current user namespace really is not enough. Lots of container solutions at least historically (which means deployed right now) don't use the user namespace. I can see an argument to make the check: "capable(CAP_SYS_RESOURCE)". Although for pty applications I don't know if that is particularly meaningful. I am a little dubious of making it a check at allocation time rather than at mount time. The issue is that tty allocation is an unprivileged operation. I expect applications such as sshd (the one that really matters) will have droped privileges by the time they allocation a pty. So I feel much more comfortable with a model where things are arranged so that applications within access of the devpts filesystem can use it (and are not limited), and applications not in range can't. Roughly the authenticate at open time model. Also what devpts implements today. The is also the question of how things should work if you are running in a system where every new daemon, and every new login is in it's own mount namespace. Allowing each of these to have a distinct /tmp directory. I believe systemd systems are well on their way to doing that today. As such it does not seem appropriate to check the mount namespace of the opener of the tty. Who knows we may not be long until the pty master lives in some very tight bubble where it can barely do anything (as that is the program that talks to the network) and user namespaces are used as part of the enforcement of that. For all of those reasons, a permission check in devpts_pty_new seems like the wrong place. Eric
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
On Wed, Apr 20, 2016 at 5:55 PM, Eric W. Biederman wrote: > Linus Torvalds writes: > >> On Tue, Apr 19, 2016 at 9:36 PM, Konstantin Khlebnikov >> wrote: >>> On Wed, Apr 20, 2016 at 6:04 AM, Eric W. Biederman The kernel.pty.reserve sysctl is neutered with no way currently implemented to be able to use the reserved ptys. >>> >>> I think we could convert this into reserve for init user namespace, >>> ssh in host will work even if containers eaten all ptys. >> >> Yes. That's basically how it effectively worked before (ie everything >> but the initial non-newinstance devpts mount would be limited to the >> non-reserved numbers). >> >> We required the non-init namespaces to do a newinstance mount, so the >> whole test for "newinstance" was effectively the same thing as just >> checking for the init namespace from a security standpoint. >> >> And in fact, rewriting it in that form (ie checking for init_ns) would >> just make it much more obvious what the intent it. > > How does this sound. > > When mounting a devpts filesystem. We look at the caller (aka current) > and if we are in the initial mount namespace set a flag in fsi that > allows that instance of devpts to draw into the reserve pool. Maybe just check current user namespace when task opens /dev/ptmx? IIRR now check looks like: count < limit - (newinstance ? reserved : 0). So, it will be count < limit - (current_in_init_userns ? 0 : newinstance). > > That will still allow crazy pieces of code like xen-create-instance run > by root that mount a devpts filesystem in a chroot environment to draw > into the reserved pool, but any sane users that set up their own mount > namespace won't be able to user the reserve pool. > > I believe that will give an almost identical policy to what we have > today, and it certainly makes a good default test for a container. Just > for cleanliness containers (of anyone's definition) almost always use > mount namespaces instead of chroots. > > Sigh one last past through all of the distros, to confirm that this > works in practice. > > Eric
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
Linus Torvalds writes: > On Tue, Apr 19, 2016 at 9:36 PM, Konstantin Khlebnikov > wrote: >> On Wed, Apr 20, 2016 at 6:04 AM, Eric W. Biederman >>> >>> The kernel.pty.reserve sysctl is neutered with no way currently >>> implemented to be able to use the reserved ptys. >> >> I think we could convert this into reserve for init user namespace, >> ssh in host will work even if containers eaten all ptys. > > Yes. That's basically how it effectively worked before (ie everything > but the initial non-newinstance devpts mount would be limited to the > non-reserved numbers). > > We required the non-init namespaces to do a newinstance mount, so the > whole test for "newinstance" was effectively the same thing as just > checking for the init namespace from a security standpoint. > > And in fact, rewriting it in that form (ie checking for init_ns) would > just make it much more obvious what the intent it. How does this sound. When mounting a devpts filesystem. We look at the caller (aka current) and if we are in the initial mount namespace set a flag in fsi that allows that instance of devpts to draw into the reserve pool. That will still allow crazy pieces of code like xen-create-instance run by root that mount a devpts filesystem in a chroot environment to draw into the reserved pool, but any sane users that set up their own mount namespace won't be able to user the reserve pool. I believe that will give an almost identical policy to what we have today, and it certainly makes a good default test for a container. Just for cleanliness containers (of anyone's definition) almost always use mount namespaces instead of chroots. Sigh one last past through all of the distros, to confirm that this works in practice. Eric
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
On Tue, Apr 19, 2016 at 9:36 PM, Konstantin Khlebnikov wrote: > On Wed, Apr 20, 2016 at 6:04 AM, Eric W. Biederman >> >> The kernel.pty.reserve sysctl is neutered with no way currently >> implemented to be able to use the reserved ptys. > > I think we could convert this into reserve for init user namespace, > ssh in host will work even if containers eaten all ptys. Yes. That's basically how it effectively worked before (ie everything but the initial non-newinstance devpts mount would be limited to the non-reserved numbers). We required the non-init namespaces to do a newinstance mount, so the whole test for "newinstance" was effectively the same thing as just checking for the init namespace from a security standpoint. And in fact, rewriting it in that form (ie checking for init_ns) would just make it much more obvious what the intent it. Linus
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
On Wed, Apr 20, 2016 at 6:04 AM, Eric W. Biederman wrote: > > The /dev/ptmx device node is changed to lookup the directory entry > "pts" in the same directory as the /dev/ptmx device node was opened > in. If there is a "pts" entry and that entry is a devpts filesystem > /dev/ptmx uses that filesystem. Otherwise the open of /dev/ptmx > fails. > > The DEVPTS_MULTIPLE_INSTANCES configuration option is removed, > so that userspace can now safely depend on each mount of devpts > creating a new instance of the filesystem. > > Each mount of devpts is now a separate and equal filesystem. > > The kernel.pty.reserve sysctl is neutered with no way currently > implemented to be able to use the reserved ptys. I think we could convert this into reserve for init user namespace, ssh in host will work even if containers eaten all ptys. > > A new vfs helper path_pts is introduced that finds a directory entry > named "pts" in the directory of the passed in path, and changes the > passed in path to point to it. The helper path_pts uses a function > path_parent_directory that was factored out of follow_dotdot. > > In the implementation of devpts: > - devpts_mnt is killed as it is no longer meaningful if all > mounts of devpts are equal. > - pts_sb_from_inode is replaced by just inode->i_sb as all > cached inodes in the tty layer are now from the devpts > filesystem. > - devpts_add_ref is rolled into the new function devpts_ptmx. > And the unnecessary inode hold is removed. > - devpts_del_ref is renamed devpts_release and reduced > to just a deacrivate_super. > - The newinstance mount option continues to be accepted but is now ignored. > > In devpts_fs.h definitions for when !CONFIG_UNIX98_PTYS are removed > as they are never used. > > Documentation/filesystems/devices.txt is updated to describe > the current situation. > > 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. With the caveat that on centos6 and on > slackware-14.1 that there wind up being two instances of the devpts > filesystem mounted on /dev/pts, the lower copy does not end up getting > used. > > Signed-off-by: "Eric W. Biederman" > --- > Documentation/filesystems/devpts.txt | 145 +++-- > drivers/tty/Kconfig | 11 -- > drivers/tty/pty.c| 41 --- > fs/devpts/inode.c| 205 > +-- > fs/namei.c | 58 -- > include/linux/devpts_fs.h| 31 ++ > include/linux/namei.h| 2 + > 7 files changed, 148 insertions(+), 345 deletions(-)
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
Al Viro writes:
> On Tue, Apr 19, 2016 at 10:43:03PM -0500, Eric W. Biederman wrote:
>> >> + if (!d_can_lookup(parent))
>> >> + return -ENOENT;
>> >
>> > And how, pray tell, would a parent of anything fail to be a directory?
>>
>> It is to make that function be visually distinct from path_parentat
>> which does something rather different.
>
> Huh? I'm asking how can that condition ever turn out to be true. Unless
> you really advocate something like
> if (2 * 17 != 34)
> return -234567; // to make it visually distinct from foobar(),
> // which doesn't have such a test
> your reply doesn't seem to make any sense...
Oh apologies I thought you were asking about the naming of the function,
path_parent_directory. Yes. The d_can_lookup does appear to be redundant.
It definitely looks like bedtime for me.
>> >> + 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);
>> >
>> > What the hell for? What does that lock on parent change for the
>> > dcache lookup you are doing here?
>>
>> Good point. That is overkill. As we know the dentry is a mount point and
>> must be in the dcache, the customary lock for performing a lookup from
>> the disk is not necessary.
>
> Er... To avoid reader confusion:
> a) d_lookup() does *not* do a filesystem lookup
> b) it does not need inode_lock()
> c) it (and not a "lookup from the disk") is what's actually being
> called in the code in question.
And since I was stripping down the ordinary filesystem lookup path to
just the pieces needed I apparently wound up with a few extras.
Do you think it would be possible to guarantee an rcu lookup for the
operations in path_pts? I think needing to perform a follow_mount makes
that impossible to guarantee.
All the caller wants is to find the superblock of the mounted filesystem
and increment sb->s_active.
Eric
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
On Tue, Apr 19, 2016 at 10:43:03PM -0500, Eric W. Biederman wrote:
> >> + if (!d_can_lookup(parent))
> >> + return -ENOENT;
> >
> > And how, pray tell, would a parent of anything fail to be a directory?
>
> It is to make that function be visually distinct from path_parentat
> which does something rather different.
Huh? I'm asking how can that condition ever turn out to be true. Unless
you really advocate something like
if (2 * 17 != 34)
return -234567; // to make it visually distinct from foobar(),
// which doesn't have such a test
your reply doesn't seem to make any sense...
> >> + 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);
> >
> > What the hell for? What does that lock on parent change for the
> > dcache lookup you are doing here?
>
> Good point. That is overkill. As we know the dentry is a mount point and
> must be in the dcache, the customary lock for performing a lookup from
> the disk is not necessary.
Er... To avoid reader confusion:
a) d_lookup() does *not* do a filesystem lookup
b) it does not need inode_lock()
c) it (and not a "lookup from the disk") is what's actually being
called in the code in question.
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
Al Viro writes:
> On Tue, Apr 19, 2016 at 10:04:20PM -0500, Eric W. Biederman wrote:
>> +#ifdef CONFIG_UNIX98_PTYS
>> +int path_pts(struct path *path)
>> +{
>> +/* Find "pts" in the same directory as the input path */
>> +struct dentry *child, *parent;
>> +struct qstr this;
>> +int ret;
>> +
>> +ret = path_parent_directory(path);
>> +if (ret)
>> +return ret;
>> +
>> +parent = path->dentry;
>> +if (!d_can_lookup(parent))
>> +return -ENOENT;
>
> And how, pray tell, would a parent of anything fail to be a directory?
It is to make that function be visually distinct from path_parentat
which does something rather different.
>> +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);
>
> What the hell for? What does that lock on parent change for the
> dcache lookup you are doing here?
Good point. That is overkill. As we know the dentry is a mount point and
must be in the dcache, the customary lock for performing a lookup from
the disk is not necessary.
>> +child = d_lookup(parent, &this);
>> +inode_unlock(parent->d_inode);
>> +if (!child)
>> +return -ENOENT;
>
> Take a look at d_hash_and_lookup(), BTW...
Yes. That does look like a reasonable simplification.
Eric
Re: [PATCH] devpts: Make each mount of devpts an independent filesystem.
On Tue, Apr 19, 2016 at 10:04:20PM -0500, Eric W. Biederman wrote:
> +#ifdef CONFIG_UNIX98_PTYS
> +int path_pts(struct path *path)
> +{
> + /* Find "pts" in the same directory as the input path */
> + struct dentry *child, *parent;
> + struct qstr this;
> + int ret;
> +
> + ret = path_parent_directory(path);
> + if (ret)
> + return ret;
> +
> + parent = path->dentry;
> + if (!d_can_lookup(parent))
> + return -ENOENT;
And how, pray tell, would a parent of anything fail to be a directory?
> + 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);
What the hell for? What does that lock on parent change for the
dcache lookup you are doing here?
> + child = d_lookup(parent, &this);
> + inode_unlock(parent->d_inode);
> + if (!child)
> + return -ENOENT;
Take a look at d_hash_and_lookup(), BTW...
> + 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/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
> index e0ee0b3000b2..486f6282b0c6 100644
> --- a/include/linux/devpts_fs.h
> +++ b/include/linux/devpts_fs.h
> @@ -17,36 +17,21 @@
>
> #ifdef CONFIG_UNIX98_PTYS
>
> -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);
> -void devpts_del_ref(struct inode *ptmx_inode);
> +struct pts_fs_info;
> +
> +struct pts_fs_info *devpts_acquire(struct file *filp);
> +void devpts_release(struct pts_fs_info *fsi);
> +
> +int devpts_new_index(struct pts_fs_info *fsi);
> +void devpts_kill_index(struct pts_fs_info *fsi, int idx);
> /* mknod in devpts */
> -struct inode *devpts_pty_new(struct inode *ptmx_inode, dev_t device, int
> index,
> +struct inode *devpts_pty_new(struct pts_fs_info *fsi, dev_t device, int
> index,
> void *priv);
> /* get private structure */
> void *devpts_get_priv(struct inode *pts_inode);
> /* unlink */
> void devpts_pty_kill(struct inode *inode);
>
> -#else
> -
> -/* Dummy stubs in the no-pty case */
> -static inline int devpts_new_index(struct inode *ptmx_inode) { return
> -EINVAL; }
> -static inline void devpts_kill_index(struct inode *ptmx_inode, int idx) { }
> -static inline void devpts_add_ref(struct inode *ptmx_inode) { }
> -static inline void devpts_del_ref(struct inode *ptmx_inode) { }
> -static inline struct inode *devpts_pty_new(struct inode *ptmx_inode,
> - dev_t device, int index, void *priv)
> -{
> - return ERR_PTR(-EINVAL);
> -}
> -static inline void *devpts_get_priv(struct inode *pts_inode)
> -{
> - return NULL;
> -}
> -static inline void devpts_pty_kill(struct inode *inode) { }
> -
> #endif
>
>
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 77d01700daf7..f29abda31e6d 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -45,6 +45,8 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT,
> LAST_BIND};
> #define LOOKUP_ROOT 0x2000
> #define LOOKUP_EMPTY 0x4000
>
> +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.8.1
>
[PATCH] devpts: Make each mount of devpts an independent filesystem.
The /dev/ptmx device node is changed to lookup the directory entry "pts" in the same directory as the /dev/ptmx device node was opened in. If there is a "pts" entry and that entry is a devpts filesystem /dev/ptmx uses that filesystem. Otherwise the open of /dev/ptmx fails. The DEVPTS_MULTIPLE_INSTANCES configuration option is removed, so that userspace can now safely depend on each mount of devpts creating a new instance of the filesystem. Each mount of devpts is now a separate and equal filesystem. The kernel.pty.reserve sysctl is neutered with no way currently implemented to be able to use the reserved ptys. A new vfs helper path_pts is introduced that finds a directory entry named "pts" in the directory of the passed in path, and changes the passed in path to point to it. The helper path_pts uses a function path_parent_directory that was factored out of follow_dotdot. In the implementation of devpts: - devpts_mnt is killed as it is no longer meaningful if all mounts of devpts are equal. - pts_sb_from_inode is replaced by just inode->i_sb as all cached inodes in the tty layer are now from the devpts filesystem. - devpts_add_ref is rolled into the new function devpts_ptmx. And the unnecessary inode hold is removed. - devpts_del_ref is renamed devpts_release and reduced to just a deacrivate_super. - The newinstance mount option continues to be accepted but is now ignored. In devpts_fs.h definitions for when !CONFIG_UNIX98_PTYS are removed as they are never used. Documentation/filesystems/devices.txt is updated to describe the current situation. 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. With the caveat that on centos6 and on slackware-14.1 that there wind up being two instances of the devpts filesystem mounted on /dev/pts, the lower copy does not end up getting used. Signed-off-by: "Eric W. Biederman" --- Documentation/filesystems/devpts.txt | 145 +++-- drivers/tty/Kconfig | 11 -- drivers/tty/pty.c| 41 --- fs/devpts/inode.c| 205 +-- fs/namei.c | 58 -- include/linux/devpts_fs.h| 31 ++ include/linux/namei.h| 2 + 7 files changed, 148 insertions(+), 345 deletions(-) diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt index 30d2fcb32f72..6a40d7e59247 100644 --- a/Documentation/filesystems/devpts.txt +++ b/Documentation/filesystems/devpts.txt @@ -1,141 +1,26 @@ +Each mount of the devpts filesystem is now distinct such that ptys +and their indicies allocated in one mount are independent from ptys +and their indicies in all other mounts. -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. +All mounts of the devpts filesystem now create a /dev/pts/ptmx node +with permissions . -To preserve backward compatibility, this support for multiple instances is -enabled only if: +To retain backwards compatibility the a ptmx device node (aka any node +created with "mknod name c 5 2") when opened will look for an instance +of devpts under the name "pts" in the same directory as the ptmx device +node. - - 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 , 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
