linux-kernel - Re: [PATCH 1/1 v2]: coredump: use current->group

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4E610DAE.4080607@ixiacom.com>
Date:	Fri, 2 Sep 2011 10:09:02 -0700
From:	Earl Chew <echew@...acom.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Alan Cox <alan@...rguk.ukuu.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
	"andi@...stfloor.org" <andi@...stfloor.org>
Subject: Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead
 of current->comm

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/