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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Lucas De Marchi <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
> ---
> 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
>
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 <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
> ---
> 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
>
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 <[email protected]>
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.
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.
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.
On Sat, Mar 9, 2013 at 5:23 PM, Oleg Nesterov <[email protected]> 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 <[email protected]>
>
> 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
On Sat, Mar 9, 2013 at 5:25 PM, Oleg Nesterov <[email protected]> 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
On Sat, Mar 9, 2013 at 5:42 PM, Oleg Nesterov <[email protected]> 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
Oleg Nesterov <[email protected]> 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 <[email protected]>
On Mon, Mar 25, 2013 at 9:55 AM, David Howells <[email protected]> wrote:
> Oleg Nesterov <[email protected]> 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