Re: [PATCH 0/4] coredump: core dump masking support v4

["Kawai, Hidehiro" <hidehiro.kawai.ez@hitachi.com>]

Hi,
Thank you for your kind comments.
I'm sorry for my late reply. 

Andrew Morton wrote:

> On Fri, 02 Mar 2007 13:41:30 +0900
> "Kawai, Hidehiro" <hidehiro.kawai.ez@hitachi.com> wrote:
> 
>>This patch series is version 4 of the core dump masking feature,
>>which provides a per-process flag not to dump anonymous shared
>>memory segments.
> 
> First up, please convince us that this problem cannot be solved in
> userspace. 
> Note that we now support dumping core over a pipe to a
> userspace application which can perform filtering such as this (see
> Documentation/sysctl/kernel.txt:core_pattern).

I understand.  Thank you for your suggestion.  I'll reply about it in
another mail, but it may take a few days.

 
> Assuming that your argument is successful...
> 
> - The unpleasing trylock in proc_coredump_omit_anon_shared_write() is
>   there, I believe, to handle the case where a coredump is presently in
>   progress.  We don't want to change the filtering rule while the dump is
>   happening.
> 
>   What I suggest you do instead is to take a copy of
>   mm->coredump_omit_anon_shared into a local variable with one single read
>   per coredump.  Pass that local down into all the callees which need to
>   see it.  That way, no locking is needed.

Previous v3 patchset does what you suggest, and here are links to the
patches:

[PATCH 2/4] coredump: ELF: enable to omit anonymous shared memory
http://lkml.org/lkml/2007/2/16/156

[PATCH 3/4] coredump: ELF-FDPIC: enable to omit anonymous shared memory
http://lkml.org/lkml/2007/2/16/157

However, there was an opposite opinion.  To pass the flag status, I
added omit_anon_shared argument to elf_fdpic_dump_segments().  Then,
David pointed that the argument was unncecessary, because the function
also receives mm_struct *mm which includes coredump_omit_anon_shared.
But mm->coredump_omit_anon_shared can be changed while core dumping, and
it may causes the core file to be corrupted.  So in v4 patchset I used
r/w semaphore to prevent mm->coredump_omit_anon_shared from being changed.

If I add an addtional argument to elf_fdpic_dump_segments() again, I
have to explain it to David.  I'll tell him that removing mm argument
from the function will be a solution since it refers current->mm directly
and the mm argument is never used.

 
> - These games we're playing with the atomicity of the bitfields in the
>   mm_struct need to go away.
> 
>   First up, please prepare a standalone patch which removes
>   mm_struct.dumpable and adds `unsigned long mm_struct.flags'.  Include a
>   comment telling people that they must use atomic bitops (set_bit, clear_bit) on
>   mm_struct.flags.

OK.  I'll do it in the next version.


> - Finally, the code as you have it here is very specific to your specific
>   requirement: don't dump shared memory segments.
> 
>   But if we're going to implement in-kernel core-dump filtering of this
>   nature, we should design it extensibly, even if we don't actually
>   implement those extensions at this time.

I understood. Since I had done so initially, I'll revert it to.

 
>   Because other people might (reasonably) wish to omit anonymous memory,
>   or private mappings, or file-backed VMAs, or whatever.
> 
>   So maybe /proc/pid/coredump_omit_anon_shared should become
>   /proc/pid/core_dumpfilter, which is a carefully documented bitmask.

There are people who wish to dump VMAs which are not dumped by default.
Taking this into account, some bits of core_dumpfilter will be set by
default.  This means users have to be aware of the default bitmask
when they change the bitmask.  Perhaps changing the bitmask requires
3 steps:

  1. read the default bitmask
  2. change bits of the mask
  3. write it to the proc entry

So I think it is better if we provide /proc/pid/core_flags (default:
all bits are 0) instead of core_dumpfilter.  With this interface,
users who use only one bit of the bitmask (this will be a common case) 
just have to write 2^n to the proc entry.  It takes only one step:

 1. write a value to the proc entry

If we can implement at the same cost, core_flags will be better
because it is useful for users.  What would you think about that?


By the way, Robin Holt wrote as follows:

> Can you make this a little more transparent?  Having a magic bitmask does
> not seem like the best way to do stuff.  Could you maybe make a core_flags
> directory with a seperate file for each flag.  It could still map to a
> single field in the mm, but be broken out for the proc filesystem.

Do you think Robin's suggestion is acceptable?

Best regards,
-- 
Hidehiro Kawai
Hitachi, Ltd., Systems Development Laboratory