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

[PATCH v3 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 v3 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

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

[Andrew Morton]

On Mon, 11 Mar 2013 21:48:06 -0300 Lucas De Marchi <lucas.demarchi@profusion.mobi> 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.

call_usermodehelper_exec() was exported to modules but
call_usermodehelper_setup() was not?

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

[Lucas De Marchi]

On Tue, Mar 12, 2013 at 5:31 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 11 Mar 2013 21:48:06 -0300 Lucas De Marchi <lucas.demarchi@profusion.mobi> 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.
>
> call_usermodehelper_exec() was exported to modules but
> call_usermodehelper_setup() was not?
>

That was a mistake. We need both functions exported. Should I send and
updated patch or a fixup?


Lucas De Marchi

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

[Andrew Morton]

On Tue, 12 Mar 2013 18:52:54 -0300 Lucas De Marchi <lucas.demarchi@profusion.mobi> wrote:

> On Tue, Mar 12, 2013 at 5:31 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Mon, 11 Mar 2013 21:48:06 -0300 Lucas De Marchi <lucas.demarchi@profusion.mobi> 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.
> >
> > call_usermodehelper_exec() was exported to modules but
> > call_usermodehelper_setup() was not?
> >
> 
> That was a mistake. We need both functions exported. Should I send and
> updated patch or a fixup?

From: Andrew Morton <akpm@linux-foundation.org>
Subject: usermodehelper-export-_exec-and-_setup-functions-fix

export call_usermodehelper_setup() to modules

Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Lucas De Marchi <lucas.demarchi@profusion.mobi>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/kmod.c |    1 +
 1 file changed, 1 insertion(+)

diff -puN kernel/kmod.c~usermodehelper-export-_exec-and-_setup-functions-fix kernel/kmod.c
--- a/kernel/kmod.c~usermodehelper-export-_exec-and-_setup-functions-fix
+++ a/kernel/kmod.c
@@ -541,6 +541,7 @@ struct subprocess_info *call_usermodehel
   out:
 	return sub_info;
 }
+EXPORT_SYMBOL(call_usermodehelper_setup);
 
 /**
  * call_usermodehelper_exec - start a usermode application
_

[PATCH v3 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>
---

Changes from v2: remove check for wait == UMH_NO_WAIT since this never happens,
	as pointed out by Oleg

 kernel/kmod.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index b39f240..ae255a3 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -77,6 +77,7 @@ static void free_modprobe_argv(struct subprocess_info *info)
 
 static int call_modprobe(char *module_name, int wait)
 {
+	struct subprocess_info *info;
 	static char *envp[] = {
 		"HOME=/",
 		"TERM=linux",
@@ -98,8 +99,15 @@ 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);
+	info = call_usermodehelper_setup(modprobe_path, argv, envp, GFP_KERNEL,
+					 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 v3 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>
---

Changes from v2: simplify error handling as suggested by Oleg.

 security/keys/request_key.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 4bd6bdb..c411f9b 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,9 +93,16 @@ 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,
+					  session_keyring);
+	if (!info)
+		return -ENOMEM;
+
+	key_get(session_keyring);
+	return call_usermodehelper_exec(info, wait);
 }
 
 /*
-- 
1.8.1.5

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

[James Morris]

On Mon, 11 Mar 2013, Lucas De Marchi wrote:

> 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>

Acked-by: James Morris <james.l.morris@oracle.com>




-- 
James Morris
<jmorris@namei.org>

[PATCH v3 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 v3 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>
---

Changes from v2: simplify error handling as suggested by Oleg.

 fs/coredump.c           | 12 +++++++++---
 init/do_mounts_initrd.c |  8 ++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 7dfb3b0..d52f6bd 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,14 @@ 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);
+		retval = -ENOMEM;
+		sub_info = call_usermodehelper_setup(helper_argv[0],
+						helper_argv, NULL, GFP_KERNEL,
+						umh_pipe_setup, NULL, &cprm);
+		if (sub_info)
+			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..3e0878e 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,11 @@ 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)
+		return;
+	call_usermodehelper_exec(info, UMH_WAIT_PROC);
 
 	current->flags &= ~PF_FREEZER_SKIP;
 
-- 
1.8.1.5

[PATCH v3 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 ae255a3..166aff5 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -554,8 +554,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
@@ -615,29 +615,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 v3 4/7] KEYS: split call to call_usermodehelper_fns()

[David Howells]

Lucas De Marchi <lucas.demarchi@profusion.mobi> wrote:

> 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>

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