[PATCH v2 0/7] kmod/usermodehelper changes

[PATCH v2 0/7] kmod/usermodehelper changes

[Lucas De Marchi]

Changes from v1:
  - Remove call_usermodehelper_fns() converting all calling sites to either
    call_usermodelper() or call_usermodehelper_setup() + call_usermodehelper_exec()
  - Don't check the return code in call_usermodehelper_freeinfo() - now that
    allocating the subprocess_info is separating from executing it, it's safe to
    allways call the cleanup.

Lucas De Marchi (7):
  kernel/sys.c: Use the simpler call_usermodehelper()
  usermodehelper: Export _exec() and _setup() functions
  kmod: split call to call_usermodehelper_fns()
  KEYS: split call to call_usermodehelper_fns()
  coredump: remove trailling whitespaces
  Split remaining calls to call_usermodehelper_fns()
  kmod: remove call_usermodehelper_fns()

 fs/coredump.c               |  21 +++++++---
 include/linux/kmod.h        |  17 ++++----
 init/do_mounts_initrd.c     |  11 ++++-
 kernel/kmod.c               | 100 +++++++++++++++++++++++---------------------
 kernel/sys.c                |   3 +-
 security/keys/request_key.c |  14 +++++--
 6 files changed, 96 insertions(+), 70 deletions(-)

-- 
1.8.1.5

[PATCH v2 1/7] kernel/sys.c: Use the simpler call_usermodehelper()

[Lucas De Marchi]

Commit "7ff6764 usermodehelper: cleanup/fix __orderly_poweroff() &&
argv_free()" simplified __orderly_poweroff() removing the need to use
call_usermodehelper_fns().

Since we are not passing any callback, it's simpler to use
call_usermodehelper().

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 kernel/sys.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 81f5644..bd15276 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2203,8 +2203,7 @@ static int __orderly_poweroff(void)
 		return -ENOMEM;
 	}
 
-	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_EXEC,
-				      NULL, NULL, NULL);
+	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
 	argv_free(argv);
 
 	return ret;
-- 
1.8.1.5

[PATCH v2 2/7] usermodehelper: Export _exec() and _setup() functions

[Lucas De Marchi]

call_usermodehelper_setup() + call_usermodehelper_exec() need to be
called instead of call_usermodehelper_fns() when the cleanup function
needs to be called even when an ENOMEM error occurs. In this case using
call_usermodehelper_fns() the user can't distinguish if the cleanup
function was called or not.

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 include/linux/kmod.h |  8 ++++++++
 kernel/kmod.c        | 56 +++++++++++++++++++++-------------------------------
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 5398d58..7eebcf5 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -71,6 +71,14 @@ call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
 			int (*init)(struct subprocess_info *info, struct cred *new),
 			void (*cleanup)(struct subprocess_info *), void *data);
 
+extern struct subprocess_info *
+call_usermodehelper_setup(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);
+
+extern int
+call_usermodehelper_exec(struct subprocess_info *info, int wait);
+
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, int wait)
 {
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 56dd349..b39f240 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -502,14 +502,28 @@ static void helper_unlock(void)
  * @argv: arg vector for process
  * @envp: environment for process
  * @gfp_mask: gfp mask for memory allocation
+ * @cleanup: a cleanup function
+ * @init: an init function
+ * @data: arbitrary context sensitive data
  *
  * Returns either %NULL on allocation failure, or a subprocess_info
  * structure.  This should be passed to call_usermodehelper_exec to
  * exec the process and free the structure.
+ *
+ * The init function is used to customize the helper process prior to
+ * exec.  A non-zero return code causes the process to error out, exit,
+ * and return the failure to the calling process
+ *
+ * The cleanup function is just before ethe subprocess_info is about to
+ * be freed.  This can be used for freeing the argv and envp.  The
+ * Function must be runnable in either a process context or the
+ * context in which call_usermodehelper_exec is called.
  */
-static
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask)
+		char **envp, gfp_t gfp_mask,
+		int (*init)(struct subprocess_info *info, struct cred *new),
+		void (*cleanup)(struct subprocess_info *info),
+		void *data)
 {
 	struct subprocess_info *sub_info;
 	sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
@@ -520,38 +534,15 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	sub_info->path = path;
 	sub_info->argv = argv;
 	sub_info->envp = envp;
+
+	sub_info->cleanup = cleanup;
+	sub_info->init = init;
+	sub_info->data = data;
   out:
 	return sub_info;
 }
 
 /**
- * call_usermodehelper_setfns - set a cleanup/init function
- * @info: a subprocess_info returned by call_usermodehelper_setup
- * @cleanup: a cleanup function
- * @init: an init function
- * @data: arbitrary context sensitive data
- *
- * The init function is used to customize the helper process prior to
- * exec.  A non-zero return code causes the process to error out, exit,
- * and return the failure to the calling process
- *
- * The cleanup function is just before ethe subprocess_info is about to
- * be freed.  This can be used for freeing the argv and envp.  The
- * Function must be runnable in either a process context or the
- * context in which call_usermodehelper_exec is called.
- */
-static
-void call_usermodehelper_setfns(struct subprocess_info *info,
-		    int (*init)(struct subprocess_info *info, struct cred *new),
-		    void (*cleanup)(struct subprocess_info *info),
-		    void *data)
-{
-	info->cleanup = cleanup;
-	info->init = init;
-	info->data = data;
-}
-
-/**
  * call_usermodehelper_exec - start a usermode application
  * @sub_info: information about the subprocessa
  * @wait: wait for the application to finish and return status.
@@ -563,7 +554,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
  * asynchronously if wait is not set, and runs as a child of keventd.
  * (ie. it runs with full root capabilities).
  */
-static
 int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 {
 	DECLARE_COMPLETION_ONSTACK(done);
@@ -615,6 +605,7 @@ unlock:
 	helper_unlock();
 	return retval;
 }
+EXPORT_SYMBOL(call_usermodehelper_exec);
 
 /*
  * call_usermodehelper_fns() will not run the caller-provided cleanup function
@@ -630,13 +621,12 @@ int call_usermodehelper_fns(
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
+					 init, cleanup, data);
 
 	if (info == NULL)
 		return -ENOMEM;
 
-	call_usermodehelper_setfns(info, init, cleanup, data);
-
 	return call_usermodehelper_exec(info, wait);
 }
 EXPORT_SYMBOL(call_usermodehelper_fns);
-- 
1.8.1.5

[PATCH v2 3/7] kmod: split call to call_usermodehelper_fns()

[Lucas De Marchi]

Use call_usermodehelper_setup() + call_usermodehelper_exec() instead of
calling call_usermodehelper_fns(). In case the latter returns -ENOMEM
the cleanup function may had not been called - in this case we would
not free argv and module_name.

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 kernel/kmod.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index b39f240..2fd6222 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -77,6 +77,8 @@ static void free_modprobe_argv(struct subprocess_info *info)
 
 static int call_modprobe(char *module_name, int wait)
 {
+	struct subprocess_info *info;
+	gfp_t gfp_mask;
 	static char *envp[] = {
 		"HOME=/",
 		"TERM=linux",
@@ -98,8 +100,17 @@ static int call_modprobe(char *module_name, int wait)
 	argv[3] = module_name;	/* check free_modprobe_argv() */
 	argv[4] = NULL;
 
-	return call_usermodehelper_fns(modprobe_path, argv, envp,
-		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+	gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
+	info = call_usermodehelper_setup(modprobe_path, argv, envp,
+					 gfp_mask, NULL, free_modprobe_argv,
+					 NULL);
+	if (!info)
+		goto free_module_name;
+
+	return call_usermodehelper_exec(info, wait | UMH_KILLABLE);
+
+free_module_name:
+	kfree(module_name);
 free_argv:
 	kfree(argv);
 out:
-- 
1.8.1.5

[PATCH v2 4/7] KEYS: split call to call_usermodehelper_fns()

[Lucas De Marchi]

Use call_usermodehelper_setup() + call_usermodehelper_exec() instead of
calling call_usermodehelper_fns(). In case there's an OOM in this last
function the cleanup function may not be called - in this case we would
miss a call to key_put().

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 security/keys/request_key.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 4bd6bdb..c958f34 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,9 +93,17 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 					struct key *session_keyring, int wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
-				       umh_keys_init, umh_keys_cleanup,
-				       key_get(session_keyring));
+	struct subprocess_info *info;
+
+	info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
+					  umh_keys_init, umh_keys_cleanup,
+					  key_get(session_keyring));
+	if (!info) {
+		key_put(session_keyring);
+		return -ENOMEM;
+	}
+
+	return call_usermodehelper_exec(info, wait);
 }
 
 /*
-- 
1.8.1.5

[PATCH v2 5/7] coredump: remove trailling whitespaces

[Lucas De Marchi]

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 fs/coredump.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index c647965..7dfb3b0 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -522,7 +522,7 @@ void do_coredump(siginfo_t *siginfo)
 
 	ispipe = format_corename(&cn, &cprm);
 
- 	if (ispipe) {
+	if (ispipe) {
 		int dump_count;
 		char **helper_argv;
 
@@ -576,10 +576,10 @@ void do_coredump(siginfo_t *siginfo)
 					NULL, &cprm);
 		argv_free(helper_argv);
 		if (retval) {
- 			printk(KERN_INFO "Core dump to %s pipe failed\n",
+			printk(KERN_INFO "Core dump to %s pipe failed\n",
 			       cn.corename);
 			goto close_fail;
- 		}
+		}
 	} else {
 		struct inode *inode;
 
-- 
1.8.1.5

[PATCH v2 6/7] Split remaining calls to call_usermodehelper_fns()

[Lucas De Marchi]

These are the only users of call_usermodehelper_fns(). This function
suffers from not being able to determine if the cleanup is called. Even
if in this places the cleanup pointer is NULL, convert them to use the
separate call_usermodehelper_setup() + call_usermodehelper_exec()
functions so we can remove the _fns variant.

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 fs/coredump.c           | 15 ++++++++++++---
 init/do_mounts_initrd.c | 11 +++++++++--
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 7dfb3b0..468b4f6 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -525,6 +525,7 @@ void do_coredump(siginfo_t *siginfo)
 	if (ispipe) {
 		int dump_count;
 		char **helper_argv;
+		struct subprocess_info *sub_info;
 
 		if (ispipe < 0) {
 			printk(KERN_WARNING "format_corename failed\n");
@@ -571,9 +572,17 @@ void do_coredump(siginfo_t *siginfo)
 			goto fail_dropcount;
 		}
 
-		retval = call_usermodehelper_fns(helper_argv[0], helper_argv,
-					NULL, UMH_WAIT_EXEC, umh_pipe_setup,
-					NULL, &cprm);
+		sub_info = call_usermodehelper_setup(helper_argv[0],
+						helper_argv, NULL, GFP_KERNEL,
+						umh_pipe_setup, NULL, &cprm);
+		if (!sub_info) {
+			printk(KERN_WARNING "%s failed to allocate memory\n",
+			       __func__);
+			argv_free(helper_argv);
+			goto fail_dropcount;
+		}
+
+		retval = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
 		argv_free(helper_argv);
 		if (retval) {
 			printk(KERN_INFO "Core dump to %s pipe failed\n",
diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index a32ec1c..747cc66 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -50,6 +50,7 @@ static int init_linuxrc(struct subprocess_info *info, struct cred *new)
 
 static void __init handle_initrd(void)
 {
+	struct subprocess_info *info;
 	static char *argv[] = { "linuxrc", NULL, };
 	extern char *envp_init[];
 	int error;
@@ -70,8 +71,14 @@ static void __init handle_initrd(void)
 	 */
 	current->flags |= PF_FREEZER_SKIP;
 
-	call_usermodehelper_fns("/linuxrc", argv, envp_init, UMH_WAIT_PROC,
-			init_linuxrc, NULL, NULL);
+	info = call_usermodehelper_setup("/linuxrc", argv, envp_init,
+					 GFP_KERNEL, init_linuxrc, NULL, NULL);
+	if (!info) {
+		printk(KERN_WARNING "%s failed to allocate memory\n",
+		       __func__);
+		return;
+	}
+	call_usermodehelper_exec(info, UMH_WAIT_PROC);
 
 	current->flags &= ~PF_FREEZER_SKIP;
 
-- 
1.8.1.5

[PATCH v2 7/7] kmod: remove call_usermodehelper_fns()

[Lucas De Marchi]

This function suffers from not being able to determine if the cleanup is
called in case it returns -ENOMEM. Nobody is using it anymore, so let's
remove it.

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 include/linux/kmod.h | 11 +----------
 kernel/kmod.c        | 31 +++++++++++++++++--------------
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 7eebcf5..0555cc6 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -67,9 +67,7 @@ struct subprocess_info {
 };
 
 extern int
-call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
-			int (*init)(struct subprocess_info *info, struct cred *new),
-			void (*cleanup)(struct subprocess_info *), void *data);
+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,
@@ -79,13 +77,6 @@ call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
 extern int
 call_usermodehelper_exec(struct subprocess_info *info, int wait);
 
-static inline int
-call_usermodehelper(char *path, char **argv, char **envp, int wait)
-{
-	return call_usermodehelper_fns(path, argv, envp, wait,
-				       NULL, NULL, NULL);
-}
-
 extern struct ctl_table usermodehelper_table[];
 
 enum umh_disable_depth {
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 2fd6222..eebb63c 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -557,8 +557,8 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
  * call_usermodehelper_exec - start a usermode application
  * @sub_info: information about the subprocessa
  * @wait: wait for the application to finish and return status.
- *        when -1 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
+ *        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.
  *
  * Runs a user-space application.  The application is started
@@ -618,29 +618,32 @@ unlock:
 }
 EXPORT_SYMBOL(call_usermodehelper_exec);
 
-/*
- * call_usermodehelper_fns() will not run the caller-provided cleanup function
- * if a memory allocation failure is experienced.  So the caller might need to
- * check the call_usermodehelper_fns() return value: if it is -ENOMEM, perform
- * the necessaary cleanup within the caller.
+/**
+ * call_usermodehelper() - prepare and start a usermode application
+ * @path: path to usermode executable
+ * @argv: arg vector for process
+ * @envp: environment for process
+ * @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.
+ *
+ * This function is the equivalent to use call_usermodehelper_setup() and
+ * call_usermodehelper_exec().
  */
-int call_usermodehelper_fns(
-	char *path, char **argv, char **envp, int wait,
-	int (*init)(struct subprocess_info *info, struct cred *new),
-	void (*cleanup)(struct subprocess_info *), void *data)
+int call_usermodehelper(char *path, char **argv, char **envp, int wait)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
 	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
-					 init, cleanup, data);
-
+					 NULL, NULL, NULL);
 	if (info == NULL)
 		return -ENOMEM;
 
 	return call_usermodehelper_exec(info, wait);
 }
-EXPORT_SYMBOL(call_usermodehelper_fns);
+EXPORT_SYMBOL(call_usermodehelper);
 
 static int proc_cap_handler(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
-- 
1.8.1.5

Re: [PATCH v2 1/7] kernel/sys.c: Use the simpler call_usermodehelper()

[Oleg Nesterov]

On 03/08, Lucas De Marchi wrote:
>
> Commit "7ff6764 usermodehelper: cleanup/fix __orderly_poweroff() &&
> argv_free()" simplified __orderly_poweroff() removing the need to use
> call_usermodehelper_fns().
>
> Since we are not passing any callback, it's simpler to use
> call_usermodehelper().
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  kernel/sys.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 81f5644..bd15276 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2203,8 +2203,7 @@ static int __orderly_poweroff(void)
>  		return -ENOMEM;
>  	}
>  
> -	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_EXEC,
> -				      NULL, NULL, NULL);
> +	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
>  	argv_free(argv);
>  
>  	return ret;
> -- 
> 1.8.1.5
> 

Re: [PATCH v2 2/7] usermodehelper: Export _exec() and _setup() functions

[Oleg Nesterov]

On 03/08, Lucas De Marchi wrote:
>
> call_usermodehelper_setup() + call_usermodehelper_exec() need to be
> called instead of call_usermodehelper_fns() when the cleanup function
> needs to be called even when an ENOMEM error occurs. In this case using
> call_usermodehelper_fns() the user can't distinguish if the cleanup
> function was called or not.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  include/linux/kmod.h |  8 ++++++++
>  kernel/kmod.c        | 56 +++++++++++++++++++++-------------------------------
>  2 files changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 5398d58..7eebcf5 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -71,6 +71,14 @@ call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
>  			int (*init)(struct subprocess_info *info, struct cred *new),
>  			void (*cleanup)(struct subprocess_info *), void *data);
>  
> +extern struct subprocess_info *
> +call_usermodehelper_setup(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);
> +
> +extern int
> +call_usermodehelper_exec(struct subprocess_info *info, int wait);
> +
>  static inline int
>  call_usermodehelper(char *path, char **argv, char **envp, int wait)
>  {
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 56dd349..b39f240 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -502,14 +502,28 @@ static void helper_unlock(void)
>   * @argv: arg vector for process
>   * @envp: environment for process
>   * @gfp_mask: gfp mask for memory allocation
> + * @cleanup: a cleanup function
> + * @init: an init function
> + * @data: arbitrary context sensitive data
>   *
>   * Returns either %NULL on allocation failure, or a subprocess_info
>   * structure.  This should be passed to call_usermodehelper_exec to
>   * exec the process and free the structure.
> + *
> + * The init function is used to customize the helper process prior to
> + * exec.  A non-zero return code causes the process to error out, exit,
> + * and return the failure to the calling process
> + *
> + * The cleanup function is just before ethe subprocess_info is about to
> + * be freed.  This can be used for freeing the argv and envp.  The
> + * Function must be runnable in either a process context or the
> + * context in which call_usermodehelper_exec is called.
>   */
> -static
>  struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> -						  char **envp, gfp_t gfp_mask)
> +		char **envp, gfp_t gfp_mask,
> +		int (*init)(struct subprocess_info *info, struct cred *new),
> +		void (*cleanup)(struct subprocess_info *info),
> +		void *data)
>  {
>  	struct subprocess_info *sub_info;
>  	sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
> @@ -520,38 +534,15 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>  	sub_info->path = path;
>  	sub_info->argv = argv;
>  	sub_info->envp = envp;
> +
> +	sub_info->cleanup = cleanup;
> +	sub_info->init = init;
> +	sub_info->data = data;
>    out:
>  	return sub_info;
>  }
>  
>  /**
> - * call_usermodehelper_setfns - set a cleanup/init function
> - * @info: a subprocess_info returned by call_usermodehelper_setup
> - * @cleanup: a cleanup function
> - * @init: an init function
> - * @data: arbitrary context sensitive data
> - *
> - * The init function is used to customize the helper process prior to
> - * exec.  A non-zero return code causes the process to error out, exit,
> - * and return the failure to the calling process
> - *
> - * The cleanup function is just before ethe subprocess_info is about to
> - * be freed.  This can be used for freeing the argv and envp.  The
> - * Function must be runnable in either a process context or the
> - * context in which call_usermodehelper_exec is called.
> - */
> -static
> -void call_usermodehelper_setfns(struct subprocess_info *info,
> -		    int (*init)(struct subprocess_info *info, struct cred *new),
> -		    void (*cleanup)(struct subprocess_info *info),
> -		    void *data)
> -{
> -	info->cleanup = cleanup;
> -	info->init = init;
> -	info->data = data;
> -}
> -
> -/**
>   * call_usermodehelper_exec - start a usermode application
>   * @sub_info: information about the subprocessa
>   * @wait: wait for the application to finish and return status.
> @@ -563,7 +554,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
>   * asynchronously if wait is not set, and runs as a child of keventd.
>   * (ie. it runs with full root capabilities).
>   */
> -static
>  int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  {
>  	DECLARE_COMPLETION_ONSTACK(done);
> @@ -615,6 +605,7 @@ unlock:
>  	helper_unlock();
>  	return retval;
>  }
> +EXPORT_SYMBOL(call_usermodehelper_exec);
>  
>  /*
>   * call_usermodehelper_fns() will not run the caller-provided cleanup function
> @@ -630,13 +621,12 @@ int call_usermodehelper_fns(
>  	struct subprocess_info *info;
>  	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>  
> -	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
> +	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> +					 init, cleanup, data);
>  
>  	if (info == NULL)
>  		return -ENOMEM;
>  
> -	call_usermodehelper_setfns(info, init, cleanup, data);
> -
>  	return call_usermodehelper_exec(info, wait);
>  }
>  EXPORT_SYMBOL(call_usermodehelper_fns);
> -- 
> 1.8.1.5
> 

Re: [PATCH v2 3/7] kmod: split call to call_usermodehelper_fns()

[Oleg Nesterov]

On 03/08, Lucas De Marchi wrote:
>
> Use call_usermodehelper_setup() + call_usermodehelper_exec() instead of
> calling call_usermodehelper_fns(). In case the latter returns -ENOMEM
> the cleanup function may had not been called - in this case we would
> not free argv and module_name.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>

Thanks!

looks correct, but...

> @@ -98,8 +100,17 @@ static int call_modprobe(char *module_name, int wait)
>  	argv[3] = module_name;	/* check free_modprobe_argv() */
>  	argv[4] = NULL;
>
> -	return call_usermodehelper_fns(modprobe_path, argv, envp,
> -		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> +	gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;

Why? it is never called with UMH_NO_WAIT,

> +	info = call_usermodehelper_setup(modprobe_path, argv, envp,
> +					 gfp_mask, NULL, free_modprobe_argv,

can't we simply use GFP_KERNEL?

Oleg.

Re: [PATCH v2 4/7] KEYS: split call to call_usermodehelper_fns()

[Oleg Nesterov]

On 03/08, Lucas De Marchi wrote:
>
>  static int call_usermodehelper_keys(char *path, char **argv, char **envp,
>  					struct key *session_keyring, int wait)
>  {
> -	return call_usermodehelper_fns(path, argv, envp, wait,
> -				       umh_keys_init, umh_keys_cleanup,
> -				       key_get(session_keyring));
> +	struct subprocess_info *info;
> +
> +	info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
> +					  umh_keys_init, umh_keys_cleanup,
> +					  key_get(session_keyring));
> +	if (!info) {
> +		key_put(session_keyring);
> +		return -ENOMEM;
> +	}
> +
> +	return call_usermodehelper_exec(info, wait);

Looks correct, but can't we simpluify it a bit?

	info = call_usermodehelper_setup(session_keyring);
	if (!info)
		return ENOMEM;

	key_get(session_keyring));
	return call_usermodehelper_exec(info);

Oleg.

Re: [PATCH v2 6/7] Split remaining calls to call_usermodehelper_fns()

[Oleg Nesterov]

On 03/08, Lucas De Marchi wrote:
>
> @@ -571,9 +572,17 @@ void do_coredump(siginfo_t *siginfo)
>  			goto fail_dropcount;
>  		}
>
> -		retval = call_usermodehelper_fns(helper_argv[0], helper_argv,
> -					NULL, UMH_WAIT_EXEC, umh_pipe_setup,
> -					NULL, &cprm);
> +		sub_info = call_usermodehelper_setup(helper_argv[0],
> +						helper_argv, NULL, GFP_KERNEL,
> +						umh_pipe_setup, NULL, &cprm);
> +		if (!sub_info) {
> +			printk(KERN_WARNING "%s failed to allocate memory\n",
> +			       __func__);

Why?

> +			argv_free(helper_argv);
> +			goto fail_dropcount;
> +		}
> +
> +		retval = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);

I do not really like another argv_free() here... How about

		retval = -ENOMEM;
		info = call_usermodehelper_setup(...);
		if (info)
			retval = call_usermodehelper_fns(...);
		argv_free();

?

Oleg.

Re: [PATCH v2 3/7] kmod: split call to call_usermodehelper_fns()

[Lucas De Marchi]

On Sat, Mar 9, 2013 at 5:23 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/08, Lucas De Marchi wrote:
>>
>> Use call_usermodehelper_setup() + call_usermodehelper_exec() instead of
>> calling call_usermodehelper_fns(). In case the latter returns -ENOMEM
>> the cleanup function may had not been called - in this case we would
>> not free argv and module_name.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
>
> Thanks!
>
> looks correct, but...
>
>> @@ -98,8 +100,17 @@ static int call_modprobe(char *module_name, int wait)
>>       argv[3] = module_name;  /* check free_modprobe_argv() */
>>       argv[4] = NULL;
>>
>> -     return call_usermodehelper_fns(modprobe_path, argv, envp,
>> -             wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> +     gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>
> Why? it is never called with UMH_NO_WAIT,
>
>> +     info = call_usermodehelper_setup(modprobe_path, argv, envp,
>> +                                      gfp_mask, NULL, free_modprobe_argv,
>
> can't we simply use GFP_KERNEL?

True... I was preserving the previous behavior and didn't check the
callers of this function.
I'm going to send a v3.

Thanks
Lucas De Marchi

Re: [PATCH v2 4/7] KEYS: split call to call_usermodehelper_fns()

[Lucas De Marchi]

On Sat, Mar 9, 2013 at 5:25 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/08, Lucas De Marchi wrote:
>>
>>  static int call_usermodehelper_keys(char *path, char **argv, char **envp,
>>                                       struct key *session_keyring, int wait)
>>  {
>> -     return call_usermodehelper_fns(path, argv, envp, wait,
>> -                                    umh_keys_init, umh_keys_cleanup,
>> -                                    key_get(session_keyring));
>> +     struct subprocess_info *info;
>> +
>> +     info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
>> +                                       umh_keys_init, umh_keys_cleanup,
>> +                                       key_get(session_keyring));
>> +     if (!info) {
>> +             key_put(session_keyring);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     return call_usermodehelper_exec(info, wait);
>
> Looks correct, but can't we simpluify it a bit?
>
>         info = call_usermodehelper_setup(session_keyring);
>         if (!info)
>                 return ENOMEM;
>
>         key_get(session_keyring));
>         return call_usermodehelper_exec(info);

Yep, looks better this way.

Lucas De Marchi

Re: [PATCH v2 6/7] Split remaining calls to call_usermodehelper_fns()

[Lucas De Marchi]

On Sat, Mar 9, 2013 at 5:42 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/08, Lucas De Marchi wrote:
>>
>> @@ -571,9 +572,17 @@ void do_coredump(siginfo_t *siginfo)
>>                       goto fail_dropcount;
>>               }
>>
>> -             retval = call_usermodehelper_fns(helper_argv[0], helper_argv,
>> -                                     NULL, UMH_WAIT_EXEC, umh_pipe_setup,
>> -                                     NULL, &cprm);
>> +             sub_info = call_usermodehelper_setup(helper_argv[0],
>> +                                             helper_argv, NULL, GFP_KERNEL,
>> +                                             umh_pipe_setup, NULL, &cprm);
>> +             if (!sub_info) {
>> +                     printk(KERN_WARNING "%s failed to allocate memory\n",
>> +                            __func__);
>
> Why?
>
>> +                     argv_free(helper_argv);
>> +                     goto fail_dropcount;
>> +             }
>> +
>> +             retval = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
>
> I do not really like another argv_free() here... How about
>
>                 retval = -ENOMEM;
>                 info = call_usermodehelper_setup(...);
>                 if (info)
>                         retval = call_usermodehelper_fns(...);
>                 argv_free();
>
> ?

Looks good. I'll prepare a v3

Thanks
Lucas De Marchi

Re: [PATCH v2 4/7] KEYS: split call to call_usermodehelper_fns()

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> Looks correct, but can't we simpluify it a bit?
> 
> 	info = call_usermodehelper_setup(session_keyring);
> 	if (!info)
> 		return ENOMEM;

-ENOMEM;-)

> 	key_get(session_keyring));
> 	return call_usermodehelper_exec(info);

Looks reasonable.  I like it better this way as it makes working out how to
deal with all the error cases so much simpler:-).

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH v2 4/7] KEYS: split call to call_usermodehelper_fns()

[Lucas De Marchi]

On Mon, Mar 25, 2013 at 9:55 AM, David Howells <dhowells@redhat.com> wrote:
> Oleg Nesterov <oleg@redhat.com> wrote:
>
>> Looks correct, but can't we simpluify it a bit?
>>
>>       info = call_usermodehelper_setup(session_keyring);
>>       if (!info)
>>               return ENOMEM;
>
> -ENOMEM;-)

Yep, it's in -mm with -ENOMEM:
http://git.cmpxchg.org/?p=linux-mmotm.git;a=commitdiff;h=a8c67c6e380a8ff15997e8673e97d9fcdf7b8f9c

Lucas De Marchi