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

  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).  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(-)
  

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


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

>      }
> -
> -  if (write == 0 && modified_state == MEMORY_MAPPING_UNMODIFIED
> -      && !solib_keep_data_in_core (vaddr, size))
> +  else if (read == 0)
> +    {
> +      /* If the memory segment has no read permission set, then we have to
> +	 generate a segment header for it, but without contents (i.e.,
> +	 FileSiz = 0), otherwise when we later try to access it for
> +	 read/write, we'll get an error or jam the kernel.
> +
> +	 The Linux kernel differs from GDB in this point because it
> +	 can actually read this mapping, so it dumps this mapping's
> +	 contents in the corefile.  */
> +      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
> +    }
> +  else if (write == 0 && modified_state == MEMORY_MAPPING_UNKNOWN_STATE
> +	   && !solib_keep_data_in_core (vaddr, size))
>      {
>        /* See if this region of memory lies inside a known file on disk.
>  	 If so, we can avoid copying its contents by clearing SEC_LOAD.  */
> 

Thanks,
Pedro Alves
  

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,