LKML Archive on
help / color / mirror / Atom feed
From: "Kawai, Hidehiro" <>
To: Andrew Morton <>
Cc: "Kawai, Hidehiro" <>,
	kernel list <>,
	Pavel Machek <>, Robin Holt <>,
	David Howells <>,
	Alan Cox <>,
	Masami Hiramatsu <>,
	sugita <>,
	Satoshi OSHIMA <>,
	Hideo AOKI <>
Subject: Re: [PATCH 0/4] coredump: core dump masking support v4
Date: Wed, 28 Mar 2007 21:37:07 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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" <> 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

[PATCH 2/4] coredump: ELF: enable to omit anonymous shared memory

[PATCH 3/4] coredump: ELF-FDPIC: enable to omit anonymous shared memory

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

  reply	other threads:[~2007-03-28 12:38 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-03-02  4:41 Kawai, Hidehiro
2007-03-02  4:47 ` [PATCH 1/4] coredump: add an interface to control the core dump routine Kawai, Hidehiro
2007-03-02  9:34   ` Pavel Machek
2007-03-26 13:02     ` Kawai, Hidehiro
2007-03-29 10:49       ` Pavel Machek
2007-03-29 19:16       ` David Howells
2007-03-29 21:17         ` Andrew Morton
2007-03-30 10:29           ` Kawai, Hidehiro
2007-03-30 16:10             ` Andrew Morton
2007-03-31 13:03             ` David Howells
2007-03-02  4:49 ` [PATCH 2/4] coredump: ELF: enable to omit anonymous shared memory Kawai, Hidehiro
2007-03-02  4:50 ` [PATCH 3/4] coredump: ELF-FDPIC: " Kawai, Hidehiro
2007-03-02  4:51 ` [PATCH 4/4] coredump: documentation for proc entry Kawai, Hidehiro
2007-03-02  9:35   ` Pavel Machek
2007-03-20 11:11     ` Kawai, Hidehiro
2007-03-15 20:37 ` [PATCH 0/4] coredump: core dump masking support v4 Andrew Morton
2007-03-23 13:13   ` Kawai, Hidehiro
2007-03-28 12:37     ` Kawai, Hidehiro [this message]
2007-03-28 17:32       ` Andrew Morton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 0/4] coredump: core dump masking support v4' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).