lkml.org 
[lkml]   [2010]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] exec: allow core_pipe recursion check to look for a value of 1 rather than 0
On Thu, 21 Jan 2010 15:08:06 -0500
Neil Horman <nhorman@tuxdriver.com> wrote:

> 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 some of the Abrt maintainers
> in fedora, with good results. Patch applies to the latest -mm tree as of this
> AM.
>

hrm. Fair enough, I guess..

> +/*
> + * This is used as a helper to set up the task that execs
> + * our user space core collector application
> + * Its called in the context of the task thats going to
> + * exec itself to be the helper, so we can modify current here
> + */

It's worth spending another 15 seconds on the comments. That way, they
end up looking like they're written in English.

> +void core_pipe_setup(void)
> +{
> + task_lock(current->group_leader);
> + current->signal->rlim[RLIMIT_CORE].rlim_cur = 1;
> + task_unlock(current->group_leader);
> +}

I'll make this static.

> --- a/fs/nfs/cache_lib.c
> +++ b/fs/nfs/cache_lib.c
> @@ -46,7 +46,7 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)
>
> if (nfs_cache_getent_prog[0] == '\0')
> goto out;
> - ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
> + ret = call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_EXEC);
> /*
> * Disable the upcall mechanism if we're getting an ENOENT or
> * EACCES error. The admin can re-enable it on the fly by using
> diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
> index f3df0ba..dddf780 100644
> --- a/fs/ocfs2/stackglue.c
> +++ b/fs/ocfs2/stackglue.c
> @@ -407,7 +407,7 @@ static void ocfs2_leave_group(const char *group)
> envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
> envp[2] = NULL;
>
> - ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> + ret = call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_PROC);
> if (ret < 0) {
> printk(KERN_ERR
> "ocfs2: Error %d running user helper "
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 384ca8b..ca5e531 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -48,7 +48,9 @@ struct subprocess_info;
>
> /* Allocate a subprocess_info structure */
> struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> - char **envp, gfp_t gfp_mask);
> + char **envp,
> + void (*finit)(void),
> + gfp_t gfp_mask);

What does "finit" mean? Doesn't seem very intuitive.

> /* Set various pieces of state into the subprocess_info structure */
> void call_usermodehelper_setkeys(struct subprocess_info *info,
> @@ -72,12 +74,13 @@ 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(char *path, char **argv, char **envp,
> + void (*finit)(void), 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);
> + info = call_usermodehelper_setup(path, argv, envp, finit, gfp_mask);
> if (info == NULL)
> return -ENOMEM;
> return call_usermodehelper_exec(info, wait);

The semantics of the `finit' callback should be documented here. It's
a kernel-wide interface and others might want to use it.


You're not a big fan of checkpatch, it seems.


\
 
 \ /
  Last update: 2010-01-27 01:01    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site