Message-Id: <20100126155359.c5ad7cd3.akpm@linux-foundation.org>
Date:	Tue, 26 Jan 2010 15:53:59 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Neil Horman <nhorman@...driver.com>
Cc:	linux-kernel@...r.kernel.org, jmoskovc@...hat.com,
	mingo@...hat.com, drbd-dev@...ts.linbit.com,
	benh@...nel.crashing.org, t.sailer@...mni.ethz.ch, abelay@....edu,
	gregkh@...e.de, spock@...too.org, viro@...iv.linux.org.uk,
	neilb@...e.de, mfasheh@...e.com, menage@...gle.com,
	shemminger@...ux-foundation.org, takedakn@...data.co.jp
Subject: Re: [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@...driver.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists