From a06a4dc3a08201ff6a8a958f935b3cbf7744115f Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Wed, 26 May 2010 14:42:58 -0700 Subject: kmod: add init function to usermodehelper 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 fixed 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: 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 Reviewed-by: Oleg Nesterov Cc: Andi Kleen Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kmod.h | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-) (limited to 'include/linux/kmod.h') diff --git a/include/linux/kmod.h b/include/linux/kmod.h index facb27fe7de0..f9edf63e90ea 100644 --- a/include/linux/kmod.h +++ b/include/linux/kmod.h @@ -23,6 +23,7 @@ #include #include #include +#include #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,17 +90,31 @@ 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) -- cgit v1.2.3 From 898b374af6f71041bd3bceebe257e564f3f1d458 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Wed, 26 May 2010 14:42:59 -0700 Subject: exec: 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 Reviewed-by: Oleg Nesterov Cc: Andi Kleen Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kmod.h | 7 ------- 1 file changed, 7 deletions(-) (limited to 'include/linux/kmod.h') diff --git a/include/linux/kmod.h b/include/linux/kmod.h index f9edf63e90ea..5c058778ad35 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); -- cgit v1.2.3 From 685bfd2c48bb3284d31e73ff3151c957d76deda9 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 26 May 2010 14:43:00 -0700 Subject: umh: creds: convert call_usermodehelper_keys() to use subprocess_info->init() call_usermodehelper_keys() uses call_usermodehelper_setkeys() to change subprocess_info->cred in advance. Now that we have info->init() we can change this code to set tgcred->session_keyring in context of execing kernel thread. Note: since currently call_usermodehelper_keys() is never called with UMH_NO_WAIT, call_usermodehelper_keys()->key_get() and umh_keys_cleanup() are not really needed, we could rely on install_session_keyring_to_cred() which does key_get() on success. Signed-off-by: Oleg Nesterov Acked-by: Neil Horman Acked-by: David Howells Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kmod.h | 17 ----------------- 1 file changed, 17 deletions(-) (limited to 'include/linux/kmod.h') diff --git a/include/linux/kmod.h b/include/linux/kmod.h index 5c058778ad35..d876dce217f0 100644 --- a/include/linux/kmod.h +++ b/include/linux/kmod.h @@ -72,8 +72,6 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask); /* Set various pieces of state into the subprocess_info structure */ -void call_usermodehelper_setkeys(struct subprocess_info *info, - struct key *session_keyring); void call_usermodehelper_setfns(struct subprocess_info *info, int (*init)(struct subprocess_info *info), void (*cleanup)(struct subprocess_info *info), @@ -112,21 +110,6 @@ call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait) NULL, NULL, NULL); } -static inline int -call_usermodehelper_keys(char *path, char **argv, char **envp, - struct key *session_keyring, enum umh_wait 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); - if (info == NULL) - return -ENOMEM; - - call_usermodehelper_setkeys(info, session_keyring); - return call_usermodehelper_exec(info, wait); -} - extern void usermodehelper_init(void); extern int usermodehelper_disable(void); -- cgit v1.2.3 From c70a626d3eba373514c72287c93588b6974a0059 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 26 May 2010 14:43:01 -0700 Subject: umh: creds: kill subprocess_info->cred logic Now that nobody ever changes subprocess_info->cred we can kill this member and related code. ____call_usermodehelper() always runs in the context of freshly forked kernel thread, it has the proper ->cred copied from its parent kthread, keventd. Signed-off-by: Oleg Nesterov Acked-by: Neil Horman Acked-by: David Howells Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kmod.h | 1 - 1 file changed, 1 deletion(-) (limited to 'include/linux/kmod.h') diff --git a/include/linux/kmod.h b/include/linux/kmod.h index d876dce217f0..6efd7a78de6a 100644 --- a/include/linux/kmod.h +++ b/include/linux/kmod.h @@ -56,7 +56,6 @@ enum umh_wait { struct subprocess_info { struct work_struct work; struct completion *complete; - struct cred *cred; char *path; char **argv; char **envp; -- cgit v1.2.3