Re: [PATCH 2/4] Update gcore_create_callback to better handle different states of mappings

Sergio Durigan Junior
2015-03-19T23:08:48Z

On Thursday, March 19 2015, Pedro Alves wrote:

> On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
>> This patch updates gdb/gcore.c's gcore_create_callback function and
>> changes its logic to improve the handling of different states of
>> memory mappings.
>>
>> One of the major modifications is that mappings marked as UNMODIFIED
>> are now completely ignored by the function (i.e., not even the segment
>> headers are created anymore). This is now possible because of the
>> introduction of the new UNKNOWN state (see below). We now know that
>> when the mapping is UNMODIFIED, it means that the user has either
>> chose to ignore it via /proc/PID/coredump_filter, or that it is marked
>> as VM_DONTDUMP (and therefore should not be dumped anyway). This is
>> what the Linux kernel does, too.
>>
>> The new UNKNOWN state is being used to identify situations when we
>> don't really know whether the mapping should be dumped or not. Based
>> on that, we run an existing heuristic responsible for deciding if we
>> should include the mapping's contents or not.
>>
>> One last thing worth mentioning is that this check:
>>
>> if (read == 0 && write == 0 && exec == 0
>> && modified_state == MEMORY_MAPPING_UNMODIFIED)
>>
>> has been simplified to:
>>
>> if (read == 0)
>>
>> This is because if the mapping has not 'read' permission set, it does
>> not make sense to include its contents in the corefile (GDB would
>> actually not be allowed to do that).
>
> I don't think this is true. GDB can certainly read memory of
> mappings that don't have the read permission bit set: ptrace
> bypasses the memory protections. Otherwise, how could gdb poke
> breakpoints instructions? AFAICS, it can even read memory mapped
> with no permissions at all:
>
> (top-gdb) info inferiors
> Num Description Executable
> * 1 process 24337 /home/pedro/gdb/mygit/build/gdb/gdb
> (top-gdb) shell cat /proc/24337/maps
> ...
> 3615fb4000-36161b3000 ---p 001b4000 fd:00 263789 /usr/lib64/libc-2.18.so
> ...
> (top-gdb) x/4b 0x3615fb4000
> 0x3615fb4000: 0xf6 0x9a 0xf7 0x15

Indeed, this is not true. I apologize for making this confusion.

>> Therefore, we just create a
>> segment header for it. The Linux kernel differs from GDB here because
>> it actually include the contents of this mapping in the corefile, but
>> this is because it can read its contents.
>>
>> Changes from v2:
>>
>> - Return immediately if modified_state == MEMORY_MAPPING_UNMODIFIED
>>
>> - Improve comments explaining the case above, and also the case when
>> "read == 0".
>>
>> gdb/ChangeLog:
>> 2015-03-18 Sergio Durigan Junior <sergiodj@redhat.com>
>> Jan Kratochvil <jan.kratochvil@redhat.com>
>> Oleg Nesterov <oleg@redhat.com>
>>
>> PR corefiles/16092
>> * gcore.c (gcore_create_callback): Update code to handle the case
>> when 'modified_state == MEMORY_MAPPING_UNMODIFIED'. Simplify
>> condition used to decide when to create only a segment header for
>> the mapping. Improve check to decide when to run a heuristic to
>> decide whether to dump the mapping's contents.
>> ---
>> gdb/gcore.c | 39 +++++++++++++++++++++++++--------------
>> 1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/gcore.c b/gdb/gcore.c
>> index 751ddac..8dfcc02 100644
>> --- a/gdb/gcore.c
>> +++ b/gdb/gcore.c
>> @@ -422,23 +422,34 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
>> asection *osec;
>> flagword flags = SEC_ALLOC | SEC_HAS_CONTENTS | SEC_LOAD;
>>
>> - /* If the memory segment has no permissions set, ignore it, otherwise
>> - when we later try to access it for read/write, we'll get an error
>> - or jam the kernel. */
>> - if (read == 0 && write == 0 && exec == 0
>> - && modified_state == MEMORY_MAPPING_UNMODIFIED)
>> + if (modified_state == MEMORY_MAPPING_UNMODIFIED)
>> {
>> - if (info_verbose)
>> - {
>> - fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n",
>> - plongest (size), paddress (target_gdbarch (), vaddr));
>> - }
>> -
>> + /* When the memory mapping is marked as unmodified, this means
>> + that it should not be included in the coredump file (either
>> + because it was marked as VM_DONTDUMP, or because the user
>> + explicitly chose to ignore it using the
>> + /proc/PID/coredump_filter mechanism).
>> +
>> + We could in theory create a section header for it (i.e., mark
>> + it as '~(SEC_LOAD | SEC_HAS_CONTENTS)', just like we do when
>> + the mapping does not have the 'read' permission set), but the
>> + Linux kernel itself ignores these mappings, and so do we. */
>> return 0;
>
> It really seems wrong to me to bake in Linuxisms into this generic
> function, shared with all supported configurations. As mentioned
> elsewhere, I think we should instead leave it up to the (Linux-specific)
> caller to simply not call this function if the mapping shouldn't
> be dumped at all.

I totally agree. As I did with the patch to add the new enum
memory_mapping_state, I am also withdrawing this patch. I will send a
new series soon.

Thanks,

--
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible