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

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

[bookjovi]

From: Jovi Zhang <bookjovi@gmail.com>

Regressing from 2.6.35

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.

Note that most Linux distribution set default core file limit as 0,
because many user don't want to get core file even process crash,
wahtever pipe coredump pattern used or not.

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

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

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
---
 fs/exec.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 25dcbe5..c33085d 100644
--- 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;
+
 	cred = prepare_creds();
 	if (!cred)
 		goto fail;
-- 
1.7.2.3

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

[Oleg Nesterov]

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.

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

[Oleg Nesterov]

On 08/21, Oleg Nesterov wrote:
>
> On 08/21, bookjovi@gmail.com wrote:
> >
> > 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?

Personally I think we should fix the comment. I think RLIMIT_CORE
doesn't apply in this case, limit == 1 check is very special. And
this is what linux always did, except between 725eae32 and 898b374a.


Oleg.

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

[Neil Horman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 752 bytes --]

Concur.  The comment should be changed
Neil

Oleg Nesterov <oleg@redhat.com> wrote:

>On 08/21, Oleg Nesterov wrote:
>>
>> On 08/21, bookjovi@gmail.com wrote:
>> >
>> > 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?
>
>Personally I think we should fix the comment. I think RLIMIT_CORE
>doesn't apply in this case, limit == 1 check is very special. And
>this is what linux always did, except between 725eae32 and 898b374a.
>
>
>Oleg.
>
>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Jovi Zhang]

Sorry, I should dig the problem more deeply.
I will resend the patch for fix the comments. Thanks.

On Sun, Aug 21, 2011 at 6:36 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
>
> Concur.  The comment should be changed
> Neil
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> >On 08/21, Oleg Nesterov wrote:
> >>
> >> On 08/21, bookjovi@gmail.com wrote:
> >> >
> >> > 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?
> >
> >Personally I think we should fix the comment. I think RLIMIT_CORE
> >doesn't apply in this case, limit == 1 check is very special. And
> >this is what linux always did, except between 725eae32 and 898b374a.
> >
> >
> >Oleg.
> >
> >

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

[Oleg Nesterov]

On 08/22, Jovi Zhang wrote:
>
> I will resend the patch for fix the comments.

Yes, please ;)

While at it, could you add something like

	* See umh_pipe_setup() which sets RLIMIT_CORE = 1.

at the start of this comment? Unless you know how the code works
it is not obvious where this "1" comes from.

Oleg.

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

[Pádraig Brady]

On 08/21/2011 11:36 PM, Neil Horman wrote:
> Concur.  The comment should be changed
> Neil
> 
> Oleg Nesterov <oleg@redhat.com> wrote:
> 
>> On 08/21, Oleg Nesterov wrote:
>>>
>>> On 08/21, bookjovi@gmail.com wrote:
>>>>
>>>> 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?
>>
>> Personally I think we should fix the comment. I think RLIMIT_CORE
>> doesn't apply in this case, limit == 1 check is very special. And
>> this is what linux always did, except between 725eae32 and 898b374a.

Sorry for jumping in late here.
I would really like `ulimit -c 0` to completely disable core dumps,
including not running core_pattern, as I also mentioned here:
https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
I noticed this in a script where ctrl-\ was taking a long
time to be registered as the core_pattern was run unconditionally.

Testing on 2.6.38.8-34.fc15.x86_64 here shows the IMHO problematic behavior:

# echo "|/bin/true" > /proc/sys/kernel/core_pattern
# ulimit -c 0
# cat
^\Quit (core dumped)

cheers,
Pádraig.

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

[Oleg Nesterov]

On 08/22, Pádraig Brady wrote:
>
> On 08/21/2011 11:36 PM, Neil Horman wrote:
> > Concur.  The comment should be changed
> > Neil
> >
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> >> On 08/21, Oleg Nesterov wrote:
> >>>
> >>> On 08/21, bookjovi@gmail.com wrote:
> >>>>
> >>>> 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?
> >>
> >> Personally I think we should fix the comment. I think RLIMIT_CORE
> >> doesn't apply in this case, limit == 1 check is very special. And
> >> this is what linux always did, except between 725eae32 and 898b374a.
>
> Sorry for jumping in late here.
> I would really like `ulimit -c 0` to completely disable core dumps,
> including not running core_pattern, as I also mentioned here:
> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
> I noticed this in a script where ctrl-\ was taking a long
> time to be registered as the core_pattern was run unconditionally.

May be. As I said, I do not really know and personally I agree with
everything. My only point was, this is not the bug, this is what we
always did.

This is up to Neil, I think.

Oleg.

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

[Jovi Zhang]

2011/8/23 Oleg Nesterov <oleg@redhat.com>:
> On 08/22, Pádraig Brady wrote:
>>
>> On 08/21/2011 11:36 PM, Neil Horman wrote:
>> > Concur.  The comment should be changed
>> > Neil
>> >
>> > Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> >> On 08/21, Oleg Nesterov wrote:
>> >>>
>> >>> On 08/21, bookjovi@gmail.com wrote:
>> >>>>
>> >>>> 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?
>> >>
>> >> Personally I think we should fix the comment. I think RLIMIT_CORE
>> >> doesn't apply in this case, limit == 1 check is very special. And
>> >> this is what linux always did, except between 725eae32 and 898b374a.
>>
>> Sorry for jumping in late here.
>> I would really like `ulimit -c 0` to completely disable core dumps,
>> including not running core_pattern, as I also mentioned here:
>> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
>> I noticed this in a script where ctrl-\ was taking a long
>> time to be registered as the core_pattern was run unconditionally.
>
> May be. As I said, I do not really know and personally I agree with
> everything. My only point was, this is not the bug, this is what we
> always did.
>
> This is up to Neil, I think.
>
> Oleg.
>
>
Well, so here have two questions.
1) That comments "but a limit of 0 skips the dump" definitely is wrong
right now, it don't match with reality.
2) In ispipe case, core limit 0 should skip the dump or not? this need
more discussion.
   from pipe coredump point of view, core limit is irrelevant, it
doesn't write to file system.
   from user point of view, there will be a lot of core files if we
let core limit 0 create core file, user might be boring.

I fix the comments part by below patch(thanks Oleg's comments), please
use attachment patch when merge.

>From dc7b02a1e0e413fb96d22f1d4ef4da98115cfb9d Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@gmail.com>
Date: Wed, 17 Aug 2011 15:34:29 +0800
Subject: [PATCH] coredump: fix wrong comments on core limits of pipe
coredump case

In commit 898b374a, core limits recursive check vaule changed from 0 to 1,
but the corresponding comments was not changed correctly.

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
---
 fs/exec.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 25dcbe5..ba493cc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -2158,15 +2158,16 @@ void do_coredump(long signr, int exit_code,
struct pt_regs *regs)
 		}

 		if (cprm.limit == 1) {
-			/*
+			/* See umh_pipe_setup() which sets RLIMIT_CORE = 1.
+			 *
 			 * Normally core limits are irrelevant to pipes, since
 			 * we're not writing to the file system, but we use
-			 * cprm.limit of 1 here as a speacial value. Any
-			 * non-1 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 =  !1
-			 * but it runs as root, and can do lots of stupid things
+			 * cprm.limit of 1 here as a speacial value, this is a
+			 * consistent way to catch recursive crashes.
+			 * We can still crash if the core_pattern binary sets
+			 * RLIM_CORE = !1, 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
-- 
1.6.5.2

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

[Jovi Zhang]

[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]

On Wed, Aug 24, 2011 at 6:14 PM, Jovi Zhang <bookjovi@gmail.com> wrote:
> 2011/8/23 Oleg Nesterov <oleg@redhat.com>:
>> On 08/22, Pádraig Brady wrote:
>>>
>>> On 08/21/2011 11:36 PM, Neil Horman wrote:
>>> > Concur.  The comment should be changed
>>> > Neil
>>> >
>>> > Oleg Nesterov <oleg@redhat.com> wrote:
>>> >
>>> >> On 08/21, Oleg Nesterov wrote:
>>> >>>
>>> >>> On 08/21, bookjovi@gmail.com wrote:
>>> >>>>
>>> >>>> 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?
>>> >>
>>> >> Personally I think we should fix the comment. I think RLIMIT_CORE
>>> >> doesn't apply in this case, limit == 1 check is very special. And
>>> >> this is what linux always did, except between 725eae32 and 898b374a.
>>>
>>> Sorry for jumping in late here.
>>> I would really like `ulimit -c 0` to completely disable core dumps,
>>> including not running core_pattern, as I also mentioned here:
>>> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
>>> I noticed this in a script where ctrl-\ was taking a long
>>> time to be registered as the core_pattern was run unconditionally.
>>
>> May be. As I said, I do not really know and personally I agree with
>> everything. My only point was, this is not the bug, this is what we
>> always did.
>>
>> This is up to Neil, I think.
>>
>> Oleg.
>>
>>
> Well, so here have two questions.
> 1) That comments "but a limit of 0 skips the dump" definitely is wrong
> right now, it don't match with reality.
> 2) In ispipe case, core limit 0 should skip the dump or not? this need
> more discussion.
>    from pipe coredump point of view, core limit is irrelevant, it
> doesn't write to file system.
>    from user point of view, there will be a lot of core files if we
> let core limit 0 create core file, user might be boring.
>
> I fix the comments part by below patch(thanks Oleg's comments), please
> use attachment patch when merge.
>

Patch attached.

[-- Attachment #2: 0001-coredump-fix-wrong-comments-on-core-limits-of-pipe-c.patch --]
[-- Type: application/octet-stream, Size: 1761 bytes --]

From dc7b02a1e0e413fb96d22f1d4ef4da98115cfb9d Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@gmail.com>
Date: Wed, 17 Aug 2011 15:34:29 +0800
Subject: [PATCH] coredump: fix wrong comments on core limits of pipe coredump case

In commit 898b374a, core limits recursive check vaule changed from 0 to 1,
but the corresponding comments was not changed correctly.

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
---
 fs/exec.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 25dcbe5..ba493cc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -2158,15 +2158,16 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		}
 
 		if (cprm.limit == 1) {
-			/*
+			/* See umh_pipe_setup() which sets RLIMIT_CORE = 1.
+			 *
 			 * Normally core limits are irrelevant to pipes, since
 			 * we're not writing to the file system, but we use
-			 * cprm.limit of 1 here as a speacial value. Any
-			 * non-1 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 =  !1
-			 * but it runs as root, and can do lots of stupid things
+			 * cprm.limit of 1 here as a speacial value, this is a
+			 * consistent way to catch recursive crashes.
+			 * We can still crash if the core_pattern binary sets
+			 * RLIM_CORE = !1, 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
-- 
1.6.5.2

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

[Neil Horman]

On Wed, Aug 24, 2011 at 06:14:24PM +0800, Jovi Zhang wrote:
> 2011/8/23 Oleg Nesterov <oleg@redhat.com>:
> > On 08/22, Pádraig Brady wrote:
> >>
> >> On 08/21/2011 11:36 PM, Neil Horman wrote:
> >> > Concur.  The comment should be changed
> >> > Neil
> >> >
> >> > Oleg Nesterov <oleg@redhat.com> wrote:
> >> >
> >> >> On 08/21, Oleg Nesterov wrote:
> >> >>>
> >> >>> On 08/21, bookjovi@gmail.com wrote:
> >> >>>>
> >> >>>> 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?
> >> >>
> >> >> Personally I think we should fix the comment. I think RLIMIT_CORE
> >> >> doesn't apply in this case, limit == 1 check is very special. And
> >> >> this is what linux always did, except between 725eae32 and 898b374a.
> >>
> >> Sorry for jumping in late here.
> >> I would really like `ulimit -c 0` to completely disable core dumps,
> >> including not running core_pattern, as I also mentioned here:
> >> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
> >> I noticed this in a script where ctrl-\ was taking a long
> >> time to be registered as the core_pattern was run unconditionally.
> >
> > May be. As I said, I do not really know and personally I agree with
> > everything. My only point was, this is not the bug, this is what we
> > always did.
> >
> > This is up to Neil, I think.
> >
> > Oleg.
> >
> >
> Well, so here have two questions.
> 1) That comments "but a limit of 0 skips the dump" definitely is wrong
> right now, it don't match with reality.
Agreed, I think your patch fixes this correctly.

> 2) In ispipe case, core limit 0 should skip the dump or not? this need
> more discussion.
>    from pipe coredump point of view, core limit is irrelevant, it
> doesn't write to file system.
>    from user point of view, there will be a lot of core files if we
> let core limit 0 create core file, user might be boring.
> 
The case (ispipe==true && cprm.lmit==0) has to result in us dumping a core. I
use to be convinced otherwise, but several user space developers changed my
mind, particularly the guys writing the abrt daemon.  The reason being, the
default process limit for RLIMIT_CORE is zero.  If you're writing a daemon like
abrt that wants to catch program crashes, even during boot, there are tons of
hoops you have to jump through to get core pipes enabled properly if you need to
change RLIMIT_CORE.  Specifically you have to modify all existing processes
RLIMIT_CORE values to be non-zero (a racy proposition) as well as modify the
init processes RLIMIT_CORE value (so that it gets inherited by future
processes).  Thats a pretty rickety thing to set up, and they really didn't want
to have that much fiddling to do to get it all working, and I don't blame them.
The fact that you're setting up a core pipe in the first place, implies to user
space that you want an executed notification of cores, and in that execution you
have the ability to filter which cores you actually care about.  If you're
worried about too many processes spawning or getting the cpu bogged with crash
handling, we have the core_limit sysctl to keep us throttled.

The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
skip the core dump, breaks lots of user space expectations (which I know, is
counter-intuitive), but changing it will open up a large can of worms, it works
properly as it is.

> I fix the comments part by below patch(thanks Oleg's comments), please
> use attachment patch when merge.
> 
> From dc7b02a1e0e413fb96d22f1d4ef4da98115cfb9d Mon Sep 17 00:00:00 2001
> From: Jovi Zhang <bookjovi@gmail.com>
> Date: Wed, 17 Aug 2011 15:34:29 +0800
> Subject: [PATCH] coredump: fix wrong comments on core limits of pipe
> coredump case
> 
> In commit 898b374a, core limits recursive check vaule changed from 0 to 1,
> but the corresponding comments was not changed correctly.
> 
> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

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

[Pádraig Brady]

On 08/24/2011 12:01 PM, Neil Horman wrote:
> On Wed, Aug 24, 2011 at 06:14:24PM +0800, Jovi Zhang wrote:
>> 2011/8/23 Oleg Nesterov <oleg@redhat.com>:
>>> On 08/22, Pádraig Brady wrote:
>>>>
>>>> On 08/21/2011 11:36 PM, Neil Horman wrote:
>>>>> Concur.  The comment should be changed
>>>>> Neil
>>>>>
>>>>> Oleg Nesterov <oleg@redhat.com> wrote:
>>>>>
>>>>>> On 08/21, Oleg Nesterov wrote:
>>>>>>>
>>>>>>> On 08/21, bookjovi@gmail.com wrote:
>>>>>>>>
>>>>>>>> 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?
>>>>>>
>>>>>> Personally I think we should fix the comment. I think RLIMIT_CORE
>>>>>> doesn't apply in this case, limit == 1 check is very special. And
>>>>>> this is what linux always did, except between 725eae32 and 898b374a.
>>>>
>>>> Sorry for jumping in late here.
>>>> I would really like `ulimit -c 0` to completely disable core dumps,
>>>> including not running core_pattern, as I also mentioned here:
>>>> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
>>>> I noticed this in a script where ctrl-\ was taking a long
>>>> time to be registered as the core_pattern was run unconditionally.
>>>
>>> May be. As I said, I do not really know and personally I agree with
>>> everything. My only point was, this is not the bug, this is what we
>>> always did.
>>>
>>> This is up to Neil, I think.
>>>
>>> Oleg.
>>>
>>>
>> Well, so here have two questions.
>> 1) That comments "but a limit of 0 skips the dump" definitely is wrong
>> right now, it don't match with reality.
> Agreed, I think your patch fixes this correctly.
> 
>> 2) In ispipe case, core limit 0 should skip the dump or not? this need
>> more discussion.
>>    from pipe coredump point of view, core limit is irrelevant, it
>> doesn't write to file system.
>>    from user point of view, there will be a lot of core files if we
>> let core limit 0 create core file, user might be boring.
>>
> The case (ispipe==true && cprm.lmit==0) has to result in us dumping a core. I
> use to be convinced otherwise, but several user space developers changed my
> mind, particularly the guys writing the abrt daemon.  The reason being, the
> default process limit for RLIMIT_CORE is zero.  If you're writing a daemon like
> abrt that wants to catch program crashes, even during boot, there are tons of
> hoops you have to jump through to get core pipes enabled properly if you need to
> change RLIMIT_CORE.  Specifically you have to modify all existing processes
> RLIMIT_CORE values to be non-zero (a racy proposition) as well as modify the
> init processes RLIMIT_CORE value (so that it gets inherited by future
> processes).  Thats a pretty rickety thing to set up, and they really didn't want
> to have that much fiddling to do to get it all working, and I don't blame them.

I'm not convinced by that, but thanks for the info.

> The fact that you're setting up a core pipe in the first place, implies to user
> space that you want an executed notification of cores, and in that execution you
> have the ability to filter which cores you actually care about.  If you're
> worried about too many processes spawning or getting the cpu bogged with crash
> handling, we have the core_limit sysctl to keep us throttled.

For the archive, I think you're referring to core_pipe_limit which sets
the limit of the number of processes to wait for in parallel.

> The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> skip the core dump, breaks lots of user space expectations (which I know, is
> counter-intuitive), but changing it will open up a large can of worms, it works
> properly as it is.

OK. Retesting on my new laptop and latest abrt implementation,
shows that the response to SIGQUIT is much better (on the order of 20ms).
Still there is lots of redundant logic triggered by default
on most systems where abrt et. al. are used.

Drats I just noticed another problem with not being able to disable core dumps
for a process. The `timeout` command from coreutils now tries to propagate
the signal from the process it's monitoring up. But since core dumps
can't be disabled, abrt will attribute any crashes to `timeout` rather
than what it's monitoring. I'll guess we'll have to revert:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=5a647a0

cheers,
Pádraig.

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

[Neil Horman]

On Thu, Aug 25, 2011 at 11:03:35AM +0100, Pádraig Brady wrote:
> On 08/24/2011 12:01 PM, Neil Horman wrote:
> > On Wed, Aug 24, 2011 at 06:14:24PM +0800, Jovi Zhang wrote:
> >> 2011/8/23 Oleg Nesterov <oleg@redhat.com>:
> >>> On 08/22, Pádraig Brady wrote:
> >>>>
> >>>> On 08/21/2011 11:36 PM, Neil Horman wrote:
> >>>>> Concur.  The comment should be changed
> >>>>> Neil
> >>>>>
> >>>>> Oleg Nesterov <oleg@redhat.com> wrote:
> >>>>>
> >>>>>> On 08/21, Oleg Nesterov wrote:
> >>>>>>>
> >>>>>>> On 08/21, bookjovi@gmail.com wrote:
> >>>>>>>>
> >>>>>>>> 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?
> >>>>>>
> >>>>>> Personally I think we should fix the comment. I think RLIMIT_CORE
> >>>>>> doesn't apply in this case, limit == 1 check is very special. And
> >>>>>> this is what linux always did, except between 725eae32 and 898b374a.
> >>>>
> >>>> Sorry for jumping in late here.
> >>>> I would really like `ulimit -c 0` to completely disable core dumps,
> >>>> including not running core_pattern, as I also mentioned here:
> >>>> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
> >>>> I noticed this in a script where ctrl-\ was taking a long
> >>>> time to be registered as the core_pattern was run unconditionally.
> >>>
> >>> May be. As I said, I do not really know and personally I agree with
> >>> everything. My only point was, this is not the bug, this is what we
> >>> always did.
> >>>
> >>> This is up to Neil, I think.
> >>>
> >>> Oleg.
> >>>
> >>>
> >> Well, so here have two questions.
> >> 1) That comments "but a limit of 0 skips the dump" definitely is wrong
> >> right now, it don't match with reality.
> > Agreed, I think your patch fixes this correctly.
> > 
> >> 2) In ispipe case, core limit 0 should skip the dump or not? this need
> >> more discussion.
> >>    from pipe coredump point of view, core limit is irrelevant, it
> >> doesn't write to file system.
> >>    from user point of view, there will be a lot of core files if we
> >> let core limit 0 create core file, user might be boring.
> >>
> > The case (ispipe==true && cprm.lmit==0) has to result in us dumping a core. I
> > use to be convinced otherwise, but several user space developers changed my
> > mind, particularly the guys writing the abrt daemon.  The reason being, the
> > default process limit for RLIMIT_CORE is zero.  If you're writing a daemon like
> > abrt that wants to catch program crashes, even during boot, there are tons of
> > hoops you have to jump through to get core pipes enabled properly if you need to
> > change RLIMIT_CORE.  Specifically you have to modify all existing processes
> > RLIMIT_CORE values to be non-zero (a racy proposition) as well as modify the
> > init processes RLIMIT_CORE value (so that it gets inherited by future
> > processes).  Thats a pretty rickety thing to set up, and they really didn't want
> > to have that much fiddling to do to get it all working, and I don't blame them.
> 
> I'm not convinced by that, but thanks for the info.
> 
I wasn't either.  But the outcry from those developers was strong enough (and
the support for the way I had it was silent), that I'm not willing to support
chaning it back.

> > The fact that you're setting up a core pipe in the first place, implies to user
> > space that you want an executed notification of cores, and in that execution you
> > have the ability to filter which cores you actually care about.  If you're
> > worried about too many processes spawning or getting the cpu bogged with crash
> > handling, we have the core_limit sysctl to keep us throttled.
> 
> For the archive, I think you're referring to core_pipe_limit which sets
> the limit of the number of processes to wait for in parallel.
> 
Yes, thats correct, thank you.

 > The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> > skip the core dump, breaks lots of user space expectations (which I know, is
> > counter-intuitive), but changing it will open up a large can of worms, it works
> > properly as it is.
> 
> OK. Retesting on my new laptop and latest abrt implementation,
> shows that the response to SIGQUIT is much better (on the order of 20ms).
> Still there is lots of redundant logic triggered by default
> on most systems where abrt et. al. are used.
It is, but I had that conversation with the abrt guys too, and they seemed ok
with it, and went to effort to minimize that path.

> 
> Drats I just noticed another problem with not being able to disable core dumps
> for a process. The `timeout` command from coreutils now tries to propagate
> the signal from the process it's monitoring up. But since core dumps
> can't be disabled, abrt will attribute any crashes to `timeout` rather
> than what it's monitoring. I'll guess we'll have to revert:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=5a647a0
> 
That suggests to me that we need another flag that user space can control to
override this behavior.  It appears we may have one too - the PR_SET_DUMPABLE
flag in the prctl syscall.  Its not POSIX compliant so you'd have to ifdef it
into coreutils, but it exists, and sounds like exactly what you'd need in the
above case.  Looking at do_coredump, clearing the PR_SET_DUMPABLE flag with
prctl causes __get_dumpable to return 0, which skips all the code in do_coredump
entirely.  I think thats identical behavior to having a zero core limit skip the
dump.

Regards
Neil

> cheers,
> Pádraig.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Pádraig Brady]

On 08/25/2011 11:55 AM, Neil Horman wrote:
> On Thu, Aug 25, 2011 at 11:03:35AM +0100, Pádraig Brady wrote:
>>
>> Drats I just noticed another problem with not being able to disable core dumps
>> for a process. The `timeout` command from coreutils now tries to propagate
>> the signal from the process it's monitoring up. But since core dumps
>> can't be disabled, abrt will attribute any crashes to `timeout` rather
>> than what it's monitoring. I'll guess we'll have to revert:
>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=5a647a0
>>
> That suggests to me that we need another flag that user space can control to
> override this behavior.  It appears we may have one too - the PR_SET_DUMPABLE
> flag in the prctl syscall.  Its not POSIX compliant so you'd have to ifdef it
> into coreutils, but it exists, and sounds like exactly what you'd need in the
> above case.  Looking at do_coredump, clearing the PR_SET_DUMPABLE flag with
> prctl causes __get_dumpable to return 0, which skips all the code in do_coredump
> entirely.  I think thats identical behavior to having a zero core limit skip the
> dump.

This would be the first use of prctl in coreutils,
but I think this should handle things.

thanks for that!
Pádraig.

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

[Oleg Nesterov]

On 08/24, Neil Horman wrote:
>
> The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> skip the core dump, breaks lots of user space expectations

Not sure this really makes sense, but perhaps ispipe can skip the dump
if RLIMIT_CORE == 0 _and_ the signal was sent from the user-space.

Oleg.

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

[Neil Horman]

On Thu, Aug 25, 2011 at 05:57:35PM +0200, Oleg Nesterov wrote:
> On 08/24, Neil Horman wrote:
> >
> > The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> > skip the core dump, breaks lots of user space expectations
> 
> Not sure this really makes sense, but perhaps ispipe can skip the dump
> if RLIMIT_CORE == 0 _and_ the signal was sent from the user-space.
> 
If you can guarantee that the signal came from user space, yes, that would work
I imagine.  alternatively I expect we could modify the kernel thread creation
routine such that it sets PR_SET_DUMPABLE to zero for all kernel threads

Either would work I imagine.
Neil

> Oleg.
> 
> 

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

[Oleg Nesterov]

On 08/25, Neil Horman wrote:
>
> On Thu, Aug 25, 2011 at 05:57:35PM +0200, Oleg Nesterov wrote:
> > On 08/24, Neil Horman wrote:
> > >
> > > The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> > > skip the core dump, breaks lots of user space expectations
> >
> > Not sure this really makes sense, but perhaps ispipe can skip the dump
> > if RLIMIT_CORE == 0 _and_ the signal was sent from the user-space.
> >
> If you can guarantee that the signal came from user space, yes, that would work
> I imagine.

No, I was wrong.

> alternatively I expect we could modify the kernel thread creation
> routine such that it sets PR_SET_DUMPABLE to zero for all kernel threads

Just curious... why?

Oleg.

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

[Neil Horman]

On Fri, Aug 26, 2011 at 04:11:39PM +0200, Oleg Nesterov wrote:
> On 08/25, Neil Horman wrote:
> >
> > On Thu, Aug 25, 2011 at 05:57:35PM +0200, Oleg Nesterov wrote:
> > > On 08/24, Neil Horman wrote:
> > > >
> > > > The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> > > > skip the core dump, breaks lots of user space expectations
> > >
> > > Not sure this really makes sense, but perhaps ispipe can skip the dump
> > > if RLIMIT_CORE == 0 _and_ the signal was sent from the user-space.
> > >
> > If you can guarantee that the signal came from user space, yes, that would work
> > I imagine.
> 
> No, I was wrong.
> 
> > alternatively I expect we could modify the kernel thread creation
> > routine such that it sets PR_SET_DUMPABLE to zero for all kernel threads
> 
> Just curious... why?
> 
Sorry, I read your words above backwards.  I was trying to suggest an equivalent
solution, but my idea was quite inverted.
Neil

> Oleg.
> 
> 

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

[Pádraig Brady]

On 08/25/2011 04:57 PM, Oleg Nesterov wrote:
> On 08/24, Neil Horman wrote:
>>
>> The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
>> skip the core dump, breaks lots of user space expectations
> 
> Not sure this really makes sense, but perhaps ispipe can skip the dump
> if RLIMIT_CORE == 0 _and_ the signal was sent from the user-space.

I like the sound of that, though don't know the details for
determining signal origin. SIGQUIT generated from a Ctrl-\
from the tty driver might be problematic for example.

cheers,
Pádraig.

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

[Oleg Nesterov]

On 08/26, Pádraig Brady wrote:
>
> On 08/25/2011 04:57 PM, Oleg Nesterov wrote:
> > On 08/24, Neil Horman wrote:
> >>
> >> The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> >> skip the core dump, breaks lots of user space expectations
> >
> > Not sure this really makes sense, but perhaps ispipe can skip the dump
> > if RLIMIT_CORE == 0 _and_ the signal was sent from the user-space.
>
> I like the sound of that, though don't know the details for
> determining signal origin. SIGQUIT generated from a Ctrl-\
> from the tty driver might be problematic for example.

Hmm. Thanks for correcting me.

Indeed, contrary to what I expected tty uses SEND_SIG_PRIV, not
SEND_SIG_NOINFO. This is SI_KERNEL. Hmm. OTOH, this makes sense
from check_kill_permission() pov.

Thanks.

Oleg.

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

[Jovi Zhang]

On Wed, Aug 24, 2011 at 7:01 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Wed, Aug 24, 2011 at 06:14:24PM +0800, Jovi Zhang wrote:
> > 2011/8/23 Oleg Nesterov <oleg@redhat.com>:
> > > On 08/22, Pádraig Brady wrote:
> > >>
> > >> On 08/21/2011 11:36 PM, Neil Horman wrote:
> > >> > Concur.  The comment should be changed
> > >> > Neil
> > >> >
> > >> > Oleg Nesterov <oleg@redhat.com> wrote:
> > >> >
> > >> >> On 08/21, Oleg Nesterov wrote:
> > >> >>>
> > >> >>> On 08/21, bookjovi@gmail.com wrote:
> > >> >>>>
> > >> >>>> 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?
> > >> >>
> > >> >> Personally I think we should fix the comment. I think RLIMIT_CORE
> > >> >> doesn't apply in this case, limit == 1 check is very special. And
> > >> >> this is what linux always did, except between 725eae32 and 898b374a.
> > >>
> > >> Sorry for jumping in late here.
> > >> I would really like `ulimit -c 0` to completely disable core dumps,
> > >> including not running core_pattern, as I also mentioned here:
> > >> https://bugs.launchpad.net/ubuntu/+source/apport/+bug/62511
> > >> I noticed this in a script where ctrl-\ was taking a long
> > >> time to be registered as the core_pattern was run unconditionally.
> > >
> > > May be. As I said, I do not really know and personally I agree with
> > > everything. My only point was, this is not the bug, this is what we
> > > always did.
> > >
> > > This is up to Neil, I think.
> > >
> > > Oleg.
> > >
> > >
> > Well, so here have two questions.
> > 1) That comments "but a limit of 0 skips the dump" definitely is wrong
> > right now, it don't match with reality.
> Agreed, I think your patch fixes this correctly.
>
> > 2) In ispipe case, core limit 0 should skip the dump or not? this need
> > more discussion.
> >    from pipe coredump point of view, core limit is irrelevant, it
> > doesn't write to file system.
> >    from user point of view, there will be a lot of core files if we
> > let core limit 0 create core file, user might be boring.
> >
> The case (ispipe==true && cprm.lmit==0) has to result in us dumping a core. I
> use to be convinced otherwise, but several user space developers changed my
> mind, particularly the guys writing the abrt daemon.  The reason being, the
> default process limit for RLIMIT_CORE is zero.  If you're writing a daemon like
> abrt that wants to catch program crashes, even during boot, there are tons of
> hoops you have to jump through to get core pipes enabled properly if you need to
> change RLIMIT_CORE.  Specifically you have to modify all existing processes
> RLIMIT_CORE values to be non-zero (a racy proposition) as well as modify the
> init processes RLIMIT_CORE value (so that it gets inherited by future
> processes).  Thats a pretty rickety thing to set up, and they really didn't want
> to have that much fiddling to do to get it all working, and I don't blame them.
> The fact that you're setting up a core pipe in the first place, implies to user
> space that you want an executed notification of cores, and in that execution you
> have the ability to filter which cores you actually care about.  If you're
> worried about too many processes spawning or getting the cpu bogged with crash
> handling, we have the core_limit sysctl to keep us throttled.
>
> The long and the short of it is, making RLIMIT_CORE == 0 for the ispipe case
> skip the core dump, breaks lots of user space expectations (which I know, is
> counter-intuitive), but changing it will open up a large can of worms, it works
> properly as it is.
>
> > I fix the comments part by below patch(thanks Oleg's comments), please
> > use attachment patch when merge.
> >
> > From dc7b02a1e0e413fb96d22f1d4ef4da98115cfb9d Mon Sep 17 00:00:00 2001
> > From: Jovi Zhang <bookjovi@gmail.com>
> > Date: Wed, 17 Aug 2011 15:34:29 +0800
> > Subject: [PATCH] coredump: fix wrong comments on core limits of pipe
> > coredump case
> >
> > In commit 898b374a, core limits recursive check vaule changed from 0 to 1,
> > but the corresponding comments was not changed correctly.
> >
> > Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Neil Horman <nhorman@tuxdriver.com>
> Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
>

Hi,
    How about this old patch? this patch still not upstreamed?
.jovi

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

[Jovi Zhang]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Hi Andrew,

Is that attached patch ok to go through your -mm tree? this patch
reviewed many months ago, but still not goto mainstream. :)

that comments is quite mismatch with the code.

Thanks.

.jovi

[-- Attachment #2: 0001-coredump-fix-wrong-comments-on-core-limits-of-pipe-c.patch --]
[-- Type: application/octet-stream, Size: 1761 bytes --]

From dc7b02a1e0e413fb96d22f1d4ef4da98115cfb9d Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@gmail.com>
Date: Wed, 17 Aug 2011 15:34:29 +0800
Subject: [PATCH] coredump: fix wrong comments on core limits of pipe coredump case

In commit 898b374a, core limits recursive check vaule changed from 0 to 1,
but the corresponding comments was not changed correctly.

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
---
 fs/exec.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 25dcbe5..ba493cc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -2158,15 +2158,16 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		}
 
 		if (cprm.limit == 1) {
-			/*
+			/* See umh_pipe_setup() which sets RLIMIT_CORE = 1.
+			 *
 			 * Normally core limits are irrelevant to pipes, since
 			 * we're not writing to the file system, but we use
-			 * cprm.limit of 1 here as a speacial value. Any
-			 * non-1 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 =  !1
-			 * but it runs as root, and can do lots of stupid things
+			 * cprm.limit of 1 here as a speacial value, this is a
+			 * consistent way to catch recursive crashes.
+			 * We can still crash if the core_pattern binary sets
+			 * RLIM_CORE = !1, 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
-- 
1.6.5.2