[v3,1/3] Make call_usermodehelper_exec possible to set pid namespace

Submitted by Zhao Lei on Aug. 29, 2016, 12:06 p.m.

Details

Message ID 94bf69d92a3204491862b8c72111f4e046bf5f9e.1472471586.git.zhaolei@cn.fujitsu.com
State New
Series "Make core_pattern support namespace"
Headers show

Commit Message

Zhao Lei Aug. 29, 2016, 12:06 p.m.
Current call_usermodehelper_exec() can not set pid namespace for
the executed program, because we need addition fork to make pid
namespace active.

This patch add above function for call_usermodehelper_exec().
When init_intermediate callback return -EAGAIN, the usermodehelper
will fork again to make pid namespace active, and run program
in the child process.

This function is helpful for coredump to run pipe_program in
specific container environment.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 fs/coredump.c               |   3 +-
 include/linux/kmod.h        |   2 +
 init/do_mounts_initrd.c     |   3 +-
 kernel/kmod.c               | 133 ++++++++++++++++++++++++++++++++++++++------
 lib/kobject_uevent.c        |   3 +-
 security/keys/request_key.c |   4 +-
 6 files changed, 127 insertions(+), 21 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/coredump.c b/fs/coredump.c
index 281b768..ceb0ee8 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -641,7 +641,8 @@  void do_coredump(const siginfo_t *siginfo)
 		retval = -ENOMEM;
 		sub_info = call_usermodehelper_setup(helper_argv[0],
 						helper_argv, NULL, GFP_KERNEL,
-						umh_pipe_setup, NULL, &cprm);
+						NULL, umh_pipe_setup,
+						NULL, &cprm);
 		if (sub_info)
 			retval = call_usermodehelper_exec(sub_info,
 							  UMH_WAIT_EXEC);
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index fcfd2bf..8fb8c0e 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -61,6 +61,7 @@  struct subprocess_info {
 	char **envp;
 	int wait;
 	int retval;
+	int (*init_intermediate)(struct subprocess_info *info);
 	int (*init)(struct subprocess_info *info, struct cred *new);
 	void (*cleanup)(struct subprocess_info *info);
 	void *data;
@@ -71,6 +72,7 @@  call_usermodehelper(char *path, char **argv, char **envp, int wait);
 
 extern struct subprocess_info *
 call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
+			  int (*init_intermediate)(struct subprocess_info *info),
 			  int (*init)(struct subprocess_info *info, struct cred *new),
 			  void (*cleanup)(struct subprocess_info *), void *data);
 
diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index a1000ca..bb5dce5 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -72,7 +72,8 @@  static void __init handle_initrd(void)
 	current->flags |= PF_FREEZER_SKIP;
 
 	info = call_usermodehelper_setup("/linuxrc", argv, envp_init,
-					 GFP_KERNEL, init_linuxrc, NULL, NULL);
+					 GFP_KERNEL, NULL, init_linuxrc, NULL,
+					 NULL);
 	if (!info)
 		return;
 	call_usermodehelper_exec(info, UMH_WAIT_PROC);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 0277d12..30a5802 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -91,7 +91,7 @@  static int call_modprobe(char *module_name, int wait)
 	argv[4] = NULL;
 
 	info = call_usermodehelper_setup(modprobe_path, argv, envp, GFP_KERNEL,
-					 NULL, free_modprobe_argv, NULL);
+					 NULL, NULL, free_modprobe_argv, NULL);
 	if (!info)
 		goto free_module_name;
 
@@ -209,14 +209,11 @@  static void umh_complete(struct subprocess_info *sub_info)
 		call_usermodehelper_freeinfo(sub_info);
 }
 
-/*
- * This is the task which runs the usermode application
- */
-static int call_usermodehelper_exec_async(void *data)
+static int __call_usermodehelper_exec_doexec(void *data)
 {
 	struct subprocess_info *sub_info = data;
 	struct cred *new;
-	int retval;
+	int retval = 0;
 
 	spin_lock_irq(&current->sighand->siglock);
 	flush_signal_handlers(current, 1);
@@ -228,10 +225,11 @@  static int call_usermodehelper_exec_async(void *data)
 	 */
 	set_user_nice(current, 0);
 
-	retval = -ENOMEM;
 	new = prepare_kernel_cred(current);
-	if (!new)
+	if (!new) {
+		retval = -ENOMEM;
 		goto out;
+	}
 
 	spin_lock(&umh_sysctl_lock);
 	new->cap_bset = cap_intersect(usermodehelper_bset, new->cap_bset);
@@ -248,20 +246,121 @@  static int call_usermodehelper_exec_async(void *data)
 	}
 
 	commit_creds(new);
-
 	retval = do_execve(getname_kernel(sub_info->path),
-			   (const char __user *const __user *)sub_info->argv,
-			   (const char __user *const __user *)sub_info->envp);
+		   (const char __user *const __user *)sub_info->argv,
+		   (const char __user *const __user *)sub_info->envp);
+
 out:
-	sub_info->retval = retval;
+	return retval;
+}
+
+static int call_usermodehelper_exec_doexec(void *data)
+{
+	struct subprocess_info *sub_info = data;
+	int ret = __call_usermodehelper_exec_doexec(data);
+
 	/*
-	 * call_usermodehelper_exec_sync() will call umh_complete
-	 * if UHM_WAIT_PROC.
+	 * If it is called in non-sync mode:
+	 * On fail:
+	 *   should set sub_info->retval, call umh_complete and exit process.
+	 * On success:
+	 *   should set sub_info->retval, call umh_complete and return to
+	 *   user-mode program.
+	 * It it is called in sync mode:
+	 * On fail:
+	 *   should set sub_info->retval and exit process, the parent process
+	 *   will wait and call umh_complete()
+	 * On success:
+	 *   should return to user-mode program, the parent process will wait
+	 *   and get program's ret from wait(), and call umh_complete().
 	 */
+	sub_info->retval = ret;
+
 	if (!(sub_info->wait & UMH_WAIT_PROC))
 		umh_complete(sub_info);
-	if (!retval)
+
+	if (!ret)
 		return 0;
+
+	/*
+	 * see comment in call_usermodehelper_exec_sync() before change
+	 * it to do_exit(ret).
+	 */
+	do_exit(0);
+}
+
+/*
+ * This is the task which runs the usermode application
+ */
+static int call_usermodehelper_exec_async(void *data)
+{
+	struct subprocess_info *sub_info = data;
+	int ret = 0;
+	int refork = 0;
+
+	if (sub_info->init_intermediate) {
+		ret = sub_info->init_intermediate(sub_info);
+		switch (ret) {
+		case 0:
+			break;
+		case -EAGAIN:
+			/*
+			 * if pid namespace is changed,
+			 * we need refork to make it active.
+			 */
+			ret = 0;
+			refork = 1;
+			break;
+		default:
+			goto out;
+		}
+	}
+
+	if (refork) {
+		/*
+		 * Code copyed from call_usermodehelper_exec_work() and
+		 * call_usermodehelper_exec_sync(), see comments in these
+		 * functions for detail.
+		 */
+		pid_t pid;
+
+		if (sub_info->wait & UMH_WAIT_PROC) {
+			kernel_sigaction(SIGCHLD, SIG_DFL);
+			pid = kernel_thread(call_usermodehelper_exec_doexec,
+					    sub_info, SIGCHLD);
+			if (pid < 0) {
+				ret = pid;
+			} else {
+				ret = -ECHILD;
+				sys_wait4(pid, (int __user *)&ret, 0, NULL);
+			}
+			kernel_sigaction(SIGCHLD, SIG_IGN);
+
+			sub_info->retval = ret;
+		} else {
+			pid = kernel_thread(call_usermodehelper_exec_doexec,
+					    sub_info, SIGCHLD);
+			if (pid < 0) {
+				sub_info->retval = pid;
+				umh_complete(sub_info);
+			}
+		}
+	} else {
+		ret = __call_usermodehelper_exec_doexec(data);
+
+out:
+		/*
+		 * see comment in call_usermodehelper_exec_doexec() for detail
+		 */
+		sub_info->retval = ret;
+
+		if (!(sub_info->wait & UMH_WAIT_PROC))
+			umh_complete(sub_info);
+
+		if (!ret)
+			return 0;
+	}
+
 	do_exit(0);
 }
 
@@ -518,6 +617,7 @@  static void helper_unlock(void)
  */
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 		char **envp, gfp_t gfp_mask,
+		int (*init_intermediate)(struct subprocess_info *info),
 		int (*init)(struct subprocess_info *info, struct cred *new),
 		void (*cleanup)(struct subprocess_info *info),
 		void *data)
@@ -533,6 +633,7 @@  struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	sub_info->envp = envp;
 
 	sub_info->cleanup = cleanup;
+	sub_info->init_intermediate = init_intermediate;
 	sub_info->init = init;
 	sub_info->data = data;
   out:
@@ -619,7 +720,7 @@  int call_usermodehelper(char *path, char **argv, char **envp, int wait)
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
 	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
-					 NULL, NULL, NULL);
+					 NULL, NULL, NULL, NULL);
 	if (info == NULL)
 		return -ENOMEM;
 
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index f6c2c1e..7e571f0 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -345,7 +345,8 @@  int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 		retval = -ENOMEM;
 		info = call_usermodehelper_setup(env->argv[0], env->argv,
 						 env->envp, GFP_KERNEL,
-						 NULL, cleanup_uevent_env, env);
+						 NULL, NULL, cleanup_uevent_env,
+						 env);
 		if (info) {
 			retval = call_usermodehelper_exec(info, UMH_NO_WAIT);
 			env = NULL;	/* freed by cleanup_uevent_env */
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index a29e355..087c11d 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -78,8 +78,8 @@  static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 	struct subprocess_info *info;
 
 	info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
-					  umh_keys_init, umh_keys_cleanup,
-					  session_keyring);
+					 NULL, umh_keys_init, umh_keys_cleanup,
+					 session_keyring);
 	if (!info)
 		return -ENOMEM;
 

Comments

kbuild test robot Aug. 29, 2016, 4:58 p.m.
Hi Zhao,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.8-rc4 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Zhao-Lei/Make-core_pattern-support-namespace/20160829-201413
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   lib/crc32.c:148: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:148: warning: Excess function parameter 'tab' description in 'crc32_le_generic'
   lib/crc32.c:293: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:293: warning: Excess function parameter 'tab' description in 'crc32_be_generic'
   lib/crc32.c:1: warning: no structured comments found
>> kernel/kmod.c:624: warning: No description found for parameter 'init_intermediate'

vim +/init_intermediate +624 kernel/kmod.c

a06a4dc3 Neil Horman         2010-05-26  608   *
a06a4dc3 Neil Horman         2010-05-26  609   * The init function is used to customize the helper process prior to
a06a4dc3 Neil Horman         2010-05-26  610   * exec.  A non-zero return code causes the process to error out, exit,
a06a4dc3 Neil Horman         2010-05-26  611   * and return the failure to the calling process
0ab4dc92 Jeremy Fitzhardinge 2007-07-17  612   *
a06a4dc3 Neil Horman         2010-05-26  613   * The cleanup function is just before ethe subprocess_info is about to
0ab4dc92 Jeremy Fitzhardinge 2007-07-17  614   * be freed.  This can be used for freeing the argv and envp.  The
0ab4dc92 Jeremy Fitzhardinge 2007-07-17  615   * Function must be runnable in either a process context or the
0ab4dc92 Jeremy Fitzhardinge 2007-07-17  616   * context in which call_usermodehelper_exec is called.
0ab4dc92 Jeremy Fitzhardinge 2007-07-17  617   */
938e4b22 Lucas De Marchi     2013-04-30  618  struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
938e4b22 Lucas De Marchi     2013-04-30  619  		char **envp, gfp_t gfp_mask,
a0807e8e Zhao Lei            2016-08-29  620  		int (*init_intermediate)(struct subprocess_info *info),
87966996 David Howells       2011-06-17  621  		int (*init)(struct subprocess_info *info, struct cred *new),
a06a4dc3 Neil Horman         2010-05-26  622  		void (*cleanup)(struct subprocess_info *info),
a06a4dc3 Neil Horman         2010-05-26  623  		void *data)
0ab4dc92 Jeremy Fitzhardinge 2007-07-17 @624  {
938e4b22 Lucas De Marchi     2013-04-30  625  	struct subprocess_info *sub_info;
938e4b22 Lucas De Marchi     2013-04-30  626  	sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
938e4b22 Lucas De Marchi     2013-04-30  627  	if (!sub_info)
938e4b22 Lucas De Marchi     2013-04-30  628  		goto out;
938e4b22 Lucas De Marchi     2013-04-30  629  
b6b50a81 Frederic Weisbecker 2015-09-09  630  	INIT_WORK(&sub_info->work, call_usermodehelper_exec_work);
938e4b22 Lucas De Marchi     2013-04-30  631  	sub_info->path = path;
938e4b22 Lucas De Marchi     2013-04-30  632  	sub_info->argv = argv;

:::::: The code at line 624 was first introduced by commit
:::::: 0ab4dc92278a0f3816e486d6350c6652a72e06c8 usermodehelper: split setup from execution

:::::: TO: Jeremy Fitzhardinge <jeremy@xensource.com>
:::::: CC: Jeremy Fitzhardinge <jeremy@goop.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Andrei Vagin Sept. 6, 2016, 9:11 a.m.
On Mon, Aug 29, 2016 at 08:06:39PM +0800, Zhao Lei wrote:
> Current call_usermodehelper_exec() can not set pid namespace for
> the executed program, because we need addition fork to make pid
> namespace active.
> 
> This patch add above function for call_usermodehelper_exec().
> When init_intermediate callback return -EAGAIN, the usermodehelper
> will fork again to make pid namespace active, and run program
> in the child process.

I am not sure that we need to make this extra fork(). When I commented
the previous patches, I said that we need to make fork() after
setns(CLONE_NEWPID). But we alread have one fork(), why we can't set a
required pidns before kernel_thread(call_usermodehelper_exec_async) and
then switch back into the origin pidns.

> 
> This function is helpful for coredump to run pipe_program in
> specific container environment.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  fs/coredump.c               |   3 +-
>  include/linux/kmod.h        |   2 +
>  init/do_mounts_initrd.c     |   3 +-
>  kernel/kmod.c               | 133 ++++++++++++++++++++++++++++++++++++++------
>  lib/kobject_uevent.c        |   3 +-
>  security/keys/request_key.c |   4 +-
>  6 files changed, 127 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 281b768..ceb0ee8 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -641,7 +641,8 @@ void do_coredump(const siginfo_t *siginfo)
>  		retval = -ENOMEM;
>  		sub_info = call_usermodehelper_setup(helper_argv[0],
>  						helper_argv, NULL, GFP_KERNEL,
> -						umh_pipe_setup, NULL, &cprm);
> +						NULL, umh_pipe_setup,
> +						NULL, &cprm);
>  		if (sub_info)
>  			retval = call_usermodehelper_exec(sub_info,
>  							  UMH_WAIT_EXEC);
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index fcfd2bf..8fb8c0e 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -61,6 +61,7 @@ struct subprocess_info {
>  	char **envp;
>  	int wait;
>  	int retval;
> +	int (*init_intermediate)(struct subprocess_info *info);
>  	int (*init)(struct subprocess_info *info, struct cred *new);
>  	void (*cleanup)(struct subprocess_info *info);
>  	void *data;
> @@ -71,6 +72,7 @@ call_usermodehelper(char *path, char **argv, char **envp, int wait);
>  
>  extern struct subprocess_info *
>  call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
> +			  int (*init_intermediate)(struct subprocess_info *info),
>  			  int (*init)(struct subprocess_info *info, struct cred *new),
>  			  void (*cleanup)(struct subprocess_info *), void *data);
>  
> diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
> index a1000ca..bb5dce5 100644
> --- a/init/do_mounts_initrd.c
> +++ b/init/do_mounts_initrd.c
> @@ -72,7 +72,8 @@ static void __init handle_initrd(void)
>  	current->flags |= PF_FREEZER_SKIP;
>  
>  	info = call_usermodehelper_setup("/linuxrc", argv, envp_init,
> -					 GFP_KERNEL, init_linuxrc, NULL, NULL);
> +					 GFP_KERNEL, NULL, init_linuxrc, NULL,
> +					 NULL);
>  	if (!info)
>  		return;
>  	call_usermodehelper_exec(info, UMH_WAIT_PROC);
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 0277d12..30a5802 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -91,7 +91,7 @@ static int call_modprobe(char *module_name, int wait)
>  	argv[4] = NULL;
>  
>  	info = call_usermodehelper_setup(modprobe_path, argv, envp, GFP_KERNEL,
> -					 NULL, free_modprobe_argv, NULL);
> +					 NULL, NULL, free_modprobe_argv, NULL);
>  	if (!info)
>  		goto free_module_name;
>  
> @@ -209,14 +209,11 @@ static void umh_complete(struct subprocess_info *sub_info)
>  		call_usermodehelper_freeinfo(sub_info);
>  }
>  
> -/*
> - * This is the task which runs the usermode application
> - */
> -static int call_usermodehelper_exec_async(void *data)
> +static int __call_usermodehelper_exec_doexec(void *data)
>  {
>  	struct subprocess_info *sub_info = data;
>  	struct cred *new;
> -	int retval;
> +	int retval = 0;
>  
>  	spin_lock_irq(&current->sighand->siglock);
>  	flush_signal_handlers(current, 1);
> @@ -228,10 +225,11 @@ static int call_usermodehelper_exec_async(void *data)
>  	 */
>  	set_user_nice(current, 0);
>  
> -	retval = -ENOMEM;
>  	new = prepare_kernel_cred(current);
> -	if (!new)
> +	if (!new) {
> +		retval = -ENOMEM;
>  		goto out;
> +	}
>  
>  	spin_lock(&umh_sysctl_lock);
>  	new->cap_bset = cap_intersect(usermodehelper_bset, new->cap_bset);
> @@ -248,20 +246,121 @@ static int call_usermodehelper_exec_async(void *data)
>  	}
>  
>  	commit_creds(new);
> -
>  	retval = do_execve(getname_kernel(sub_info->path),
> -			   (const char __user *const __user *)sub_info->argv,
> -			   (const char __user *const __user *)sub_info->envp);
> +		   (const char __user *const __user *)sub_info->argv,
> +		   (const char __user *const __user *)sub_info->envp);
> +
>  out:
> -	sub_info->retval = retval;
> +	return retval;
> +}
> +
> +static int call_usermodehelper_exec_doexec(void *data)
> +{
> +	struct subprocess_info *sub_info = data;
> +	int ret = __call_usermodehelper_exec_doexec(data);
> +
>  	/*
> -	 * call_usermodehelper_exec_sync() will call umh_complete
> -	 * if UHM_WAIT_PROC.
> +	 * If it is called in non-sync mode:
> +	 * On fail:
> +	 *   should set sub_info->retval, call umh_complete and exit process.
> +	 * On success:
> +	 *   should set sub_info->retval, call umh_complete and return to
> +	 *   user-mode program.
> +	 * It it is called in sync mode:
> +	 * On fail:
> +	 *   should set sub_info->retval and exit process, the parent process
> +	 *   will wait and call umh_complete()
> +	 * On success:
> +	 *   should return to user-mode program, the parent process will wait
> +	 *   and get program's ret from wait(), and call umh_complete().
>  	 */
> +	sub_info->retval = ret;
> +
>  	if (!(sub_info->wait & UMH_WAIT_PROC))
>  		umh_complete(sub_info);
> -	if (!retval)
> +
> +	if (!ret)
>  		return 0;
> +
> +	/*
> +	 * see comment in call_usermodehelper_exec_sync() before change
> +	 * it to do_exit(ret).
> +	 */
> +	do_exit(0);
> +}
> +
> +/*
> + * This is the task which runs the usermode application
> + */
> +static int call_usermodehelper_exec_async(void *data)
> +{
> +	struct subprocess_info *sub_info = data;
> +	int ret = 0;
> +	int refork = 0;
> +
> +	if (sub_info->init_intermediate) {
> +		ret = sub_info->init_intermediate(sub_info);
> +		switch (ret) {
> +		case 0:
> +			break;
> +		case -EAGAIN:
> +			/*
> +			 * if pid namespace is changed,
> +			 * we need refork to make it active.
> +			 */
> +			ret = 0;
> +			refork = 1;
> +			break;
> +		default:
> +			goto out;
> +		}
> +	}
> +
> +	if (refork) {
> +		/*
> +		 * Code copyed from call_usermodehelper_exec_work() and
> +		 * call_usermodehelper_exec_sync(), see comments in these
> +		 * functions for detail.
> +		 */
> +		pid_t pid;
> +
> +		if (sub_info->wait & UMH_WAIT_PROC) {
> +			kernel_sigaction(SIGCHLD, SIG_DFL);
> +			pid = kernel_thread(call_usermodehelper_exec_doexec,
> +					    sub_info, SIGCHLD);
> +			if (pid < 0) {
> +				ret = pid;
> +			} else {
> +				ret = -ECHILD;
> +				sys_wait4(pid, (int __user *)&ret, 0, NULL);
> +			}
> +			kernel_sigaction(SIGCHLD, SIG_IGN);
> +
> +			sub_info->retval = ret;
> +		} else {
> +			pid = kernel_thread(call_usermodehelper_exec_doexec,
> +					    sub_info, SIGCHLD);
> +			if (pid < 0) {
> +				sub_info->retval = pid;
> +				umh_complete(sub_info);
> +			}
> +		}
> +	} else {
> +		ret = __call_usermodehelper_exec_doexec(data);
> +
> +out:
> +		/*
> +		 * see comment in call_usermodehelper_exec_doexec() for detail
> +		 */
> +		sub_info->retval = ret;
> +
> +		if (!(sub_info->wait & UMH_WAIT_PROC))
> +			umh_complete(sub_info);
> +
> +		if (!ret)
> +			return 0;
> +	}
> +
>  	do_exit(0);
>  }
>  
> @@ -518,6 +617,7 @@ static void helper_unlock(void)
>   */
>  struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>  		char **envp, gfp_t gfp_mask,
> +		int (*init_intermediate)(struct subprocess_info *info),
>  		int (*init)(struct subprocess_info *info, struct cred *new),
>  		void (*cleanup)(struct subprocess_info *info),
>  		void *data)
> @@ -533,6 +633,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>  	sub_info->envp = envp;
>  
>  	sub_info->cleanup = cleanup;
> +	sub_info->init_intermediate = init_intermediate;
>  	sub_info->init = init;
>  	sub_info->data = data;
>    out:
> @@ -619,7 +720,7 @@ int call_usermodehelper(char *path, char **argv, char **envp, int wait)
>  	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>  
>  	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> -					 NULL, NULL, NULL);
> +					 NULL, NULL, NULL, NULL);
>  	if (info == NULL)
>  		return -ENOMEM;
>  
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index f6c2c1e..7e571f0 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -345,7 +345,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
>  		retval = -ENOMEM;
>  		info = call_usermodehelper_setup(env->argv[0], env->argv,
>  						 env->envp, GFP_KERNEL,
> -						 NULL, cleanup_uevent_env, env);
> +						 NULL, NULL, cleanup_uevent_env,
> +						 env);
>  		if (info) {
>  			retval = call_usermodehelper_exec(info, UMH_NO_WAIT);
>  			env = NULL;	/* freed by cleanup_uevent_env */
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index a29e355..087c11d 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -78,8 +78,8 @@ static int call_usermodehelper_keys(char *path, char **argv, char **envp,
>  	struct subprocess_info *info;
>  
>  	info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
> -					  umh_keys_init, umh_keys_cleanup,
> -					  session_keyring);
> +					 NULL, umh_keys_init, umh_keys_cleanup,
> +					 session_keyring);
>  	if (!info)
>  		return -ENOMEM;
>  
> -- 
> 1.8.5.1
> 
> 
>
Zhao Lei Sept. 6, 2016, 9:35 a.m.
Hi, Andrei Vagin

Thanks for review.

> -----Original Message-----
> From: Andrei Vagin [mailto:avagin@gmail.com]
> Sent: Tuesday, September 06, 2016 5:12 PM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>
> Cc: linux-kernel@vger.kernel.org; containers@lists.linux-foundation.org; Eric W.
> Biederman <ebiederm@xmission.com>; Mateusz Guzik
> <mguzik@redhat.com>; Kamezawa Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com>; St├ęphane Graber <stgraber@ubuntu.com>
> Subject: Re: [PATCH v3 1/3] Make call_usermodehelper_exec possible to set pid
> namespace
> 
> On Mon, Aug 29, 2016 at 08:06:39PM +0800, Zhao Lei wrote:
> > Current call_usermodehelper_exec() can not set pid namespace for
> > the executed program, because we need addition fork to make pid
> > namespace active.
> >
> > This patch add above function for call_usermodehelper_exec().
> > When init_intermediate callback return -EAGAIN, the usermodehelper
> > will fork again to make pid namespace active, and run program
> > in the child process.
> 
> I am not sure that we need to make this extra fork(). When I commented
> the previous patches, I said that we need to make fork() after
> setns(CLONE_NEWPID). But we alread have one fork(), why we can't set a
> required pidns before kernel_thread(call_usermodehelper_exec_async) and
> then switch back into the origin pidns.
> 
Yes we can avoid extra fork() in above way, but I think it is better not only
setting pid ns before fork, but setting all ns before fork.

If we set pid ns before fork and set other ns after fork, the setting of ns code
will be separated into two parts, and because it is a caller's hook, it will make
the caller code complex.

If we set all ns before fork, both caller's code and the function will be simple,
the kthread will switch all ns before fork, do fork, and switch back, the child
process will inherit all ns from kthread(except for ns_for_child).

Above way seems having no problem, but I have small worry that we touched
ns of a kthread, please tell me if I losted something.

What is your opinion?

Thanks
Zhaolei

> > This function is helpful for coredump to run pipe_program in
> > specific container environment.
> >
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  fs/coredump.c               |   3 +-
> >  include/linux/kmod.h        |   2 +
> >  init/do_mounts_initrd.c     |   3 +-
> >  kernel/kmod.c               | 133
> ++++++++++++++++++++++++++++++++++++++------
> >  lib/kobject_uevent.c        |   3 +-
> >  security/keys/request_key.c |   4 +-
> >  6 files changed, 127 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 281b768..ceb0ee8 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -641,7 +641,8 @@ void do_coredump(const siginfo_t *siginfo)
> >  		retval = -ENOMEM;
> >  		sub_info = call_usermodehelper_setup(helper_argv[0],
> >  						helper_argv, NULL, GFP_KERNEL,
> > -						umh_pipe_setup, NULL, &cprm);
> > +						NULL, umh_pipe_setup,
> > +						NULL, &cprm);
> >  		if (sub_info)
> >  			retval = call_usermodehelper_exec(sub_info,
> >  							  UMH_WAIT_EXEC);
> > diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> > index fcfd2bf..8fb8c0e 100644
> > --- a/include/linux/kmod.h
> > +++ b/include/linux/kmod.h
> > @@ -61,6 +61,7 @@ struct subprocess_info {
> >  	char **envp;
> >  	int wait;
> >  	int retval;
> > +	int (*init_intermediate)(struct subprocess_info *info);
> >  	int (*init)(struct subprocess_info *info, struct cred *new);
> >  	void (*cleanup)(struct subprocess_info *info);
> >  	void *data;
> > @@ -71,6 +72,7 @@ call_usermodehelper(char *path, char **argv, char
> **envp, int wait);
> >
> >  extern struct subprocess_info *
> >  call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t
> gfp_mask,
> > +			  int (*init_intermediate)(struct subprocess_info *info),
> >  			  int (*init)(struct subprocess_info *info, struct cred *new),
> >  			  void (*cleanup)(struct subprocess_info *), void *data);
> >
> > diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
> > index a1000ca..bb5dce5 100644
> > --- a/init/do_mounts_initrd.c
> > +++ b/init/do_mounts_initrd.c
> > @@ -72,7 +72,8 @@ static void __init handle_initrd(void)
> >  	current->flags |= PF_FREEZER_SKIP;
> >
> >  	info = call_usermodehelper_setup("/linuxrc", argv, envp_init,
> > -					 GFP_KERNEL, init_linuxrc, NULL, NULL);
> > +					 GFP_KERNEL, NULL, init_linuxrc, NULL,
> > +					 NULL);
> >  	if (!info)
> >  		return;
> >  	call_usermodehelper_exec(info, UMH_WAIT_PROC);
> > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > index 0277d12..30a5802 100644
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -91,7 +91,7 @@ static int call_modprobe(char *module_name, int wait)
> >  	argv[4] = NULL;
> >
> >  	info = call_usermodehelper_setup(modprobe_path, argv, envp,
> GFP_KERNEL,
> > -					 NULL, free_modprobe_argv, NULL);
> > +					 NULL, NULL, free_modprobe_argv, NULL);
> >  	if (!info)
> >  		goto free_module_name;
> >
> > @@ -209,14 +209,11 @@ static void umh_complete(struct subprocess_info
> *sub_info)
> >  		call_usermodehelper_freeinfo(sub_info);
> >  }
> >
> > -/*
> > - * This is the task which runs the usermode application
> > - */
> > -static int call_usermodehelper_exec_async(void *data)
> > +static int __call_usermodehelper_exec_doexec(void *data)
> >  {
> >  	struct subprocess_info *sub_info = data;
> >  	struct cred *new;
> > -	int retval;
> > +	int retval = 0;
> >
> >  	spin_lock_irq(&current->sighand->siglock);
> >  	flush_signal_handlers(current, 1);
> > @@ -228,10 +225,11 @@ static int call_usermodehelper_exec_async(void
> *data)
> >  	 */
> >  	set_user_nice(current, 0);
> >
> > -	retval = -ENOMEM;
> >  	new = prepare_kernel_cred(current);
> > -	if (!new)
> > +	if (!new) {
> > +		retval = -ENOMEM;
> >  		goto out;
> > +	}
> >
> >  	spin_lock(&umh_sysctl_lock);
> >  	new->cap_bset = cap_intersect(usermodehelper_bset, new->cap_bset);
> > @@ -248,20 +246,121 @@ static int call_usermodehelper_exec_async(void
> *data)
> >  	}
> >
> >  	commit_creds(new);
> > -
> >  	retval = do_execve(getname_kernel(sub_info->path),
> > -			   (const char __user *const __user *)sub_info->argv,
> > -			   (const char __user *const __user *)sub_info->envp);
> > +		   (const char __user *const __user *)sub_info->argv,
> > +		   (const char __user *const __user *)sub_info->envp);
> > +
> >  out:
> > -	sub_info->retval = retval;
> > +	return retval;
> > +}
> > +
> > +static int call_usermodehelper_exec_doexec(void *data)
> > +{
> > +	struct subprocess_info *sub_info = data;
> > +	int ret = __call_usermodehelper_exec_doexec(data);
> > +
> >  	/*
> > -	 * call_usermodehelper_exec_sync() will call umh_complete
> > -	 * if UHM_WAIT_PROC.
> > +	 * If it is called in non-sync mode:
> > +	 * On fail:
> > +	 *   should set sub_info->retval, call umh_complete and exit process.
> > +	 * On success:
> > +	 *   should set sub_info->retval, call umh_complete and return to
> > +	 *   user-mode program.
> > +	 * It it is called in sync mode:
> > +	 * On fail:
> > +	 *   should set sub_info->retval and exit process, the parent process
> > +	 *   will wait and call umh_complete()
> > +	 * On success:
> > +	 *   should return to user-mode program, the parent process will wait
> > +	 *   and get program's ret from wait(), and call umh_complete().
> >  	 */
> > +	sub_info->retval = ret;
> > +
> >  	if (!(sub_info->wait & UMH_WAIT_PROC))
> >  		umh_complete(sub_info);
> > -	if (!retval)
> > +
> > +	if (!ret)
> >  		return 0;
> > +
> > +	/*
> > +	 * see comment in call_usermodehelper_exec_sync() before change
> > +	 * it to do_exit(ret).
> > +	 */
> > +	do_exit(0);
> > +}
> > +
> > +/*
> > + * This is the task which runs the usermode application
> > + */
> > +static int call_usermodehelper_exec_async(void *data)
> > +{
> > +	struct subprocess_info *sub_info = data;
> > +	int ret = 0;
> > +	int refork = 0;
> > +
> > +	if (sub_info->init_intermediate) {
> > +		ret = sub_info->init_intermediate(sub_info);
> > +		switch (ret) {
> > +		case 0:
> > +			break;
> > +		case -EAGAIN:
> > +			/*
> > +			 * if pid namespace is changed,
> > +			 * we need refork to make it active.
> > +			 */
> > +			ret = 0;
> > +			refork = 1;
> > +			break;
> > +		default:
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	if (refork) {
> > +		/*
> > +		 * Code copyed from call_usermodehelper_exec_work() and
> > +		 * call_usermodehelper_exec_sync(), see comments in these
> > +		 * functions for detail.
> > +		 */
> > +		pid_t pid;
> > +
> > +		if (sub_info->wait & UMH_WAIT_PROC) {
> > +			kernel_sigaction(SIGCHLD, SIG_DFL);
> > +			pid = kernel_thread(call_usermodehelper_exec_doexec,
> > +					    sub_info, SIGCHLD);
> > +			if (pid < 0) {
> > +				ret = pid;
> > +			} else {
> > +				ret = -ECHILD;
> > +				sys_wait4(pid, (int __user *)&ret, 0, NULL);
> > +			}
> > +			kernel_sigaction(SIGCHLD, SIG_IGN);
> > +
> > +			sub_info->retval = ret;
> > +		} else {
> > +			pid = kernel_thread(call_usermodehelper_exec_doexec,
> > +					    sub_info, SIGCHLD);
> > +			if (pid < 0) {
> > +				sub_info->retval = pid;
> > +				umh_complete(sub_info);
> > +			}
> > +		}
> > +	} else {
> > +		ret = __call_usermodehelper_exec_doexec(data);
> > +
> > +out:
> > +		/*
> > +		 * see comment in call_usermodehelper_exec_doexec() for detail
> > +		 */
> > +		sub_info->retval = ret;
> > +
> > +		if (!(sub_info->wait & UMH_WAIT_PROC))
> > +			umh_complete(sub_info);
> > +
> > +		if (!ret)
> > +			return 0;
> > +	}
> > +
> >  	do_exit(0);
> >  }
> >
> > @@ -518,6 +617,7 @@ static void helper_unlock(void)
> >   */
> >  struct subprocess_info *call_usermodehelper_setup(char *path, char
> **argv,
> >  		char **envp, gfp_t gfp_mask,
> > +		int (*init_intermediate)(struct subprocess_info *info),
> >  		int (*init)(struct subprocess_info *info, struct cred *new),
> >  		void (*cleanup)(struct subprocess_info *info),
> >  		void *data)
> > @@ -533,6 +633,7 @@ struct subprocess_info
> *call_usermodehelper_setup(char *path, char **argv,
> >  	sub_info->envp = envp;
> >
> >  	sub_info->cleanup = cleanup;
> > +	sub_info->init_intermediate = init_intermediate;
> >  	sub_info->init = init;
> >  	sub_info->data = data;
> >    out:
> > @@ -619,7 +720,7 @@ int call_usermodehelper(char *path, char **argv,
> char **envp, int wait)
> >  	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC :
> GFP_KERNEL;
> >
> >  	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> > -					 NULL, NULL, NULL);
> > +					 NULL, NULL, NULL, NULL);
> >  	if (info == NULL)
> >  		return -ENOMEM;
> >
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index f6c2c1e..7e571f0 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -345,7 +345,8 @@ int kobject_uevent_env(struct kobject *kobj, enum
> kobject_action action,
> >  		retval = -ENOMEM;
> >  		info = call_usermodehelper_setup(env->argv[0], env->argv,
> >  						 env->envp, GFP_KERNEL,
> > -						 NULL, cleanup_uevent_env, env);
> > +						 NULL, NULL, cleanup_uevent_env,
> > +						 env);
> >  		if (info) {
> >  			retval = call_usermodehelper_exec(info, UMH_NO_WAIT);
> >  			env = NULL;	/* freed by cleanup_uevent_env */
> > diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> > index a29e355..087c11d 100644
> > --- a/security/keys/request_key.c
> > +++ b/security/keys/request_key.c
> > @@ -78,8 +78,8 @@ static int call_usermodehelper_keys(char *path, char
> **argv, char **envp,
> >  	struct subprocess_info *info;
> >
> >  	info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
> > -					  umh_keys_init, umh_keys_cleanup,
> > -					  session_keyring);
> > +					 NULL, umh_keys_init, umh_keys_cleanup,
> > +					 session_keyring);
> >  	if (!info)
> >  		return -ENOMEM;
> >
> > --
> > 1.8.5.1
> >
> >
> >
>