Messages in this thread | | | Date | Fri, 26 Jun 2009 12:37:23 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 1/2] exec: Make do_coredump more robust and safer when using pipes in core_pattern: recursive dump detection |
| |
On Fri, 26 Jun 2009 14:02:22 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> > core_pattern: Change how we detect recursive dumps with core_pattern pipes > > Change how we detect recursive dumps. Currently we have a mechanism by which > we try to compare pathnames of the crashing process to the core_pattern path. > This is broken for a dozen reasons, and just doesn't work in any sort of robust > way. I'm replacing it with the use of a 0 RLIMIT_CORE value. Since helper > apps set RLIMIT_CORE to zero, we don't write out core files for any process with > that particular limit set. It the core_pattern is a pipe, any non-zero limit is > translated to RLIM_INFINITY. This allows complete dumps to be captured, but > prevents infinite recursion in the event that the core_pattern process itself > crashes. >
The patch appears to be against 2.6.30 or something. I get rejects due to some other patch in exec.c which was added three weeks ago. Please don't do that :(
> > > exec.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index ebe359f..163cfa7 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1802,22 +1802,28 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs) > goto fail_unlock; > > if (ispipe) { > - helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc); > - /* Terminate the string before the first option */ > - delimit = strchr(corename, ' '); > - if (delimit) > - *delimit = '\0'; > - delimit = strrchr(helper_argv[0], '/'); > - if (delimit) > - delimit++; > - else > - delimit = helper_argv[0]; > - if (!strcmp(delimit, current->comm)) { > - printk(KERN_NOTICE "Recursive core dump detected, " > - "aborting\n"); > + if (core_limit == 0) { > + /* > + * Normally core limits are irrelevant to pipes, since > + * we're not writing to the file system, but we use > + * core_limit of 0 here as a speacial value. Any > + * non-zero 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 > + * 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 right pid if a thread > + * in a multi-threaded core_pattern process dies. > + */ > + printk(KERN_WARNING "Process %d(%s) has RLIMIT_CORE set to 0\n", > + task_tgid_vnr(current), current->comm); > + printk(KERN_WARNING "Aborting core\n"); > goto fail_unlock; > }
A few cosmetic things:
- The asterisks don't line up in the comment block. Normally we'll do
/* * *
rather than
/* * *
- The comment overflows 80 columns and makes a mess.
- Would it not be neater to do this check in a separate function? Then the comment block can go above the function rather than being all scrunched to the right and do_coredump() (which is already >150 lines) just gets
if (ispipe) { + if (core_limit_is_zero()) + goto fail_unlock;
| |