Re: [PATCH] coredump: fix pipe coredump when core limit is 0

[Oleg Nesterov <oleg@redhat.com>]

On 08/21, bookjovi@gmail.com wrote:
>
> From: Jovi Zhang <bookjovi@gmail.com>
>
> Regressing from 2.6.35

Hmm. Thanks Jovi.

> In pipe coredump case, normally core limits are irrelevant,
> since we're not writing to the file system, but core limit 0
> is a special value, kernel should skip the dump when limit is 0.

Hmm. probably yes... although I'd say I do not really know. iirc,
previously RLIMIT_CORE was simply ignored if ispipe. But then we
changed the rules many time.

Yes. See 725eae32df7754044809973034429a47e6035158. This is where
we changed the "limit == 0 && ispipe" behaviour.

> This error intruduced by commit c71354 in 2.6.35, that commit put
> core limit zero check into non-pipe code branch.
>
>     commit c713541125002b8bc9e681af3b09118e771e2d8a
>     Author: Oleg Nesterov <oleg@redhat.com>
>     Date:   Wed May 26 14:43:05 2010 -0700
>
>     coredump: factor out the not-ispipe file checks

Cough. I don't think so ;)

Yes, that patch moves the check, but please note that before the patch
we did

	if ((!ispipe) && (cprm.limit < binfmt->min_coredump))
		goto fail;

so I do not think this patch can make any difference.

I think this was changed by 898b374af6f71041bd3bceebe257e564f3f1d458.

> For non-pipe case, limit 0 also means drop the coredump, so just put
> the zero limit check at do_coredump function begining.

Neil, what do you think? Should we change the code or the comment?


As for the patch, it is not exactly right in any case,

> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -2119,6 +2119,10 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  	if (!__get_dumpable(cprm.mm_flags))
>  		goto fail;
>
> +	/* Core limit as 0 should skip the dump */
> +	if (cprm.limit == 0)
> +		goto fail;

Even if we do not dump, we should kill all tasks/threads which use
this ->mm. We shouldn't miss coredump_wait().

To clarify, I don't really know _why_, and probably it makes sense
to change this behaviour. But this needs a separate patch plus
discussion.

Oleg.