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    [W:0.029 / U:90.480 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site