[ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm

[ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm

[Earl Chew]

Change corepattern %e and %E to use current->group_leader->comm instead of current->comm.

Multithreaded processes can use PR_SET_NAME (or the corresponding pthread_setname_np)
to configure the name of each thread.

A core dump can be triggered from any task in a group, and substituting current->comm
means that the name of the core file for a process can vary in surprising ways.

Using current->group_leader->comm makes the name of the core file more consistent
for a process when used with %e or %E.

Signed-off-by: Earl Chew <echew@ixiacom.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Oleg Nesterov <oleg@redhat.com>
--
--- fs/exec.c.orig	2011-08-28 21:16:01.000000000 -0700
+++ fs/exec.c	2011-09-01 09:58:42.032116998 -0700
@@ -1679,7 +1679,8 @@ static int cn_print_exe_file(struct core
 	exe_file = get_mm_exe_file(current->mm);
 	if (!exe_file) {
 		char *commstart = cn->corename + cn->used;
-		ret = cn_printf(cn, "%s (path unknown)", current->comm);
+		ret = cn_printf(cn, "%s (path unknown)",
+				current->group_leader->comm);
 		cn_escape(commstart);
 		return ret;
 	}
@@ -1780,7 +1781,8 @@ static int format_corename(struct core_n
 			/* executable */
 			case 'e': {
 				char *commstart = cn->corename + cn->used;
-				err = cn_printf(cn, "%s", current->comm);
+				err = cn_printf(cn, "%s",
+					      current->group_leader->comm);
 				cn_escape(commstart);
 				break;
 			}

Re: [ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm

[Alan Cox]

On Thu, 1 Sep 2011 10:01:39 -0700
Earl Chew <echew@ixiacom.com> wrote:

> Change corepattern %e and %E to use current->group_leader->comm instead of current->comm.

Which might break stuff.

> A core dump can be triggered from any task in a group, and substituting current->comm
> means that the name of the core file for a process can vary in surprising ways.

Which some people may well be using

> Using current->group_leader->comm makes the name of the core file more consistent
> for a process when used with %e or %E.

In your view, but there is a better way to do this - add a new case and
letter for the behaviour you want. That way you don't break anyone elses
defaults and expectation and people can set a corepattern dependant upon
the group leader.

Alan

[PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm

[Earl Chew]

> In your view, but there is a better way to do this - add a new case and
> letter for the behaviour you want. That way you don't break anyone elses
> defaults and expectation and people can set a corepattern dependant upon
> the group leader.

Ok.


The patterns %n or %N are the same as %e and %E except that they 
use current->group_leader->comm instead of current->comm.


Signed-off-by: Earl Chew <echew@ixiacom.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Oleg Nesterov <oleg@redhat.com>
--
--- fs/exec.c.orig	2011-08-28 21:16:01.000000000 -0700
+++ fs/exec.c	2011-09-01 12:03:50.622059092 -0700
@@ -1670,7 +1670,7 @@ static void cn_escape(char *str)
 			*str = '!';
 }
 
-static int cn_print_exe_file(struct core_name *cn)
+static int cn_print_exe_file(struct core_name *cn, const char *comm)
 {
 	struct file *exe_file;
 	char *pathbuf, *path;
@@ -1679,7 +1679,7 @@ static int cn_print_exe_file(struct core
 	exe_file = get_mm_exe_file(current->mm);
 	if (!exe_file) {
 		char *commstart = cn->corename + cn->used;
-		ret = cn_printf(cn, "%s (path unknown)", current->comm);
+		ret = cn_printf(cn, "%s (path unknown)", comm);
 		cn_escape(commstart);
 		return ret;
 	}
@@ -1777,6 +1777,18 @@ static int format_corename(struct core_n
 				cn_escape(namestart);
 				break;
 			}
+			/* task group executable */
+			case 'n': {
+				char *commstart = cn->corename + cn->used;
+				err = cn_printf(cn, "%s",
+					      current->group_leader->comm);
+				cn_escape(commstart);
+				break;
+			}
+			case 'N':
+				err = cn_print_exe_file(cn,
+					      current->group_leader->comm);
+				break;
 			/* executable */
 			case 'e': {
 				char *commstart = cn->corename + cn->used;
@@ -1785,7 +1797,7 @@ static int format_corename(struct core_n
 				break;
 			}
 			case 'E':
-				err = cn_print_exe_file(cn);
+				err = cn_print_exe_file(cn, current->comm);
 				break;
 			/* core limit size */
 			case 'c':

Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm

[Oleg Nesterov]

On 09/01, Earl Chew wrote:
>
> > In your view, but there is a better way to do this - add a new case and
> > letter for the behaviour you want. That way you don't break anyone elses
> > defaults and expectation and people can set a corepattern dependant upon
> > the group leader.
>
> Ok.
>
>
> The patterns %n or %N are the same as %e and %E except that they
> use current->group_leader->comm instead of current->comm.

I simply do not know what is better. Alan has a point imho, "might
break stuff" is true.

OTOH, %p always reports tgid, not tid...

But in fact I do not understand the "Using current->group_leader->comm
makes the name of the core file more consistent" part. Why ?

> A core dump can be triggered from any task in a group,

Indeed. The important case is the private/synchronous signals like
SIGSEGV, you can see the name of the thread which triggered the crash.


> -static int cn_print_exe_file(struct core_name *cn)
> +static int cn_print_exe_file(struct core_name *cn, const char *comm)
>  {
>  	struct file *exe_file;
>  	char *pathbuf, *path;
> @@ -1679,7 +1679,7 @@ static int cn_print_exe_file(struct core
>  	exe_file = get_mm_exe_file(current->mm);
>  	if (!exe_file) {
>  		char *commstart = cn->corename + cn->used;
> -		ret = cn_printf(cn, "%s (path unknown)", current->comm);
> +		ret = cn_printf(cn, "%s (path unknown)", comm);

Imho, this is overkill. This is only used if get_mm_exe_file() fails,
I don't think this deserves another option. And may be we can use
group_leader->comm, this is per-process thing anyway.

But I won't insist, I agree either way.

Oleg.

Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm

[Earl Chew]

Oleg,

>> The patterns %n or %N are the same as %e and %E except that they
>> use current->group_leader->comm instead of current->comm.
> 
> I simply do not know what is better. Alan has a point imho, "might
> break stuff" is true.
> 
> OTOH, %p always reports tgid, not tid...

Which speaks partly to my notion of "consistency".

> But in fact I do not understand the "Using current->group_leader->comm
> makes the name of the core file more consistent" part. Why ?

Internals aside, "%e" is advertised, rightly or wrongly as:

http://lxr.linux.no/#linux+v3.0.4/Documentation/sysctl/kernel.txt

        %e      executable filename (may be shortened)

Using current->comm has the following issues with respect to
the documentation :

Issue 1. The executable filename has the attribute that it is
         the same for all threads in one process, while current->comm
         does not.

Issue 2. Even for the group leader current->comm is not guaranteed to
         be the executable filename at all (the new %E yields that).

I viewed my original change as more "consistent" because it
yielded the attribute alluded to in the documentation --- the
same value for all threads in the one process:

	- Consistent with the documentation
	- Consistent with respect to process name (as opposed to thread name)

>> A core dump can be triggered from any task in a group,
> 
> Indeed. The important case is the private/synchronous signals like
> SIGSEGV, you can see the name of the thread which triggered the crash.

While that is true, it doesn't seem to have been the original intent as
per the %e documentation.

> Imho, this is overkill. This is only used if get_mm_exe_file() fails,
> I don't think this deserves another option. And may be we can use
> group_leader->comm, this is per-process thing anyway.
> 
> But I won't insist, I agree either way.



Fundamentally, which do you consider "broken" ?

	o fs/exec.c using current->comm for %e
		or
	o Documentation/sysctl/kernel.txt


I suspect we'll fall on the side of keeping "broken behaviour" since
it affects existing code, and instead fix the documentation.


Should get_mm_exe_file() just use current->group_leader->comm since it's
meant to be process specific anyway, and there isn't an existing code base
for %E ?


Evidently I need to patch Documentation/sysctl/kernel.txt too.


Earl

Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm

[Oleg Nesterov]

On 09/02, Earl Chew wrote:
>
> Oleg,
>
> >> The patterns %n or %N are the same as %e and %E except that they
> >> use current->group_leader->comm instead of current->comm.
> >
> > I simply do not know what is better. Alan has a point imho, "might
> > break stuff" is true.
> >
> > OTOH, %p always reports tgid, not tid...
>
> Which speaks partly to my notion of "consistency".

That is why I mentioned it with "otoh" ;)

> I viewed my original change as more "consistent" because it
> yielded the attribute alluded to in the documentation --- the
> same value for all threads in the one process:
>
> 	- Consistent with the documentation
> 	- Consistent with respect to process name (as opposed to thread name)
>
> >> A core dump can be triggered from any task in a group,
> >
> > Indeed. The important case is the private/synchronous signals like
> > SIGSEGV, you can see the name of the thread which triggered the crash.
>
> While that is true, it doesn't seem to have been the original intent as
> per the %e documentation.

May be. May be not. I do not know.

> Should get_mm_exe_file() just use current->group_leader->comm since it's
> meant to be process specific anyway,

Probably. Although group_leader->comm is thread specific too, but
at least we do not use the "random" thread. My only point was, imho
this doesn't deserve another option.

> and there isn't an existing code base
> for %E ?

Who knows? But once again, we use ->comm in the very unlikely case.



And let me repeat just in case. I do not argue, I agree either way.

Oleg.

Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm

[Earl Chew]

Oleg, Alan,


> Who knows? But once again, we use ->comm in the very unlikely case.
>
> And let me repeat just in case. I do not argue, I agree either way.

I understand.

Before I give thought to reworking the change, perhaps some other
considerations.


The other interesting thing is that Alan brought up the relevant point
about stuff breaking.

Alan Cox wrote:
> Earl wrote:
>> Change corepattern %e and %E to use current->group_leader->comm instead of current->comm.
>
> Which might break stuff.


What are your thoughts regarding the introduction of cn_escape() in this patch ?

https://lkml.org/lkml/2011/6/7/292


While this change also has merit, it is also breaking change. There might
be code relying on %e containing embedded slashes, either directly in
core pattern:

	%e.core

or via the pipe:

	| /opt/corehandler '%e'	



Isn't the interface change introduced by cn_escape() also a concern ?


Earl