[PATCH 0/2] exec: Rediff wrecked bits of usermodehelper overhaul
Neil Horman
patch set that got wrecked when the rcustring bits got pulled. I've got he
origional summary below, and the patches follwoing. Note, their just my
changes, rediffed to work without Andis patch set. The other umh call sites can
still use plain old call_usermodehelper as they did previously. I'll submit
patches as needed for other callers to make use of the init/cleanup functions as
I have the time/ability to test those changes properly. The changes below I've
been able to test and verify myself.
Hey all-
So, about 6 months ago, I made a set of changes to how the
core-dump-to-a-pipe feature in the kernel works. We had reports of several
races, including some reports of apps bypassing our recursion check so that a
process that was forked as part of a core_pattern setup could infinitely crash
and refork until the system crashed.
We fixes those by improving our recursion checks. The new check
basically refuses to fork a process if its core limit is zero, which works well.
Unfortunately, I've been getting grief from maintainer of user space
programs that are inserted as the forked process of core_pattern. They contend
that in order for their programs (such as abrt and apport) to work, all the
running processes in a system must have their core limits set to a non-zero
value, to which I say 'yes'. I did this by design, and think thats the right
way to do things.
But I've been asked to ease this burden on user space enough times that
I thought I would take a look at it. The first suggestion was to make the
recursion check fail on a non-zero 'special' number, like one. That way the
core collector process could set its core size ulimit to 1, and enable the
kernel's recursion detection. This isn't a bad idea on the surface, but I don't
like it since its opt-in, in that if a program like abrt or apport has a bug and
fails to set such a core limit, we're left with a recursively crashing system
again.
So I've come up with this. What I've done is modify the
call_usermodehelper api such that an extra parameter is added, a function
pointer which will be called by the user helper task, after it forks, but before
it exec's the required process. This will give the caller the opportunity to
get a call back in the processes context, allowing it to do whatever it needs to
to the process in the kernel prior to exec-ing the user space code. In the case
of do_coredump, this callback is ues to set the core ulimit of the helper
process to 1. This elimnates the opt-in problem that I had above, as it allows
the ulimit for core sizes to be set to the value of 1, which is what the
recursion check looks for in do_coredump.
This patch has been tested successfully by me.
Signed-off-by: Neil Horman <nho...@tuxdriver.com>
CC: Oleg Nesterov <ol...@redhat.com>
CC: Andi Kleen <an...@firstfloor.org?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Neil Horman
encompasses the patches that were formally in -mm as:
kmod-add-init-function-to-usermodehelper.patch
kmod-add-init-function-to-usermodehelper-fix.patch
I also rolled in changes from Oleg, moving the call of the init method down in
____call_usermodehelper, so the init function could override the nice level on
the process.
Create new function call_usermodehelper_fns() and allow it
to assign both an init and cleanup function, as we'll as arbitrary data.
The init function is called from the context of the forked process and
allows for customization of the helper process prior to calling exec. Its
return code gates the continuation of the process, or causes its exit.
Also add an arbitrary data pointer to the subprocess_info struct allowing
for data to be passed from the caller to the new process, and the
subsequent cleanup process
Also, use this patch to cleanup the cleanup function. It currently takes
an argp and envp pointer for freeing, which is ugly. Lets instead just
make the subprocess_info structure public, and pass that to the cleanup
and init routines
Signed-off-by: Neil Horman <nho...@tuxdriver.com>
CC: Oleg Nesterov <ol...@redhat.com>
CC: Andi Kleen <an...@firstfloor.org>
include/linux/kmod.h | 51 +++++++++++++++++++++++++++++++++++++++++----------
kernel/kmod.c | 51 +++++++++++++++++++++++++++++----------------------
kernel/sys.c | 6 +++---
3 files changed, 73 insertions(+), 35 deletions(-)
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index facb27f..f9edf63 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -23,6 +23,7 @@
#include <linux/stddef.h>
#include <linux/errno.h>
#include <linux/compiler.h>
+#include <linux/workqueue.h>
#define KMOD_PATH_LEN 256
@@ -45,7 +46,27 @@ static inline int request_module_nowait(const char *name, ...) { return -ENOSYS;
struct key;
struct file;
-struct subprocess_info;
+
+enum umh_wait {
+ UMH_NO_WAIT = -1, /* don't wait at all */
+ UMH_WAIT_EXEC = 0, /* wait for the exec, but not the process */
+ UMH_WAIT_PROC = 1, /* wait for the process to complete */
+};
+
+struct subprocess_info {
+ struct work_struct work;
+ struct completion *complete;
+ struct cred *cred;
+ char *path;
+ char **argv;
+ char **envp;
+ enum umh_wait wait;
+ int retval;
+ struct file *stdin;
+ int (*init)(struct subprocess_info *info);
+ void (*cleanup)(struct subprocess_info *info);
+ void *data;
+};
/* Allocate a subprocess_info structure */
struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
@@ -56,14 +77,10 @@ void call_usermodehelper_setkeys(struct subprocess_info *info,
struct key *session_keyring);
int call_usermodehelper_stdinpipe(struct subprocess_info *sub_info,
struct file **filp);
-void call_usermodehelper_setcleanup(struct subprocess_info *info,
- void (*cleanup)(char **argv, char **envp));
-
-enum umh_wait {
- UMH_NO_WAIT = -1, /* don't wait at all */
- UMH_WAIT_EXEC = 0, /* wait for the exec, but not the process */
- UMH_WAIT_PROC = 1, /* wait for the process to complete */
-};
+void call_usermodehelper_setfns(struct subprocess_info *info,
+ int (*init)(struct subprocess_info *info),
+ void (*cleanup)(struct subprocess_info *info),
+ void *data);
/* Actually execute the sub-process */
int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
@@ -73,18 +90,32 @@ int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
void call_usermodehelper_freeinfo(struct subprocess_info *info);
static inline int
-call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
+call_usermodehelper_fns(char *path, char **argv, char **envp,
+ enum umh_wait wait,
+ int (*init)(struct subprocess_info *info),
+ void (*cleanup)(struct subprocess_info *), void *data)
{
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);
+
if (info == NULL)
return -ENOMEM;
+
+ call_usermodehelper_setfns(info, init, cleanup, data);
+
return call_usermodehelper_exec(info, wait);
}
static inline int
+call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
+{
+ return call_usermodehelper_fns(path, argv, envp, wait,
+ NULL, NULL, NULL);
+}
+
+static inline int
call_usermodehelper_keys(char *path, char **argv, char **envp,
struct key *session_keyring, enum umh_wait wait)
{
diff --git a/kernel/kmod.c b/kernel/kmod.c
index bf0e231..531ef62 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -116,27 +116,16 @@ int __request_module(bool wait, const char *fmt, ...)
trace_module_request(module_name, wait, _RET_IP_);
- ret = call_usermodehelper(modprobe_path, argv, envp,
- wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
+ ret = call_usermodehelper_fns(modprobe_path, argv, envp,
+ wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC,
+ NULL, NULL, NULL);
+
atomic_dec(&kmod_concurrent);
return ret;
}
EXPORT_SYMBOL(__request_module);
#endif /* CONFIG_MODULES */
-struct subprocess_info {
- struct work_struct work;
- struct completion *complete;
- struct cred *cred;
- char *path;
- char **argv;
- char **envp;
- enum umh_wait wait;
- int retval;
- struct file *stdin;
- void (*cleanup)(char **argv, char **envp);
-};
-
/*
* This is the task which runs the usermode application
*/
@@ -184,9 +173,16 @@ static int ____call_usermodehelper(void *data)
*/
set_user_nice(current, 0);
+ if (sub_info->init) {
+ retval = sub_info->init(sub_info);
+ if (retval)
+ goto fail;
+ }
+
retval = kernel_execve(sub_info->path, sub_info->argv, sub_info->envp);
/* Exec failed? */
+fail:
sub_info->retval = retval;
do_exit(0);
}
@@ -194,7 +190,7 @@ static int ____call_usermodehelper(void *data)
void call_usermodehelper_freeinfo(struct subprocess_info *info)
{
if (info->cleanup)
- (*info->cleanup)(info->argv, info->envp);
+ (*info->cleanup)(info);
if (info->cred)
put_cred(info->cred);
kfree(info);
@@ -406,21 +402,31 @@ void call_usermodehelper_setkeys(struct subprocess_info *info,
EXPORT_SYMBOL(call_usermodehelper_setkeys);
/**
- * call_usermodehelper_setcleanup - set a cleanup function
+ * 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 befor ethe subprocess_info is about to
+ * 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.
*/
-void call_usermodehelper_setcleanup(struct subprocess_info *info,
- void (*cleanup)(char **argv, char **envp))
+void call_usermodehelper_setfns(struct subprocess_info *info,
+ int (*init)(struct subprocess_info *info),
+ void (*cleanup)(struct subprocess_info *info),
+ void *data)
{
info->cleanup = cleanup;
+ info->init = init;
+ info->data = data;
}
-EXPORT_SYMBOL(call_usermodehelper_setcleanup);
+EXPORT_SYMBOL(call_usermodehelper_setfns);
/**
* call_usermodehelper_stdinpipe - set up a pipe to be used for stdin
@@ -515,7 +521,8 @@ int call_usermodehelper_pipe(char *path, char **argv, char **envp,
struct subprocess_info *sub_info;
int ret;
- sub_info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL);
+ sub_info = call_usermodehelper_setup(path, argv, envp,
+ GFP_KERNEL);
if (sub_info == NULL)
return -ENOMEM;
diff --git a/kernel/sys.c b/kernel/sys.c
index ee25411..66b9edc 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1765,9 +1765,9 @@ SYSCALL_DEFINE3(getcpu, unsigned __user *, cpup, unsigned __user *, nodep,
char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff";
-static void argv_cleanup(char **argv, char **envp)
+static void argv_cleanup(struct subprocess_info *info)
{
- argv_free(argv);
+ argv_free(info->argv);
}
/**
@@ -1801,7 +1801,7 @@ int orderly_poweroff(bool force)
goto out;
}
- call_usermodehelper_setcleanup(info, argv_cleanup);
+ call_usermodehelper_setfns(info, NULL, argv_cleanup, NULL);
ret = call_usermodehelper_exec(info, UMH_NO_WAIT);
Neil Horman
encompasses the patches that were formally in -mm as:
kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limitcleanup.patch
kmod: replace call_usermodehelper_pipe() with use of umh init function and resolve limit
The first patch in this series introduced an init function to the
call_usermodehelper api so that processes could be customized by caller.
This patch takes advantage of that fact, by customizing the helper in
do_coredump to create the pipe and set its core limit to one (for our
recusrsion check). This lets us clean up the previous uglyness in the
usermodehelper internals and factor call_usermodehelper out entirely.
While I'm at it, we can also modify the helper setup to look for a core
limit value of 1 rather than zero for our recursion check
Signed-off-by: Neil Horman <nho...@tuxdriver.com>
CC: Oleg Nesterov <ol...@redhat.com>
CC: Andi Kleen <an...@firstfloor.org>
fs/exec.c | 63 ++++++++++++++++++++++++++++++++++-----
include/linux/kmod.h | 7 ----
kernel/kmod.c | 82 ---------------------------------------------------
3 files changed, 56 insertions(+), 96 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index f351cdb..64a50b4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1784,6 +1784,50 @@ static void wait_for_dump_helpers(struct file *file)
}
+/*
+ * uhm_pipe_setup
+ * helper function to customize the process used
+ * to collect the core in userspace. Specifically
+ * it sets up a pipe and installs it as fd 0 (stdin)
+ * for the process. Returns 0 on success, or
+ * PTR_ERR on failure.
+ * Note that it also sets the core limit to 1. This
+ * is a special value that we use to trap recursive
+ * core dumps
+ */
+static int umh_pipe_setup(struct subprocess_info *info)
+{
+ struct file *rp, *wp;
+ struct fdtable *fdt;
+ struct coredump_params *cp = (struct coredump_params *)info->data;
+ struct files_struct *cf = current->files;
+
+ wp = create_write_pipe(0);
+ if (IS_ERR(wp))
+ return PTR_ERR(wp);
+
+ rp = create_read_pipe(wp, 0);
+ if (IS_ERR(rp)) {
+ free_write_pipe(wp);
+ return PTR_ERR(rp);
+ }
+
+ cp->file = wp;
+
+ sys_close(0);
+ fd_install(0, rp);
+ spin_lock(&cf->file_lock);
+ fdt = files_fdtable(cf);
+ FD_SET(0, fdt->open_fds);
+ FD_CLR(0, fdt->close_on_exec);
+ spin_unlock(&cf->file_lock);
+
+ /* and disallow core files too */
+ current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
+
+ return 0;
+}
+
void do_coredump(long signr, int exit_code, struct pt_regs *regs)
{
struct core_state core_state;
@@ -1871,15 +1915,15 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
goto fail_unlock;
if (ispipe) {
- if (cprm.limit == 0) {
+ if (cprm.limit == 1) {
/*
* Normally core limits are irrelevant to pipes, since
* we're not writing to the file system, but we use
- * cprm.limit of 0 here as a speacial value. Any
- * non-zero limit gets set to RLIM_INFINITY below, but
+ * cprm.limit of 1 here as a speacial value. Any
+ * non-1 limit gets set to RLIM_INFINITY below, but
* a limit of 0 skips the dump. This is a consistent
* way to catch recursive crashes. We can still crash
- * if the core_pattern binary sets RLIM_CORE = !0
+ * if the core_pattern binary sets RLIM_CORE = !1
* but it runs as root, and can do lots of stupid things
* Note that we use task_tgid_vnr here to grab the pid
* of the process group leader. That way we get the
@@ -1887,7 +1931,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
* core_pattern process dies.
*/
printk(KERN_WARNING
- "Process %d(%s) has RLIMIT_CORE set to 0\n",
+ "Process %d(%s) has RLIMIT_CORE set to 1\n",
task_tgid_vnr(current), current->comm);
printk(KERN_WARNING "Aborting core\n");
goto fail_unlock;
@@ -1911,8 +1955,13 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
cprm.limit = RLIM_INFINITY;
/* SIGPIPE can happen, but it's just never processed */
- if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
- &cprm.file)) {
+ cprm.file = NULL;
+ if (call_usermodehelper_fns(helper_argv[0], helper_argv, NULL,
+ UMH_WAIT_EXEC, umh_pipe_setup,
+ NULL, &cprm)) {
+ if (cprm.file)
+ filp_close(cprm.file, NULL);
+
printk(KERN_INFO "Core dump to %s pipe failed\n",
corename);
goto fail_dropcount;
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index f9edf63..5c05877 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -62,7 +62,6 @@ struct subprocess_info {
char **envp;
enum umh_wait wait;
int retval;
- struct file *stdin;
int (*init)(struct subprocess_info *info);
void (*cleanup)(struct subprocess_info *info);
void *data;
@@ -75,8 +74,6 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
/* Set various pieces of state into the subprocess_info structure */
void call_usermodehelper_setkeys(struct subprocess_info *info,
struct key *session_keyring);
-int call_usermodehelper_stdinpipe(struct subprocess_info *sub_info,
- struct file **filp);
void call_usermodehelper_setfns(struct subprocess_info *info,
int (*init)(struct subprocess_info *info),
void (*cleanup)(struct subprocess_info *info),
@@ -132,10 +129,6 @@ call_usermodehelper_keys(char *path, char **argv, char **envp,
extern void usermodehelper_init(void);
-struct file;
-extern int call_usermodehelper_pipe(char *path, char *argv[], char *envp[],
- struct file **filp);
-
extern int usermodehelper_disable(void);
extern void usermodehelper_enable(void);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 531ef62..d154454 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -147,23 +147,6 @@ static int ____call_usermodehelper(void *data)
commit_creds(sub_info->cred);
sub_info->cred = NULL;
- /* Install input pipe when needed */
- if (sub_info->stdin) {
- struct files_struct *f = current->files;
- struct fdtable *fdt;
- /* no races because files should be private here */
- sys_close(0);
- fd_install(0, sub_info->stdin);
- spin_lock(&f->file_lock);
- fdt = files_fdtable(f);
- FD_SET(0, fdt->open_fds);
- FD_CLR(0, fdt->close_on_exec);
- spin_unlock(&f->file_lock);
-
- /* and disallow core files too */
- current->signal->rlim[RLIMIT_CORE] = (struct rlimit){0, 0};
- }
-
/* We can run anywhere, unlike our parent keventd(). */
set_cpus_allowed_ptr(current, cpu_all_mask);
@@ -429,35 +412,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
EXPORT_SYMBOL(call_usermodehelper_setfns);
/**
- * call_usermodehelper_stdinpipe - set up a pipe to be used for stdin
- * @sub_info: a subprocess_info returned by call_usermodehelper_setup
- * @filp: set to the write-end of a pipe
- *
- * This constructs a pipe, and sets the read end to be the stdin of the
- * subprocess, and returns the write-end in *@filp.
- */
-int call_usermodehelper_stdinpipe(struct subprocess_info *sub_info,
- struct file **filp)
-{
- struct file *f;
-
- f = create_write_pipe(0);
- if (IS_ERR(f))
- return PTR_ERR(f);
- *filp = f;
-
- f = create_read_pipe(f, 0);
- if (IS_ERR(f)) {
- free_write_pipe(*filp);
- return PTR_ERR(f);
- }
- sub_info->stdin = f;
-
- return 0;
-}
-EXPORT_SYMBOL(call_usermodehelper_stdinpipe);
-
-/**
* call_usermodehelper_exec - start a usermode application
* @sub_info: information about the subprocessa
* @wait: wait for the application to finish and return status.
@@ -504,42 +458,6 @@ unlock:
}
EXPORT_SYMBOL(call_usermodehelper_exec);
-/**
- * call_usermodehelper_pipe - call a usermode helper process with a pipe stdin
- * @path: path to usermode executable
- * @argv: arg vector for process
- * @envp: environment for process
- * @filp: set to the write-end of a pipe
- *
- * This is a simple wrapper which executes a usermode-helper function
- * with a pipe as stdin. It is implemented entirely in terms of
- * lower-level call_usermodehelper_* functions.
- */
-int call_usermodehelper_pipe(char *path, char **argv, char **envp,
- struct file **filp)
-{
- struct subprocess_info *sub_info;
- int ret;
-
- sub_info = call_usermodehelper_setup(path, argv, envp,
- GFP_KERNEL);
- if (sub_info == NULL)
- return -ENOMEM;
-
- ret = call_usermodehelper_stdinpipe(sub_info, filp);
- if (ret < 0) {
- call_usermodehelper_freeinfo(sub_info);
- return ret;
- }
-
- ret = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
- if (ret < 0) /* Failed to execute helper, close pipe */
- filp_close(*filp, NULL);
-
- return ret;
-}
-EXPORT_SYMBOL(call_usermodehelper_pipe);
-
void __init usermodehelper_init(void)
{
khelper_wq = create_singlethread_workqueue("khelper");
Oleg Nesterov
This is "2/2", the second patch. And the next email is "1/2".
Neil Horman
> Neil, you confused the ordering ;)
>
> This is "2/2", the second patch. And the next email is "1/2".
>
the patch names while I'm at it :)
Neil