lkml.org 
[lkml]   [2007]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 0/4] coredump: core dump masking support v4
    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

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

    \
     
     \ /
      Last update: 2007-03-28 14:41    [W:0.033 / U:2.692 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site