Second attempt at contained helper execution
Subsystems (7)
v4 cover Second attempt at contained helper execution
Here is another update to the attempt at contained helper execution.
The main change is I've tried to incorporate Oleg's suggestions of directly constructing the namespaces rather than using the open/setns approach and the addition of a namespace hash store.
I'm not particularly happy with this so far as there are a bunch of ref counted objects and I've almost certainly got that wrong. But also there are object lifetime problems, some I'm aware of and for sure others I'm not. Also there is the integrity of the thread runner process. I haven't performed a double fork on thread execution, it might be painful to implement, so the thread runner might end up with the wrong namespace setup if an error occurs.
Anyway, I've decided to stop spinning my wheels with this and post an update in the hope that others can offer suggestions to help and, of course, point out things I've missed.
The other change has been to the nfs and KEYS patches. I've introduced the ability to get a token that can be used to save namespace information for later execution and I've attempted to use that for persistent namespace execution, as was discussed previously.
I'm not at all sure I've done this in a sensible way but the token does need to be accessible at helper execution time which is why I've done it this way.
I definitely need advice here too.
---
Ian Kent (12):
nsproxy - make create_new_namespaces() non-static
kmod - rename call_usermodehelper() flags parameter
vfs - move mnt_namespace definition to linux/mount.h
kmod - add namespace aware thread runner
kmod - teach call_usermodehelper() to use a namespace
kmod - add namespace info store
kmod - add call_usermodehelper_ns()
nfsd - use namespace if not executing in init namespace
nfs - cache_lib use namespace if not executing in init namespace nfs - objlayout use namespace if not executing in init namespace KEYS - use correct memory allocation flag in call_usermodehelper_keys()
KEYS: exec request-key within the requesting task's init namespace
fs/mount.h | 12 - fs/nfs/cache_lib.c | 7 + fs/nfs/objlayout/objlayout.c | 7 + fs/nfsd/netns.h | 3 fs/nfsd/nfs4recover.c | 48 +++- fs/nfsd/nfsctl.c | 6 + include/linux/key.h | 3 include/linux/kmod.h | 56 +++++ include/linux/mount.h | 14 + include/linux/nsproxy.h | 3 include/linux/sunrpc/cache.h | 2 kernel/kmod.c | 465 ++++++++++++++++++++++++++++++++++++++++-- kernel/nsproxy.c | 2 net/sunrpc/cache.c | 5 security/keys/gc.c | 2 security/keys/key.c | 4 security/keys/request_key.c | 39 +++- 17 files changed, 620 insertions(+), 58 deletions(-)
--
Ian
RFC PATCH v4 1/12 nsproxy - make create_new_namespaces() non-static
From: Ian Kent <[email protected]>
create_new_namespaces() will be needed by usermodehelper namespace restricted execution.
--- include/linux/nsproxy.h | 3 +++ kernel/nsproxy.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-)
include/linux/nsproxy.h +3 -0
@@ -62,6 +62,9 @@ extern struct nsproxy init_nsproxy;* */ +struct nsproxy *create_new_namespaces (unsigned long flags, +struct task_struct *tsk,struct user_namespace *user_ns, +struct fs_struct *new_fs);int copy_namespaces (unsigned long flags,struct task_struct *tsk);void exit_task_namespaces (struct task_struct *tsk);void switch_task_namespaces (struct task_struct *tsk,struct nsproxy *new);
kernel/nsproxy.c +1 -1
@@ -56,7 +56,7 @@ static inline struct nsproxy *create_nsproxy(void)* Return the newly created nsproxy. Do not attach this to the task, * leave it to the caller to do proper locking and attach it to task. */ -static struct nsproxy *create_new_namespaces (unsigned long flags, +struct nsproxy *create_new_namespaces (unsigned long flags,struct task_struct *tsk,struct user_namespace *user_ns,struct fs_struct *new_fs) {
RFC PATCH v4 3/12 vfs - move mnt_namespace definition to linux/mount.h
In reply to Ian KentFrom: Ian Kent <[email protected]>
The mnt_namespace definition will be needed by the usermode helper contained execution implementation, move it to include/linux/mount.h.
--- fs/mount.h | 12 ------------ include/linux/mount.h | 14 +++++++++++++- 2 files changed, 13 insertions(+), 13 deletions(-)
fs/mount.h +0 -12
@@ -1,20 +1,8 @@#include <linux/mount.h> #include <linux/seq_file.h> #include <linux/poll.h> -#include <linux/ns_common.h> #include <linux/fs_pin.h> -struct mnt_namespace { -atomic_t count; -struct ns_common ns; -struct mount * root; -struct list_head list; -struct user_namespace *user_ns; -u64 seq;/* Sequence number to prevent loops */ -wait_queue_head_t poll; -u64 event; -}; -struct mnt_pcp {int mnt_count;int mnt_writers;
include/linux/mount.h +13 -1
@@ -15,11 +15,12 @@#include <linux/spinlock.h> #include <linux/seqlock.h> #include <linux/atomic.h> +#include <linux/ns_common.h> +#include <linux/wait.h> struct super_block ;struct vfsmount ;struct dentry ; -struct mnt_namespace ;#define MNT_NOSUID 0x01#define MNT_NODEV 0x02
@@ -62,6 +63,17 @@ struct mnt_namespace;#define MNT_SYNC_UMOUNT 0x2000000#define MNT_MARKED 0x4000000 +struct mnt_namespace { +atomic_t count; +struct ns_common ns; +struct mount * root; +struct list_head list; +struct user_namespace *user_ns; +u64 seq;/* Sequence number to prevent loops */ +wait_queue_head_t poll; +u64 event; +}; +struct vfsmount {struct dentry *mnt_root;/* root of the mounted tree */ struct super_block *mnt_sb;/* pointer to superblock */
RFC PATCH v4 4/12 kmod - add namespace aware thread runner
In reply to Ian KentFrom: Ian Kent <[email protected]>
Make usermode helper thread runner namespace aware.
--- include/linux/kmod.h | 12 ++++++ kernel/kmod.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 4 deletions(-)
include/linux/kmod.h +12 -0
@@ -25,6 +25,7 @@#include <linux/compiler.h> #include <linux/workqueue.h> #include <linux/sysctl.h> +#include <linux/user_namespace.h> #define KMOD_PATH_LEN 256
@@ -52,6 +53,14 @@ struct file;#define UMH_WAIT_EXEC 1/* wait for the exec, but not the process */ #define UMH_WAIT_PROC 2/* wait for the process to complete */ #define UMH_KILLABLE 4/* wait for EXEC/PROC killable */ +#define UMH_USE_NS 32/* exec using caller's init namespace */ + +#ifdef CONFIG_NAMESPACES +struct umh_ns_info { +struct nsproxy *nsproxy; +struct user_namespace *user_ns; +}; +#endif struct subprocess_info {struct work_struct work;
@@ -64,6 +73,9 @@ struct subprocess_info {int (*init)(struct subprocess_info *info,struct cred *new);void (*cleanup)(struct subprocess_info *info);void *data; +#ifdef CONFIG_NAMESPACES +struct umh_ns_info nsinfo; +#endif }; extern int
kernel/kmod.c +94 -4
118 lines not on this page · raw message
RFC PATCH v4 5/12 kmod - teach call_usermodehelper() to use a namespace
In reply to Ian KentFrom: Ian Kent <[email protected]>
The call_usermodehelper() function executes all binaries in the global "init" root context. This doesn't allow a binary to be run within a namespace (eg. the namespaces of a container).
The init process of the callers environment is used to setup the namespaces in almost the same way the root init process when the UMH_USE_NS flag is used.
--- kernel/kmod.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
kernel/kmod.c +66 -0
@@ -56,6 +56,8 @@ static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;static DEFINE_SPINLOCK (umh_sysctl_lock);static DECLARE_RWSEM (umhelper_sem); +static void umh_put_nsproxy (struct subprocess_info *); +#ifdef CONFIG_MODULES /*
@@ -194,6 +196,7 @@ static void call_usermodehelper_freeinfo(struct subprocess_info *info) {if (info->cleanup) (*info->cleanup)(info); +umh_put_nsproxy (info);kfree (info); }
@@ -565,6 +568,61 @@ static void helper_unlock(void)wake_up (&running_helpers_waitq); } +#ifndef CONFIG_NAMESPACES +static int umh_get_nsproxy (struct subprocess_info *sub_info) +{ +return -ENOTSUP ; +} + +static void umh_put_nsproxy (struct subprocess_info *sub_info) +{ +} +#else +static int umh_get_nsproxy (struct subprocess_info *sub_info) +{ +struct umh_ns_info *nsinfo = &sub_info->nsinfo; +struct task_struct *tsk; +struct user_namespace *user_ns; +struct nsproxy *new; +int err =0 ; + +rcu_read_lock (); + tsk =find_task_by_vpid (1 ); +if (tsk) +get_task_struct (tsk); +rcu_read_unlock (); +if (!tsk) { + err = -ESRCH ; + goto out; + } + + user_ns =get_user_ns (tsk->cred->user_ns); + + new =create_new_namespaces (0 , tsk, user_ns, tsk->fs); +if (IS_ERR (new)) { + err =PTR_ERR (new); +put_user_ns (user_ns); +put_task_struct (tsk); + goto out; + } + +put_task_struct (tsk); + + nsinfo->nsproxy = new; + nsinfo->user_ns = user_ns; +out: +return err; +} + +static void umh_put_nsproxy (struct subprocess_info *sub_info) +{ +if (sub_info->nsinfo.nsproxy) { +put_nsproxy (sub_info->nsinfo.nsproxy); +put_user_ns (sub_info->nsinfo.user_ns); + } +} +#endif +/** * call_usermodehelper_setup - prepare to call a usermode helper * @path: path to usermode executable
@@ -697,6 +755,14 @@ int call_usermodehelper(char *path,if (info ==NULL )return -ENOMEM ; +if (flags &UMH_USE_NS ) { +int err =umh_get_nsproxy (info); +if (err) { +kfree (info); +return err; + } + } +return call_usermodehelper_exec (info, flags); }EXPORT_SYMBOL (call_usermodehelper);
RFC PATCH v4 6/12 kmod - add namespace info store
In reply to Ian KentFrom: Ian Kent <[email protected]>
Persistent use of namespace information is needed where contained execution is needed in a namespace other than the current namespace.
Use a simple random token as a key to store namespace information in a hashed list for later usermode helper execution.
--- include/linux/kmod.h | 14 ++++ kernel/kmod.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 193 insertions(+), 6 deletions(-)
include/linux/kmod.h +14 -0
@@ -78,6 +78,20 @@ struct subprocess_info {#endif }; +#ifndef CONFIG_NAMESPACES +static inline long umh_ns_get_token (long token) +{ +return -ENOTSUP ; +} + +static inline void umh_ns_put_token (long token) +{ +} +#else +extern long umh_ns_get_token (long token); +extern void umh_ns_put_token (long token); +#endif +extern int call_usermodehelper (char *path,char **argv,char **envp,unsigned int flags);
kernel/kmod.c +179 -6
219 lines not on this page · raw message
RFC PATCH v4 7/12 kmod - add call_usermodehelper_ns()
In reply to Ian KentFrom: Ian Kent <[email protected]>
Add function call_usermodehelper_ns() to allow passing a namespace token to lookup previously stored namespace information for usermode helper execution.
--- include/linux/kmod.h | 24 +++++++++++++ kernel/kmod.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+)
include/linux/kmod.h +24 -0
@@ -87,9 +87,33 @@ static inline long umh_ns_get_token(long token)static inline void umh_ns_put_token (long token) { } + +static inline int +call_usermodehelper_ns (char *path,char **argv,char **envp, +unsigned int flags,long token) +{ +return -ENOTSUP ; +} + +static inline struct subprocess_info * +call_usermodehelper_setup_ns (char *path,char **argv,char **envp,gfp_t gfp_mask, +int (*init)(struct subprocess_info *info,struct cred *new), +void (*cleanup)(struct subprocess_info *),void *data, +long token) +{ +return -ENOTSUP ; +}#else extern long umh_ns_get_token (long token);extern void umh_ns_put_token (long token); +extern int +call_usermodehelper_ns (char *path,char **argv,char **envp, +unsigned int flags,long token); +extern struct subprocess_info * +call_usermodehelper_setup_ns (char *path,char **argv,char **envp,gfp_t gfp_mask, +int (*init)(struct subprocess_info *info,struct cred *new), +void (*cleanup)(struct subprocess_info *),void *data, +long token);#endif extern int
kernel/kmod.c +96 -0
108 lines not on this page · raw message
RFC PATCH v4 8/12 nfsd - use namespace if not executing in init namespace
In reply to Ian KentFrom: Ian Kent <[email protected]>
If nfsd is running within a container the client tracking operations should run within the originating container also.
--- fs/nfsd/netns.h | 3 +++ fs/nfsd/nfs4recover.c | 48 +++++++++++++++++++++++++++++++----------------- fs/nfsd/nfsctl.c | 6 ++++++ 3 files changed, 40 insertions(+), 17 deletions(-)
fs/nfsd/netns.h +3 -0
@@ -112,6 +112,9 @@ struct nfsd_net {u32 clientid_counter;struct svc_serv *nfsd_serv; + +/* Namespace token */ +long umh_token; };/* Simple check to find out if a given net was properly initialized */
fs/nfsd/nfs4recover.c +31 -17
110 lines not on this page · raw message
fs/nfsd/nfsctl.c +6 -0
@@ -1224,6 +1224,8 @@ static __net_init int nfsd_init_net(struct net *net) goto out_idmap_error; nn->nfsd4_lease =90 ;/* default lease time */ nn->nfsd4_grace =90 ; +if (net != &init_net) + nn->umh_token =umh_ns_get_token (nn->umh_token);return 0 ; out_idmap_error:
@@ -1234,6 +1236,10 @@ out_export_error: static __net_exit void nfsd_exit_net(struct net *net) { +struct nfsd_net *nn =net_generic (net, nfsd_net_id); + +umh_ns_put_token (nn->umh_token); + nn->umh_token =0 ;nfsd_idmap_shutdown (net);nfsd_export_shutdown (net); }
RFC PATCH v4 10/12 nfs - objlayout use namespace if not executing in init namespace
In reply to Ian KentFrom: Ian Kent <[email protected]>
If the caller is running within a container then execute the usermode helper callback within the init namespace of the container.
--- fs/nfs/objlayout/objlayout.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
fs/nfs/objlayout/objlayout.c +6 -1
@@ -599,9 +599,14 @@ static int __objlayout_upcall(struct __auto_login *login)"PATH=/sbin:/usr/sbin:/bin:/usr/bin" , NULL }; +unsigned int umh_flags =UMH_WAIT_PROC ;char *argv[8 ];int ret; +/* If running within a container use the container namespace */ +if (current->nsproxy->net_ns != &init_net) + umh_flags |=UMH_USE_NS ; +if (unlikely (!osd_login_prog[0 ])) {dprintk ("%s: osd_login_prog is disabled\n" , __func__);return -EACCES ;
@@ -620,7 +625,7 @@ static int __objlayout_upcall(struct __auto_login *login) argv[6 ] = login->systemid_hex; argv[7 ] =NULL ; - ret =call_usermodehelper (argv[0 ], argv, envp,UMH_WAIT_PROC ); + ret =call_usermodehelper (argv[0 ], argv, envp, umh_flags);/* * Disable the upcall mechanism if we're getting an ENOENT or * EACCES error. The admin can re-enable it on the fly by using
RFC PATCH v4 11/12 KEYS - use correct memory allocation flag in call_usermodehelper_keys()
In reply to Ian KentFrom: Ian Kent <[email protected]>
When call_usermodehelper_keys() is called it assumes it won't be called with the flag UMH_NO_WAIT. Currently that's always the case.
Change this to check the flag and use the correct kernel memory allocation flag to guard against future changes.
--- security/keys/request_key.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
security/keys/request_key.c +3 -1
@@ -76,8 +76,10 @@ static int call_usermodehelper_keys(char *path, char **argv, char **envp, struct key *session_keyring, int wait) {struct subprocess_info *info; +unsigned int gfp_mask = (wait &UMH_NO_WAIT ) ? +GFP_ATOMIC :GFP_KERNEL ; - info =call_usermodehelper_setup (path, argv, envp,GFP_KERNEL , + info =call_usermodehelper_setup (path, argv, envp, gfp_mask, umh_keys_init, umh_keys_cleanup, session_keyring);if (!info)
RFC PATCH v4 12/12 KEYS: exec request-key within the requesting task's init namespace
In reply to Ian KentFrom: Ian Kent <[email protected]>
Containerized request key helper callbacks need the ability to execute a binary in a container's context. To do this calling an in kernel equivalent of setns(2) should be sufficient since the user mode helper execution kernel thread ultimately calls do_execve().
--- include/linux/key.h | 3 +++ security/keys/gc.c | 2 ++ security/keys/key.c | 4 ++++ security/keys/request_key.c | 35 +++++++++++++++++++++++++++++++++-- 4 files changed, 42 insertions(+), 2 deletions(-)
include/linux/key.h +3 -0
@@ -209,6 +209,9 @@ struct key { } payload;struct assoc_array keys; }; + +/* Namespace token */ +long umh_token; }; extern struct key *key_alloc(struct key_type *type,
security/keys/gc.c +2 -0
@@ -156,6 +156,8 @@ static noinline void key_gc_unused_keys(struct list_head *keys)kfree (key->description); +umh_ns_put_token (key->umh_token); +#ifdef KEY_DEBUGGING key->magic =KEY_DEBUG_MAGIC_X ;#endif
security/keys/key.c +4 -0
@@ -18,6 +18,7 @@#include <linux/workqueue.h> #include <linux/random.h> #include <linux/err.h> +#include <net/net_namespace.h> #include "internal.h" struct kmem_cache *key_jar;
@@ -309,6 +310,9 @@ struct key *key_alloc(struct key_type *type, const char *desc,/* publish the key by giving it a serial number */ atomic_inc (&user->nkeys);key_alloc_serial (key); +/* If running within a container use the container namespace */ +if (current->nsproxy->net_ns != &init_net) + key->umh_token =umh_ns_get_token (0 ); error:return key;
security/keys/request_key.c +33 -2
@@ -90,6 +90,31 @@ static int call_usermodehelper_keys(char *path, char **argv, char **envp, }/* +* Call a usermode helper with a specific session keyring and execute +* within a namespace. +*/ +static int call_usermodehelper_keys_ns (char *path,char **argv,char **envp, +struct key *session_keyring, +unsigned int wait,long token) +{ +struct subprocess_info *info; +unsigned int gfp_mask = (wait &UMH_NO_WAIT ) ? +GFP_ATOMIC :GFP_KERNEL ; + +if (token <=0 ) +return -EINVAL ; + + info =call_usermodehelper_setup_ns (path, argv, envp, gfp_mask, + umh_keys_init, umh_keys_cleanup, + session_keyring, token); +if (!info) +return -ENOMEM ; + +key_get (session_keyring); +return call_usermodehelper_exec (info, wait|UMH_USE_NS ); +} + +/* * Request userspace finish the construction of a key * - execute "/sbin/request-key <op> <key> <uid> <gid> <keyring> <keyring> <keyring>" */
@@ -104,6 +129,7 @@ static int call_sbin_request_key(struct key_construction *cons,char *argv[9 ], *envp[3 ], uid_str[12 ], gid_str[12 ];char key_str[12 ], keyring_str[3 ][12 ];char desc[20 ]; +unsigned int wait =UMH_WAIT_PROC ;int ret, i;kenter ("{%d},{%d},%s" , key->serial, authkey->serial, op);
@@ -174,8 +200,13 @@ static int call_sbin_request_key(struct key_construction *cons, argv[i] =NULL ;/* do it */ - ret =call_usermodehelper_keys (argv[0 ], argv, envp, keyring, -UMH_WAIT_PROC ); +/* If running within a container use the container namespace */ +if (key->umh_token) + ret =call_usermodehelper_keys_ns (argv[0 ], argv, envp, + keyring, wait, key->umh_token); +else + ret =call_usermodehelper_keys (argv[0 ], + argv, envp, keyring, wait);kdebug ("usermode -> 0x%x" , ret);if (ret >=0 ) {/* ret is the exit/wait code */
RFC PATCH v4 9/12 nfs - cache_lib use namespace if not executing in init namespace
In reply to Ian KentFrom: Ian Kent <[email protected]>
If pipefs is registered within a namespace other than the root init namespace subsequent pipefs requests should be run within the init namespace of registration.
--- fs/nfs/cache_lib.c | 7 ++++++- include/linux/sunrpc/cache.h | 2 ++ net/sunrpc/cache.c | 5 +++++ 3 files changed, 13 insertions(+), 1 deletion(-)
fs/nfs/cache_lib.c +6 -1
@@ -48,7 +48,12 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)if (nfs_cache_getent_prog[0 ] =='\0' ) goto out; - ret =call_usermodehelper (argv[0 ], argv, envp,UMH_WAIT_EXEC ); +if (cd->u.pipefs.umh_token) { +long token = cd->u.pipefs.umh_token; + ret =call_usermodehelper_ns (argv[0 ], argv, envp, +UMH_WAIT_EXEC , token); + }else + ret =call_usermodehelper (argv[0 ], argv, envp,UMH_WAIT_EXEC );/* * Disable the upcall mechanism if we're getting an ENOENT or * EACCES error. The admin can re-enable it on the fly by using
include/linux/sunrpc/cache.h +2 -0
@@ -68,6 +68,8 @@ struct cache_detail_procfs {struct cache_detail_pipefs {struct dentry *dir; +/* Namespace token */ +long umh_token; };struct cache_detail {
net/sunrpc/cache.c +5 -0
@@ -1811,6 +1811,9 @@ int sunrpc_cache_register_pipefs(struct dentry *parent,if (IS_ERR (dir))return PTR_ERR (dir); cd->u.pipefs.dir = dir; +if (cd->net != &init_net) + cd->u.pipefs.umh_token = +umh_ns_get_token (cd->u.pipefs.umh_token);return 0 ; }EXPORT_SYMBOL_GPL (sunrpc_cache_register_pipefs);
@@ -1819,6 +1822,8 @@ void sunrpc_cache_unregister_pipefs(struct cache_detail *cd) {rpc_remove_cache_dir (cd->u.pipefs.dir); cd->u.pipefs.dir =NULL ; +umh_ns_put_token (cd->u.pipefs.umh_token); + cd->u.pipefs.umh_token =0 ; }EXPORT_SYMBOL_GPL (sunrpc_cache_unregister_pipefs);
RFC PATCH v4 2/12 kmod - rename call_usermodehelper() flags parameter
In reply to Ian KentFrom: Ian Kent <[email protected]>
The wait parameter of call_usermodehelper() is not quite a parameter that describes the wait behaviour alone and will later be used to request execution within the current namespaces. This flag is tied to the wait field of the subprocess_info structure which is also a field that doesn't specify wait behaviour alone and is used to hold the passed flags information.
So change both the parameter and structure field name to flags.
--- include/linux/kmod.h | 6 +++--- kernel/kmod.c | 32 +++++++++++++++++--------------- 2 files changed, 20 insertions(+), 18 deletions(-)
include/linux/kmod.h +3 -3
@@ -59,7 +59,7 @@ struct subprocess_info {char *path;char **argv;char **envp; -int wait; +unsigned int flags;int retval;int (*init)(struct subprocess_info *info,struct cred *new);void (*cleanup)(struct subprocess_info *info);
@@ -67,7 +67,7 @@ struct subprocess_info { };extern int -call_usermodehelper (char *path,char **argv,char **envp,int wait); +call_usermodehelper (char *path,char **argv,char **envp,unsigned int flags);extern struct subprocess_info * call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
@@ -75,7 +75,7 @@ call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask, void (*cleanup)(struct subprocess_info *), void *data);extern int -call_usermodehelper_exec (struct subprocess_info *info,int wait); +call_usermodehelper_exec (struct subprocess_info *info,unsigned int flags);extern struct ctl_table usermodehelper_table[];
kernel/kmod.c +17 -15
@@ -259,7 +259,7 @@ static int ____call_usermodehelper(void *data) out: sub_info->retval = retval;/* wait_for_helper() will call umh_complete if UHM_WAIT_PROC. */ -if (!(sub_info->wait &UMH_WAIT_PROC )) +if (!(sub_info->flags &UMH_WAIT_PROC ))umh_complete (sub_info);if (!retval)return 0 ;
@@ -310,7 +310,7 @@ static void __call_usermodehelper(struct work_struct *work) container_of(work, struct subprocess_info, work);pid_t pid; -if (sub_info->wait &UMH_WAIT_PROC ) +if (sub_info->flags &UMH_WAIT_PROC ) pid =kernel_thread (wait_for_helper, sub_info,CLONE_FS |CLONE_FILES |SIGCHLD );else
@@ -525,16 +525,17 @@ EXPORT_SYMBOL(call_usermodehelper_setup);/** * call_usermodehelper_exec - start a usermode application * @sub_info: information about the subprocessa -* @wait: wait for the application to finish and return status. -* when UMH_NO_WAIT don't wait at all, but you get no useful error back -* when the program couldn't be exec'ed. This makes it safe to call -* from interrupt context. +* @flags: flag to indicate whether to wait for the application to finish +* and return status. If UMH_NO_WAIT is set don't wait at all, but +* you get no useful error back when the program couldn't be exec'ed. +* This makes it safe to call from interrupt context. * * Runs a user-space application. The application is started * asynchronously if wait is not set, and runs as a child of keventd. * (ie. it runs with full root capabilities). */ -int call_usermodehelper_exec (struct subprocess_info *sub_info,int wait) +int call_usermodehelper_exec (struct subprocess_info *sub_info, +unsigned int flags) {DECLARE_COMPLETION_ONSTACK (done);int retval =0 ;
@@ -553,14 +554,14 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)* This makes it possible to use umh_complete to free * the data structure in case of UMH_NO_WAIT. */ - sub_info->complete = (wait ==UMH_NO_WAIT ) ?NULL : &done; - sub_info->wait = wait; + sub_info->complete = (flags &UMH_NO_WAIT ) ?NULL : &done; + sub_info->flags = flags;queue_work (khelper_wq, &sub_info->work); -if (wait ==UMH_NO_WAIT )/* task has freed sub_info */ +if (flags &UMH_NO_WAIT )/* task has freed sub_info */ goto unlock; -if (wait &UMH_KILLABLE ) { +if (flags &UMH_KILLABLE ) { retval =wait_for_completion_killable (&done);if (!retval) goto wait_done;
@@ -587,7 +588,7 @@ EXPORT_SYMBOL(call_usermodehelper_exec);* @path: path to usermode executable * @argv: arg vector for process * @envp: environment for process -* @wait: wait for the application to finish and return status. +* @flags: wait for the application to finish and return status. * when UMH_NO_WAIT don't wait at all, but you get no useful error back * when the program couldn't be exec'ed. This makes it safe to call * from interrupt context.
@@ -595,17 +596,18 @@ EXPORT_SYMBOL(call_usermodehelper_exec);* This function is the equivalent to use call_usermodehelper_setup() and * call_usermodehelper_exec(). */ -int call_usermodehelper (char *path,char **argv,char **envp,int wait) +int call_usermodehelper (char *path, +char **argv,char **envp,unsigned int flags) {struct subprocess_info *info; -gfp_t gfp_mask = (wait ==UMH_NO_WAIT ) ?GFP_ATOMIC :GFP_KERNEL ; +gfp_t gfp_mask = (flags &UMH_NO_WAIT ) ?GFP_ATOMIC :GFP_KERNEL ; info =call_usermodehelper_setup (path, argv, envp, gfp_mask,NULL ,NULL ,NULL );if (info ==NULL )return -ENOMEM ; -return call_usermodehelper_exec (info, wait); +return call_usermodehelper_exec (info, flags); }EXPORT_SYMBOL (call_usermodehelper);
Quoted 5 lines · Ian Kent
The other change has been to the nfs and KEYS patches. I've introduced the ability to get a token that can be used to save namespace information for later execution and I've attempted to use that for persistent namespace execution, as was discussed previously.
The nfsd patch at least definitely looks like what we need.
--b.
Al Viro
maintainerRe: [RFC PATCH v4 03/12] vfs - move mnt_namespace definition to linux/mount.h
In reply to Ian KentQuoted 4 lines · Ian Kent
From: Ian Kent <[email protected]>
The
mnt_namespacedefinition will be needed by the usermode helper contained execution implementation, move it toinclude/linux/mount.h.
I really don't like that. AFAICS, the root of the evil is that fscking nsproxy keeps a pointer to mnt_namespace while all it really needs to know about is the address of ns_common field embedded into it. Let's see...
fs/namespace.c:697: struct mnt_namespace *ns = current->nsproxy->mnt_ns; fs/namespace.c:770: return mnt->mnt_ns == current->nsproxy->mnt_ns; fs/namespace.c:1502: return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); fs/namespace.c:1587: return current->nsproxy->mnt_ns->seq >= mnt_ns->seq; fs/namespace.c:2293: struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns; fs/namespace.c:2961: touch_mnt_namespace(current->nsproxy->mnt_ns); fs/namespace.c:3003: init_task.nsproxy->mnt_ns = ns; fs/namespace.c:3101: ns_root.mnt = ¤t->nsproxy->mnt_ns->root->mnt; fs/namespace.c:3119: struct mnt_namespace *ns = current->nsproxy->mnt_ns;
fs/namespace.c:3159: ns = &nsproxy->mnt_ns->ns;fs/namespace.c:3187: put_mnt_ns(nsproxy->mnt_ns);fs/namespace.c:3188: nsproxy->mnt_ns = mnt_ns;fs/pnode.c:282: user_ns = current->nsproxy->mnt_ns->user_ns;
fs/proc_namespace.c:246: if (!nsp || !nsp->mnt_ns) {
fs/proc_namespace.c:251: ns = nsp->mnt_ns;
include/linux/nsproxy.h:33: struct mnt_namespace *mnt_ns;
kernel/nsproxy.c:37: .mnt_ns = NULL,
kernel/nsproxy.c:70: new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);
kernel/nsproxy.c:71: if (IS_ERR(new_nsp->mnt_ns)) {kernel/nsproxy.c:72: err = PTR_ERR(new_nsp->mnt_ns);kernel/nsproxy.c:113: if (new_nsp->mnt_ns)kernel/nsproxy.c:114: put_mnt_ns(new_nsp->mnt_ns);kernel/nsproxy.c:160: if (ns->mnt_ns)kernel/nsproxy.c:161: put_mnt_ns(ns->mnt_ns);
OK, so we need
a) add in fs/mount.h
static inline struct mnt_namespace *current_mnt_ns(void)
{
return current->nsproxy->mnt_ns;
}and replace a lot of open-coded instances.
b) lift to_mnt_ns() into fs/mount.h (as static inline)
c) switch put_mnt_ns() to struct ns_common *, replacing put_mnt_ns(ns) with put_mnt_ns(&ns->ns) and using to_mnt_ns() in the body to recover the original.
d) make copy_mnt_ns() take and return struct ns_common * (same treatment as for put_mnt_ns() in (c); replace
new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);
if (IS_ERR(new_nsp->mnt_ns)) {
err = PTR_ERR(new_nsp->mnt_ns);
goto out_ns;
}
with
ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs);
if (IS_ERR(ns)) {
err = PTR_ERR(ns);
goto out_ns;
}
new_nsp->mnt_ns = to_mnt_ns(ns);e) replace struct mnt_namespace *mnt_ns with struct ns_common *__mnt_ns in nsproxy.h, deal with users of mnt_ns by
* everywhere in the tree replace put_mnt_ns(&...->mnt_ns->ns) with put_mnt_ns(...->__mnt_ns) (i.e. modify the lines modified by (c) again).
* in fs/namespace.c replace
init_task.nsproxy->mnt_ns = ns;
with
init_task.nsproxy->__mnt_ns = &ns->ns;
replace
ns = &nsproxy->mnt_ns->ns;
with
ns = nsproxy->__mnt_ns;
replace
nsproxy->mnt_ns = mnt_ns;
with
nsproxy>__mnt_ns = &mnt_ns->ns;
* in fs/proc_namespace.c replace
ns = nsp->mnt_ns;
with
ns = to_mnt_ns(nsp->__mnt_ns);
and
if (!nsp || !nsp->mnt_ns) {
with
if (!nsp || !nsp->__mnt_ns) {
* in kernel/nsproxy.c: replace
ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs);
added in (d) with
ns = copy_mnt_ns(flags, tsk->nsproxy->__mnt_ns, user_ns, new_fs);
and
new_nsp->mnt_ns = to_mnt_ns(ns);
with
new_nsp->__mnt_ns = ns;The rest of mnt_ns in that file get replaced by __mnt_ns.
* in current_mnt_ns() replace ...->mnt_ns with to_mnt_ns(...->__mnt_ns)
Do you want me to push such a series in vfs.git? After such 5 commits we have no pointers to struct mnt_namespace in nsproxy.h *and* no need for your patches to dig into ->mnt_ns->ns - we already have that as ->__mnt_ns.
Quoted 24 lines · Ian Kent
Here is another update to the attempt at contained helper execution.
The main change is I've tried to incorporate Oleg's suggestions of directly constructing the namespaces rather than using the open/setns approach and the addition of a namespace hash store.
I'm not particularly happy with this so far as there are a bunch of ref counted objects and I've almost certainly got that wrong. But also there are object lifetime problems, some I'm aware of and for sure others I'm not. Also there is the integrity of the thread runner process. I haven't performed a double fork on thread execution, it might be painful to implement, so the thread runner might end up with the wrong namespace setup if an error occurs.
Anyway, I've decided to stop spinning my wheels with this and post an update in the hope that others can offer suggestions to help and, of course, point out things I've missed.
The other change has been to the nfs and KEYS patches. I've introduced the ability to get a token that can be used to save namespace information for later execution and I've attempted to use that for persistent namespace execution, as was discussed previously.
Quoted 5 lines
I'm not at all sure I've done this in a sensible way but the token does need to be accessible at helper execution time which is why I've done it this way.
I definitely need advice here too.
As far as I can tell this patchset continues to be broken for ignoring my earlier advice.
This patchset provides an escape from cgroup, lsm, rlimit, and seccomp policy.
This patchset does not appear particularly nice in how it uses namespaces.
The only safe and sane way to do this is to have a kernel thread with all of the proper attributes configured waiting around ready to start the user mode helper.
The problem you are trying to solve is so hard that we totally failed to solve it outside of the container case. Which is why we have kthreadd. I will be very surprised if you can figure out how to cleanly solve the problem the way you are attacking it.
Eric
Re: [RFC PATCH v4 03/12] vfs - move mnt_namespace definition to linux/mount.h
In reply to Al ViroQuoted 9 lines · Al Viro
Quoted 4 lines · Ian Kent
From: Ian Kent <[email protected]>
The
mnt_namespacedefinition will be needed by the usermode helper contained execution implementation, move it toinclude/linux/mount.h.I really don't like that. AFAICS, the root of the evil is that fscking nsproxy keeps a pointer to
mnt_namespacewhile all it really needs to know about is the address ofns_commonfield embedded into it. Let's see...
Thought that might be the case.
Quoted 89 lines
fs/namespace.c:697: struct mnt_namespace *ns = current->nsproxy->mnt_ns; fs/namespace.c:770: return mnt->mnt_ns == current->nsproxy->mnt_ns; fs/namespace.c:1502: return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); fs/namespace.c:1587: return current->nsproxy->mnt_ns->seq >= mnt_ns->seq; fs/namespace.c:2293: struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns; fs/namespace.c:2961: touch_mnt_namespace(current->nsproxy->mnt_ns); fs/namespace.c:3003: init_task.nsproxy->mnt_ns = ns; fs/namespace.c:3101: ns_root.mnt = ¤t->nsproxy->mnt_ns->root->mnt; fs/namespace.c:3119: struct mnt_namespace *ns = current->nsproxy->mnt_ns;
fs/namespace.c:3159: ns =&nsproxy->mnt_ns->ns;fs/namespace.c:3187:put_mnt_ns(nsproxy->mnt_ns);fs/namespace.c:3188:nsproxy->mnt_ns=mnt_ns;fs/pnode.c:282:user_ns=current->nsproxy->mnt_ns->user_ns;fs/proc_namespace.c:246: if (!nsp || !nsp->mnt_ns) { fs/proc_namespace.c:251: ns = nsp->mnt_ns; include/linux/nsproxy.h:33: struct mnt_namespace *mnt_ns; kernel/nsproxy.c:37: .mnt_ns = NULL, kernel/nsproxy.c:70: new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs); kernel/nsproxy.c:71: if (IS_ERR(new_nsp->mnt_ns)) {
kernel/nsproxy.c:72: err =PTR_ERR(new_nsp->mnt_ns);kernel/nsproxy.c:113: if (new_nsp->mnt_ns)kernel/nsproxy.c:114:put_mnt_ns(new_nsp->mnt_ns);kernel/nsproxy.c:160: if (ns->mnt_ns)kernel/nsproxy.c:161:put_mnt_ns(ns->mnt_ns);OK, so we need
a) add in
fs/mount.h
static inline structmnt_namespace*current_mnt_ns(void){ return current->nsproxy->mnt_ns; }and replace a lot of open-coded instances.
b) lift
to_mnt_ns()intofs/mount.h(as static inline)c) switch
put_mnt_ns()to structns_common*, replacingput_mnt_ns(ns)withput_mnt_ns(&ns->ns)and usingto_mnt_ns()in the body to recover the original.d) make
copy_mnt_ns()take and return structns_common* (same treatment as forput_mnt_ns()in (c); replacenew_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs); if (IS_ERR(new_nsp->mnt_ns)) { err = PTR_ERR(new_nsp->mnt_ns); goto out_ns; } with ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs); if (IS_ERR(ns)) { err = PTR_ERR(ns); goto out_ns; } new_nsp->mnt_ns = to_mnt_ns(ns);e) replace struct
mnt_namespace*mnt_nswith structns_common*__mnt_nsinnsproxy.h, deal with users ofmnt_nsby
* everywhere in the tree replaceput_mnt_ns(&...->mnt_ns->ns)withput_mnt_ns(...->__mnt_ns)(i.e. modify the lines modified by (c) again).
* infs/namespace.creplaceinit_task.nsproxy->mnt_ns = ns; with init_task.nsproxy->__mnt_ns = &ns->ns; replace ns = &nsproxy->mnt_ns->ns; with ns = nsproxy->__mnt_ns; replace nsproxy->mnt_ns = mnt_ns; with nsproxy>__mnt_ns = &mnt_ns->ns; * in fs/proc_namespace.c replace ns = nsp->mnt_ns; with ns = to_mnt_ns(nsp->__mnt_ns); and if (!nsp || !nsp->mnt_ns) { with if (!nsp || !nsp->__mnt_ns) { * in kernel/nsproxy.c: replace ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs); added in (d) with ns = copy_mnt_ns(flags, tsk->nsproxy->__mnt_ns, user_ns, new_fs); and new_nsp->mnt_ns = to_mnt_ns(ns); with new_nsp->__mnt_ns = ns;
Quoted 6 lines
The rest of
mnt_nsin that file get replaced by__mnt_ns.
* incurrent_mnt_ns()replace ...->mnt_ns withto_mnt_ns(...->__mnt_ns)Do you want me to push such a series in vfs.git? After such 5 commits we have no pointers to struct
mnt_namespaceinnsproxy.h*and* no need for your patches to dig into ->mnt_ns->ns - we already have that as ->__mnt_ns.
Yes please, I'd be more confident if you did this than me, there's already enough to worry about with the series.
Ian
Re: [RFC PATCH v4 03/12] vfs - move mnt_namespace definition to linux/mount.h
Ian Kent <[email protected]> writes:
Quoted 104 lines · Al Viro
Quoted 9 lines · Ian Kent
Quoted 4 lines
From: Ian Kent <[email protected]>
The
mnt_namespacedefinition will be needed by the usermode helper contained execution implementation, move it toinclude/linux/mount.h.I really don't like that. AFAICS, the root of the evil is that fscking nsproxy keeps a pointer to
mnt_namespacewhile all it really needs to know about is the address ofns_commonfield embedded into it. Let's see...Thought that might be the case.
Quoted 92 lines
fs/namespace.c:697: struct mnt_namespace *ns = current->nsproxy->mnt_ns; fs/namespace.c:770: return mnt->mnt_ns == current->nsproxy->mnt_ns; fs/namespace.c:1502: return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); fs/namespace.c:1587: return current->nsproxy->mnt_ns->seq >= mnt_ns->seq; fs/namespace.c:2293: struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns; fs/namespace.c:2961: touch_mnt_namespace(current->nsproxy->mnt_ns); fs/namespace.c:3003: init_task.nsproxy->mnt_ns = ns; fs/namespace.c:3101: ns_root.mnt = ¤t->nsproxy->mnt_ns->root->mnt; fs/namespace.c:3119: struct mnt_namespace *ns = current->nsproxy->mnt_ns;
fs/namespace.c:3159: ns =&nsproxy->mnt_ns->ns;fs/namespace.c:3187:put_mnt_ns(nsproxy->mnt_ns);fs/namespace.c:3188:nsproxy->mnt_ns=mnt_ns;fs/pnode.c:282:user_ns=current->nsproxy->mnt_ns->user_ns;fs/proc_namespace.c:246: if (!nsp || !nsp->mnt_ns) { fs/proc_namespace.c:251: ns = nsp->mnt_ns; include/linux/nsproxy.h:33: struct mnt_namespace *mnt_ns; kernel/nsproxy.c:37: .mnt_ns = NULL, kernel/nsproxy.c:70: new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs); kernel/nsproxy.c:71: if (IS_ERR(new_nsp->mnt_ns)) {
kernel/nsproxy.c:72: err =PTR_ERR(new_nsp->mnt_ns);kernel/nsproxy.c:113: if (new_nsp->mnt_ns)kernel/nsproxy.c:114:put_mnt_ns(new_nsp->mnt_ns);kernel/nsproxy.c:160: if (ns->mnt_ns)kernel/nsproxy.c:161:put_mnt_ns(ns->mnt_ns);OK, so we need
a) add in
fs/mount.h
static inline structmnt_namespace*current_mnt_ns(void){ return current->nsproxy->mnt_ns; }and replace a lot of open-coded instances.
b) lift
to_mnt_ns()intofs/mount.h(as static inline)c) switch
put_mnt_ns()to structns_common*, replacingput_mnt_ns(ns)withput_mnt_ns(&ns->ns)and usingto_mnt_ns()in the body to recover the original.d) make
copy_mnt_ns()take and return structns_common* (same treatment as forput_mnt_ns()in (c); replacenew_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs); if (IS_ERR(new_nsp->mnt_ns)) { err = PTR_ERR(new_nsp->mnt_ns); goto out_ns; } with ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs); if (IS_ERR(ns)) { err = PTR_ERR(ns); goto out_ns; } new_nsp->mnt_ns = to_mnt_ns(ns);e) replace struct
mnt_namespace*mnt_nswith structns_common*__mnt_nsinnsproxy.h, deal with users ofmnt_nsby
* everywhere in the tree replaceput_mnt_ns(&...->mnt_ns->ns)withput_mnt_ns(...->__mnt_ns)(i.e. modify the lines modified by (c) again).
* infs/namespace.creplaceinit_task.nsproxy->mnt_ns = ns; with init_task.nsproxy->__mnt_ns = &ns->ns; replace ns = &nsproxy->mnt_ns->ns; with ns = nsproxy->__mnt_ns; replace nsproxy->mnt_ns = mnt_ns; with nsproxy>__mnt_ns = &mnt_ns->ns; * in fs/proc_namespace.c replace ns = nsp->mnt_ns; with ns = to_mnt_ns(nsp->__mnt_ns); and if (!nsp || !nsp->mnt_ns) { with if (!nsp || !nsp->__mnt_ns) { * in kernel/nsproxy.c: replace ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs); added in (d) with ns = copy_mnt_ns(flags, tsk->nsproxy->__mnt_ns, user_ns, new_fs); and new_nsp->mnt_ns = to_mnt_ns(ns); with new_nsp->__mnt_ns = ns;The rest of
mnt_nsin that file get replaced by__mnt_ns.
* incurrent_mnt_ns()replace ...->mnt_ns withto_mnt_ns(...->__mnt_ns)
Quoted 6 lines
Quoted 3 lines
Do you want me to push such a series in vfs.git? After such 5 commits we have no pointers to struct
mnt_namespaceinnsproxy.h*and* no need for your patches to dig into ->mnt_ns->ns - we already have that as ->__mnt_ns.Yes please, I'd be more confident if you did this than me, there's already enough to worry about with the series.
Given that this patchset is a security hole waiting to happen I don't see why Al should bother unless there are good reasons to do this otherwise.
Eric
Quoted 26 lines · Eric W. Biederman
Quoted 24 lines · Ian Kent
Here is another update to the attempt at contained helper execution.
The main change is I've tried to incorporate Oleg's suggestions of directly constructing the namespaces rather than using the open/setns approach and the addition of a namespace hash store.
I'm not particularly happy with this so far as there are a bunch of ref counted objects and I've almost certainly got that wrong. But also there are object lifetime problems, some I'm aware of and for sure others I'm not. Also there is the integrity of the thread runner process. I haven't performed a double fork on thread execution, it might be painful to implement, so the thread runner might end up with the wrong namespace setup if an error occurs.
Anyway, I've decided to stop spinning my wheels with this and post an update in the hope that others can offer suggestions to help and, of course, point out things I've missed.
The other change has been to the nfs and KEYS patches. I've introduced the ability to get a token that can be used to save namespace information for later execution and I've attempted to use that for persistent namespace execution, as was discussed previously.
Quoted 5 lines
Quoted 5 lines
I'm not at all sure I've done this in a sensible way but the token does need to be accessible at helper execution time which is why I've done it this way.
I definitely need advice here too.
Thanks for offering your advice once again Eric.
Quoted 3 lines
As far as I can tell this patchset continues to be broken for ignoring my earlier advice.
Ignoring is the wrong word.
I am ignorant of aspects of the bigger picture here but working on this is helping with that quite a bit and your continued advice is very much needed.
Quoted 3 lines
This patchset provides an escape from cgroup, lsm, rlimit, and seccomp policy.
OK.
Quoted 3 lines
This patchset does not appear particularly nice in how it uses namespaces.
Yeah, I definitely agree with that.
Quoted 4 lines
The only safe and sane way to do this is to have a kernel thread with all of the proper attributes configured waiting around ready to start the user mode helper.
I originally thought that wouldn't be viable due to the potential number of threads that would be needed.
I still think that a thread per mountpoint, as we originally discussed, won't work but looking at nfsd, nfs and the keys code for the recent patch series makes me think that there might not be so many threads needed and that depends on choosing a better place to start the threads.
Quoted 5 lines
The problem you are trying to solve is so hard that we totally failed to solve it outside of the container case. Which is why we have kthreadd. I will be very surprised if you can figure out how to cleanly solve the problem the way you are attacking it.
So the previous approach of using file_open_root()/setns_inode() is equally broken in the same respects as you mention above? You didn't mention on that before?
I get that the problem is hard to solve but I'd rather not give up on it.
Maybe the token I use could relate to a previously created kernel thread instead of namespace information.
LOL, then you can describe what I've done wrong creating the kernel threads as well, ;)
Quoted 4 lines
Eric
Re: [RFC PATCH v4 03/12] vfs - move mnt_namespace definition to linux/mount.h
In reply to Eric W. BiedermanQuoted 111 lines · Eric W. Biederman
Ian Kent <[email protected]> writes:
Quoted 108 lines · Al Viro
Quoted 9 lines · Ian Kent
Quoted 4 lines
From: Ian Kent <[email protected]>
The
mnt_namespacedefinition will be needed by the usermode helper contained execution implementation, move it toinclude/linux/mount.h.I really don't like that. AFAICS, the root of the evil is that fscking nsproxy keeps a pointer to
mnt_namespacewhile all it really needs to know about is the address ofns_commonfield embedded into it. Let's see...Thought that might be the case.
Quoted 95 lines
fs/namespace.c:697: struct mnt_namespace *ns = current->nsproxy->mnt_ns; fs/namespace.c:770: return mnt->mnt_ns == current->nsproxy->mnt_ns; fs/namespace.c:1502: return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN); fs/namespace.c:1587: return current->nsproxy->mnt_ns->seq >= mnt_ns->seq; fs/namespace.c:2293: struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns; fs/namespace.c:2961: touch_mnt_namespace(current->nsproxy->mnt_ns); fs/namespace.c:3003: init_task.nsproxy->mnt_ns = ns; fs/namespace.c:3101: ns_root.mnt = ¤t->nsproxy->mnt_ns->root->mnt; fs/namespace.c:3119: struct mnt_namespace *ns = current->nsproxy->mnt_ns;
fs/namespace.c:3159: ns =&nsproxy->mnt_ns->ns;fs/namespace.c:3187:put_mnt_ns(nsproxy->mnt_ns);fs/namespace.c:3188:nsproxy->mnt_ns=mnt_ns;fs/pnode.c:282:user_ns=current->nsproxy->mnt_ns->user_ns;fs/proc_namespace.c:246: if (!nsp || !nsp->mnt_ns) { fs/proc_namespace.c:251: ns = nsp->mnt_ns; include/linux/nsproxy.h:33: struct mnt_namespace *mnt_ns; kernel/nsproxy.c:37: .mnt_ns = NULL, kernel/nsproxy.c:70: new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs); kernel/nsproxy.c:71: if (IS_ERR(new_nsp->mnt_ns)) {
kernel/nsproxy.c:72: err =PTR_ERR(new_nsp->mnt_ns);kernel/nsproxy.c:113: if (new_nsp->mnt_ns)kernel/nsproxy.c:114:put_mnt_ns(new_nsp->mnt_ns);kernel/nsproxy.c:160: if (ns->mnt_ns)kernel/nsproxy.c:161:put_mnt_ns(ns->mnt_ns);OK, so we need
a) add in
fs/mount.h
static inline structmnt_namespace*current_mnt_ns(void){ return current->nsproxy->mnt_ns; }and replace a lot of open-coded instances.
b) lift
to_mnt_ns()intofs/mount.h(as static inline)c) switch
put_mnt_ns()to structns_common*, replacingput_mnt_ns(ns)withput_mnt_ns(&ns->ns)and usingto_mnt_ns()in the body to recover the original.d) make
copy_mnt_ns()take and return structns_common* (same treatment as forput_mnt_ns()in (c); replacenew_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs); if (IS_ERR(new_nsp->mnt_ns)) { err = PTR_ERR(new_nsp->mnt_ns); goto out_ns; } with ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs); if (IS_ERR(ns)) { err = PTR_ERR(ns); goto out_ns; } new_nsp->mnt_ns = to_mnt_ns(ns);e) replace struct
mnt_namespace*mnt_nswith structns_common*__mnt_nsinnsproxy.h, deal with users ofmnt_nsby
* everywhere in the tree replaceput_mnt_ns(&...->mnt_ns->ns)withput_mnt_ns(...->__mnt_ns)(i.e. modify the lines modified by (c) again).
* infs/namespace.creplaceinit_task.nsproxy->mnt_ns = ns; with init_task.nsproxy->__mnt_ns = &ns->ns; replace ns = &nsproxy->mnt_ns->ns; with ns = nsproxy->__mnt_ns; replace nsproxy->mnt_ns = mnt_ns; with nsproxy>__mnt_ns = &mnt_ns->ns; * in fs/proc_namespace.c replace ns = nsp->mnt_ns; with ns = to_mnt_ns(nsp->__mnt_ns); and if (!nsp || !nsp->mnt_ns) { with if (!nsp || !nsp->__mnt_ns) { * in kernel/nsproxy.c: replace ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs); added in (d) with ns = copy_mnt_ns(flags, tsk->nsproxy->__mnt_ns, user_ns, new_fs); and new_nsp->mnt_ns = to_mnt_ns(ns); with new_nsp->__mnt_ns = ns;The rest of
mnt_nsin that file get replaced by__mnt_ns.
* incurrent_mnt_ns()replace ...->mnt_ns withto_mnt_ns(...->__mnt_ns)Do you want me to push such a series in vfs.git? After such 5 commits we have no pointers to struct
mnt_namespaceinnsproxy.h*and* no need for your patches to dig into ->mnt_ns->ns - we already have that as ->__mnt_ns.
Quoted 6 lines
Quoted 2 lines
Yes please, I'd be more confident if you did this than me, there's already enough to worry about with the series.
Given that this patchset is a security hole waiting to happen I don't see why Al should bother unless there are good reasons to do this otherwise.
Having now got to your mail now, I agree.
Quoted 2 lines
Eric
Al Viro
maintainerRe: [RFC PATCH v4 03/12] vfs - move mnt_namespace definition to linux/mount.h
In reply to Eric W. BiedermanQuoted 6 lines · Eric W. Biederman
Quoted 2 lines
Yes please, I'd be more confident if you did this than me, there's already enough to worry about with the series.
Given that this patchset is a security hole waiting to happen I don't see why Al should bother unless there are good reasons to do this otherwise.
There might be, actually. &...->mnt_ns->ns is a lot saner candidate for a reference in nsproxy than ...->mnt_ns - *that* is the part nsproxy-related code cares about anyway, and unlike the rest of struct mnt_namespace it doesn't have to be opaque for everything outside of (small part of) core VFS. Additionally, ->mnt_ns is a bad name choice - it sounds like a field of struct mount and, worse yet, there *is* a field of struct mount with that name. Confusing for no good reason and makes both harder to grep for. And current_mnt_ns() is definitely open-coded too many times - the first commit in that series makes sense regardless of anything else.