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