Improve corefile generation by using /proc/PID/coredump_filter (PR corefile/16902)

  On Thursday, March 05 2015, Jan Kratochvil wrote:

> On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
>>   bit 0  Dump anonymous private mappings.
>>   bit 1  Dump anonymous shared mappings.
>>   bit 2  Dump file-backed private mappings.
>>   bit 3  Dump file-backed shared mappings.
>>   bit 4 (since Linux 2.6.24)
>>          Dump ELF headers.
>>   bit 5 (since Linux 2.6.28)
>>          Dump private huge pages.
>>   bit 6 (since Linux 2.6.28)
>>          Dump shared huge pages.
> [...]
>> The default value for this file, used by the Linux kernel, is 0x33,
>> which means that bits 0, 1 and 4 are enabled.  This is also the default
>
> and 5

And 5.  Thanks.

>> for GDB implemented in this patch, FWIW.
> [...]
>> With Oleg's help, we could improve the current algorithm for determining
>> whether a memory mapping is anonymous/file-backed, private/shared.  GDB
>> now also respects the MADV_DONTDUMP flag and does not dump the memory
>
> s/does not dump/does dump/

No, it doesn't dump.  MADV_DONTDUMP activates the "dd" flag in VmFlags,
and the patch looks for it and, if it finds the flag, it doesn't mark
the memory mapping to be dumped.  However, GDB will create the section
header in the corefile.

>> mapping marked as so, and won't try to dump "[vsyscall]" or "[vdso]"
>> mappings as before (just like the Linux kernel).
>
> Currently it also tries to dump [vvar] (by default rules) but that is
> unreadable for some reason, causing:
> warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
>                                                                 ^^^^^^^^^^^^^^
> Saved corefile /tmp/1j
> (gdb) _
> # grep 7ffff6ceb000 /proc/$p/maps
> 7ffff6ceb000-7ffff6ced000 r--p 00000000 00:00 0                          [vvar]
> ^^^^^^^^^^^^                                                              ^^^^
>
> I do not know what [vvar] is good for and why it cannot be read.

I totally forgot about this, even though we discussed it before.  Sorry;
I am sending a new version of the patch which addresses this issue.

>>   It is worth mentioning that, from all those checks described above,
>>   the most fragile is the one to see if the file name ends with "
>>   (deleted)".  This does not necessarily mean that the mapping is
>>   anonymous, because the deleted file associated with the mapping may
>>   have been a hard link to another file, for example.  The Linux kernel
>>   checks to see if "i_nlink == 0", but GDB cannot easily do this check.
>
> # stat /proc/21604/map_files/400000-4ec000 
>   File: ‘/proc/21604/map_files/400000-4ec000’ -> ‘/tmp/bash-deleted’
>   Size: 64        	Blocks: 0          IO Block: 1024   symbolic link
> Device: 3h/3d	Inode: 1554082     Links: 1
> # stat -L /proc/21604/map_files/400000-4ec000 
>   File: ‘/proc/21604/map_files/400000-4ec000’
>   Size: 1051464   	Blocks: 2056       IO Block: 4096   regular file
> Device: fd01h/64769d	Inode: 5509691     Links: 1
> # rm /tmp/bash-deleted
> # stat -L /proc/21604/map_files/400000-4ec000 
>   File: ‘/proc/21604/map_files/400000-4ec000’
>   Size: 1051464   	Blocks: 2056       IO Block: 4096   regular file
> Device: fd01h/64769d	Inode: 5509691     Links: 0
>                                                   ^
>
> One could find if i_nlink == 0 if it would be enough.  But it would work only
> if GDB runs as root so it is probably not worth coding it:
>
> $ ls -ld /proc/3803/map_files
> dr-x------ 2 lace lace 0 Mar  5 16:44 /proc/3803/map_files/
> $ stat /proc/3803/map_files/400000-4ec000
> stat: cannot stat ‘/proc/3803/map_files/400000-4ec000’: Operation not permitted

Yeah, but it would still be much easier if this information were present
in the smaps file directly.

Here's the updated patch that filters out the [vvar] mapping.

Thanks,
  

On Thu, 05 Mar 2015 21:52:56 +0100, Sergio Durigan Junior wrote:
> On Thursday, March 05 2015, Jan Kratochvil wrote:
> > On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
> >> With Oleg's help, we could improve the current algorithm for determining
> >> whether a memory mapping is anonymous/file-backed, private/shared.  GDB
> >> now also respects the MADV_DONTDUMP flag and does not dump the memory
> >
> > s/does not dump/does dump/
> 
> No, it doesn't dump.  MADV_DONTDUMP activates the "dd" flag in VmFlags,
> and the patch looks for it and, if it finds the flag, it doesn't mark
> the memory mapping to be dumped.  However, GDB will create the section
> header in the corefile.

Sorry, I meesed it up even more.  For MADV_DONTDUMP you are right, FSF GDB
dumps MADV_DONTDUMP memory, kernel does not and with this patch GDB will not.

What I wanted to say was:


> >> mapping marked as so, and won't try to dump "[vsyscall]" or "[vdso]"

s/won't try/will try/

this one.


> >> mappings as before (just like the Linux kernel).
> >
> > Currently it also tries to dump [vvar] (by default rules) but that is
> > unreadable for some reason, causing:
> > warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
> >                                                                 ^^^^^^^^^^^^^^
> > Saved corefile /tmp/1j
> > (gdb) _
> > # grep 7ffff6ceb000 /proc/$p/maps
> > 7ffff6ceb000-7ffff6ced000 r--p 00000000 00:00 0                          [vvar]
> > ^^^^^^^^^^^^                                                              ^^^^
> >
> > I do not know what [vvar] is good for and why it cannot be read.
> 
> I totally forgot about this, even though we discussed it before.  Sorry;
> I am sending a new version of the patch which addresses this issue.

It would be good to get a reply from a kernel aware person what does it mean
before such patch gets accepted.  It can be also just a Linux kernel bug.


Jan
  

On 03/05, Jan Kratochvil wrote:
>
> On Thu, 05 Mar 2015 21:52:56 +0100, Sergio Durigan Junior wrote:
> > On Thursday, March 05 2015, Jan Kratochvil wrote:
> > > On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
> > > Currently it also tries to dump [vvar] (by default rules) but that is
> > > unreadable for some reason, causing:
> > > warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
> > >                                                                 ^^^^^^^^^^^^^^
> > > Saved corefile /tmp/1j
> > > (gdb) _
> > > # grep 7ffff6ceb000 /proc/$p/maps
> > > 7ffff6ceb000-7ffff6ced000 r--p 00000000 00:00 0                          [vvar]
> > > ^^^^^^^^^^^^                                                              ^^^^
> > >
> > > I do not know what [vvar] is good for and why it cannot be read.

Well, I am not sure I understand this new mapping correctly. I need to
recheck.

But apparently it represents the kernel data (say, gtod) which vdso code
(running in user mode)  can read.

Probably gdb doesn't need to dump this vma, but see below.

> It would be good to get a reply from a kernel aware person what does it mean
> before such patch gets accepted.  It can be also just a Linux kernel bug.

_So far_ this doesn't look like a kernel bug to me.

I guess it fails because of

	struct page *no_pages[] = {NULL};
	struct vm_special_mapping vvar_mapping = {
		.name = "[vvar]",
		.pages = no_pages,
	};

so get_user_pages() -> special_mapping_fault() can't succeed, there is
no page it could return.

And the code above looks as if we deny the access on purpose. Probably
this makes sense, this section can contain the "sensitive" data, say,
hpet timer's io memory...

But! I need to recheck. In fact, it seems to me that I should discuss
this on lkml. I have some concerns, but most probably this is only my
misunderstanding, I need to read this (new to me) code more carefully.

Oleg.
  

On Wednesday, March 11 2015, Oleg Nesterov wrote:

> On 03/05, Jan Kratochvil wrote:
>>
>> On Thu, 05 Mar 2015 21:52:56 +0100, Sergio Durigan Junior wrote:
>> > On Thursday, March 05 2015, Jan Kratochvil wrote:
>> > > On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
>> > > Currently it also tries to dump [vvar] (by default rules) but that is
>> > > unreadable for some reason, causing:
>> > > warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
>> > >                                                                 ^^^^^^^^^^^^^^
>> > > Saved corefile /tmp/1j
>> > > (gdb) _
>> > > # grep 7ffff6ceb000 /proc/$p/maps
>> > > 7ffff6ceb000-7ffff6ced000 r--p 00000000 00:00 0                          [vvar]
>> > > ^^^^^^^^^^^^                                                              ^^^^
>> > >
>> > > I do not know what [vvar] is good for and why it cannot be read.
>
> Well, I am not sure I understand this new mapping correctly. I need to
> recheck.
>
> But apparently it represents the kernel data (say, gtod) which vdso code
> (running in user mode)  can read.
>
> Probably gdb doesn't need to dump this vma, but see below.

Right.  As far as I can see this was not being dumped in the previous
code, too.  I did not check whether the Linux kernel dumps this or not.

>> It would be good to get a reply from a kernel aware person what does it mean
>> before such patch gets accepted.  It can be also just a Linux kernel bug.
>
> _So far_ this doesn't look like a kernel bug to me.
>
> I guess it fails because of
>
> 	struct page *no_pages[] = {NULL};
> 	struct vm_special_mapping vvar_mapping = {
> 		.name = "[vvar]",
> 		.pages = no_pages,
> 	};
>
> so get_user_pages() -> special_mapping_fault() can't succeed, there is
> no page it could return.
>
> And the code above looks as if we deny the access on purpose. Probably
> this makes sense, this section can contain the "sensitive" data, say,
> hpet timer's io memory...
>
> But! I need to recheck. In fact, it seems to me that I should discuss
> this on lkml. I have some concerns, but most probably this is only my
> misunderstanding, I need to read this (new to me) code more carefully.

Thanks, Oleg.

For now, I will keep discarding this mapping in the dumping.  But please
let us know about your findings.

Meanwhile, I'll keep pinging this patch for reviews here.
  

Add cc's, change subject.

On 03/11, Oleg Nesterov wrote:
>
> On 03/05, Jan Kratochvil wrote:
> >
> > On Thu, 05 Mar 2015 21:52:56 +0100, Sergio Durigan Junior wrote:
> > > On Thursday, March 05 2015, Jan Kratochvil wrote:
> > > > On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
> > > > Currently it also tries to dump [vvar] (by default rules) but that is
> > > > unreadable for some reason, causing:
> > > > warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
> > > >                                                                 ^^^^^^^^^^^^^^
>
> > It would be good to get a reply from a kernel aware person what does it mean
> > before such patch gets accepted.  It can be also just a Linux kernel bug.
>
> _So far_ this doesn't look like a kernel bug to me.
>
> But! I need to recheck. In fact, it seems to me that I should discuss
> this on lkml. I have some concerns, but most probably this is only my
> misunderstanding, I need to read this (new to me) code more carefully.

Hi Andy, we need your help ;)

So, the problem is that gdb can't access the "vvar" mapping which looks
like the "normal" vma from user-space pov.

Technically this is clear. vvar_mapping->pages is the "dummy" no_pages[]
array, get_user_pages() can't succeed. In fact even follow_page() can't
work because of VM_PFNMAP/_PAGE_SPECIAL set by remap_pfn_range().

What is not clear: do we really want gup() to fail? Or it is not trivial
to turn __vvar_page into the "normal" page? (to simplify the discussion,
lets ignore hpet mapping for now).

Because this doesn't look consistent. gdb tries to "coredump" the live
process like the kernel does, but fails to dump the "r--p ... [vvar]"
region.


OK, gdb can look at VM_DONTDUMP bit in "VmFlags:" field in /proc/pid/smaps
and skip this vma. But, why (afaics) the kernel dumps this vma then? Lets
look at vma_dump_size(),

	/* always dump the vdso and vsyscall sections */
	if (always_dump_vma(vma))
		goto whole;

	if (vma->vm_flags & VM_DONTDUMP)
		return 0;

so the kernel ignores VM_DONTDUMP in this case, always_dump_vma() returns
true because of special_mapping_name(). Perhaps we should check VM_DONTDUMP
before always_dump_vma() ?


Or. We can teach gdb to read and dump its own "vvar" mapping to mimic the
kernel behaviour, this is the same read-only memory. But this hack doesn't
look nice, gdb should not know "too much" about the kernel internals.

Oleg.
  

See another email I sent, perhaps this needs more discussion...

But,

On 03/11, Oleg Nesterov wrote:
>
> On 03/05, Jan Kratochvil wrote:
> >
> > On Thu, 05 Mar 2015 21:52:56 +0100, Sergio Durigan Junior wrote:
> > > On Thursday, March 05 2015, Jan Kratochvil wrote:
> > > > On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
> > > > Currently it also tries to dump [vvar] (by default rules) but that is
> > > > unreadable for some reason, causing:
> > > > warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
> > > >                                                                 ^^^^^^^^^^^^^^
> > > > Saved corefile /tmp/1j
> > > > (gdb) _
> > > > # grep 7ffff6ceb000 /proc/$p/maps
> > > > 7ffff6ceb000-7ffff6ced000 r--p 00000000 00:00 0                          [vvar]
> > > > ^^^^^^^^^^^^                                                              ^^^^
> > > >
> > > > I do not know what [vvar] is good for and why it cannot be read.
>
> Probably gdb doesn't need to dump this vma, but see below.

Probably yes. Note that it has VM_DONTDUMP ("dd" in "VmFlags:" field).

However. If (for any reason) you decide to dump this region, gdb can
look into /proc/self/maps, find its own "vvar" mapping, and simply read
this memory. Unlike "vdso", "vvar" has the same content for every process.

(just in case, "vdso" is the same too but it is MAYWRITE, so it can have
 anonymous pages. Say, breakpoints installed by gdb).

Oleg.
  

On 03/12/2015 03:00 PM, Oleg Nesterov wrote:

> However. If (for any reason) you decide to dump this region, gdb can
> look into /proc/self/maps, find its own "vvar" mapping, and simply read
> this memory. Unlike "vdso", "vvar" has the same content for every process.

Actually it can't: GDB may well be dumping the memory of
a process running on another machine (through gdbserver).

Thanks,
Pedro Alves
  

On Thu, 12 Mar 2015 16:45:15 +0100, Pedro Alves wrote:
> On 03/12/2015 03:00 PM, Oleg Nesterov wrote:
> 
> > However. If (for any reason) you decide to dump this region, gdb can
> > look into /proc/self/maps, find its own "vvar" mapping, and simply read
> > this memory. Unlike "vdso", "vvar" has the same content for every process.
> 
> Actually it can't: GDB may well be dumping the memory of
> a process running on another machine (through gdbserver).

So it can - from gdbserver's [vvar].


Jan
  

On 03/12, Pedro Alves wrote:
>
> On 03/12/2015 03:00 PM, Oleg Nesterov wrote:
>
> > However. If (for any reason) you decide to dump this region, gdb can
> > look into /proc/self/maps, find its own "vvar" mapping, and simply read
> > this memory. Unlike "vdso", "vvar" has the same content for every process.
>
> Actually it can't: GDB may well be dumping the memory of
> a process running on another machine (through gdbserver).

Yes, thanks for correcting me...

I do not know if gdb can ask gdbserver to read its own memory, but even if
it can this doesn't look like a nice solution.

Just curious... I know that gdb can execute the code on behalf of the traced
process, so perhaps it can force the tracee to memcpy() its "vvar" memory.
Can this work with gdbserver? Again, I do not think this hack can make any
sense. I am just curious.

At least (I hope) this mapping doesn't look "important" from debugging pov,
perhaps gdb should ignore it. Lets see what Andy thinks, but I bet it is
very unlikely that the kernel will be changed to allow the access to this
vma.

Oleg.
  

On 03/12/2015 03:57 PM, Jan Kratochvil wrote:
> On Thu, 12 Mar 2015 16:45:15 +0100, Pedro Alves wrote:
>> On 03/12/2015 03:00 PM, Oleg Nesterov wrote:
>>
>>> However. If (for any reason) you decide to dump this region, gdb can
>>> look into /proc/self/maps, find its own "vvar" mapping, and simply read
>>> this memory. Unlike "vdso", "vvar" has the same content for every process.
>>
>> Actually it can't: GDB may well be dumping the memory of
>> a process running on another machine (through gdbserver).
> 
> So it can - from gdbserver's [vvar].

Sure, but GDB is just remotely reading the /proc files.
We'd need a new RSP packet to get at that object.  All
for working around something that sounds like the kernel
should be supporting without hacks.

Thanks,
Pedro Alves
  

On 03/12/2015 04:05 PM, Oleg Nesterov wrote:
> On 03/12, Pedro Alves wrote:
>>
>> On 03/12/2015 03:00 PM, Oleg Nesterov wrote:
>>
>>> However. If (for any reason) you decide to dump this region, gdb can
>>> look into /proc/self/maps, find its own "vvar" mapping, and simply read
>>> this memory. Unlike "vdso", "vvar" has the same content for every process.
>>
>> Actually it can't: GDB may well be dumping the memory of
>> a process running on another machine (through gdbserver).
> 
> Yes, thanks for correcting me...
> 
> I do not know if gdb can ask gdbserver to read its own memory, but even if
> it can this doesn't look like a nice solution.

Not currently, it can't.

> 
> Just curious... I know that gdb can execute the code on behalf of the traced
> process, so perhaps it can force the tracee to memcpy() its "vvar" memory.
> Can this work with gdbserver? Again, I do not think this hack can make any
> sense. I am just curious.

Yes, that can work.  But it's horrible.  :-)  If the user is dumping the
process's core, it's likely because the traced process is already in a
not-so-good / corrupted state.  Forcing it to run more code may make
things worse.

> At least (I hope) this mapping doesn't look "important" from debugging pov,
> perhaps gdb should ignore it. Lets see what Andy thinks, 

Agreed, let's hear what Andy says.

> but I bet it is
> very unlikely that the kernel will be changed to allow the access to this
> vma.

Thanks,
Pedro Alves
  

On Thu, Mar 12, 2015 at 7:34 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Add cc's, change subject.
>
> On 03/11, Oleg Nesterov wrote:
>>
>> On 03/05, Jan Kratochvil wrote:
>> >
>> > On Thu, 05 Mar 2015 21:52:56 +0100, Sergio Durigan Junior wrote:
>> > > On Thursday, March 05 2015, Jan Kratochvil wrote:
>> > > > On Thu, 05 Mar 2015 04:48:09 +0100, Sergio Durigan Junior wrote:
>> > > > Currently it also tries to dump [vvar] (by default rules) but that is
>> > > > unreadable for some reason, causing:
>> > > > warning: Memory read failed for corefile section, 8192 bytes at 0x7ffff6ceb000.
>> > > >                                                                 ^^^^^^^^^^^^^^
>>
>> > It would be good to get a reply from a kernel aware person what does it mean
>> > before such patch gets accepted.  It can be also just a Linux kernel bug.
>>
>> _So far_ this doesn't look like a kernel bug to me.
>>
>> But! I need to recheck. In fact, it seems to me that I should discuss
>> this on lkml. I have some concerns, but most probably this is only my
>> misunderstanding, I need to read this (new to me) code more carefully.
>
> Hi Andy, we need your help ;)
>
> So, the problem is that gdb can't access the "vvar" mapping which looks
> like the "normal" vma from user-space pov.
>
> Technically this is clear. vvar_mapping->pages is the "dummy" no_pages[]
> array, get_user_pages() can't succeed. In fact even follow_page() can't
> work because of VM_PFNMAP/_PAGE_SPECIAL set by remap_pfn_range().
>
> What is not clear: do we really want gup() to fail? Or it is not trivial
> to turn __vvar_page into the "normal" page? (to simplify the discussion,
> lets ignore hpet mapping for now).

We could presumably fiddle with the vma to allow get_user_pages to
work on at least the first vvar page.  There are some decently large
caveats, though:

 - We don't want to COW it.  If someone pokes at that page with
ptrace, for example, and it gets COWed, everything will stop working
because the offending process will no longer see updates.  That way
lies infinite loops.

 - The implementation could be odd.  The vma is either VM_MIXEDMAP or
VM_PFNMAP, and I don't see any practical way to change that.

 - The HPET and perhaps pvclock stuff.  The HPET probably doesn't have
a struct page at all, so you can't possibly get_user_pages it.

>
> Because this doesn't look consistent. gdb tries to "coredump" the live
> process like the kernel does, but fails to dump the "r--p ... [vvar]"
> region.
>
>
> OK, gdb can look at VM_DONTDUMP bit in "VmFlags:" field in /proc/pid/smaps
> and skip this vma. But, why (afaics) the kernel dumps this vma then? Lets
> look at vma_dump_size(),
>
>         /* always dump the vdso and vsyscall sections */
>         if (always_dump_vma(vma))
>                 goto whole;
>
>         if (vma->vm_flags & VM_DONTDUMP)
>                 return 0;
>
> so the kernel ignores VM_DONTDUMP in this case, always_dump_vma() returns
> true because of special_mapping_name(). Perhaps we should check VM_DONTDUMP
> before always_dump_vma() ?
>

That sounds reasonable to me.  I'll write the patch later today.  gdb
will still need changes, though, right?

--Andy

>
> Or. We can teach gdb to read and dump its own "vvar" mapping to mimic the
> kernel behaviour, this is the same read-only memory. But this hack doesn't
> look nice, gdb should not know "too much" about the kernel internals.
>
> Oleg.
>
  

On 03/12, Andy Lutomirski wrote:
>
> On Thu, Mar 12, 2015 at 7:34 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > What is not clear: do we really want gup() to fail? Or it is not trivial
> > to turn __vvar_page into the "normal" page? (to simplify the discussion,
> > lets ignore hpet mapping for now).
>
> We could presumably fiddle with the vma to allow get_user_pages to
> work on at least the first vvar page.  There are some decently large
> caveats, though:
>
>  - We don't want to COW it.  If someone pokes at that page with
> ptrace, for example, and it gets COWed, everything will stop working
> because the offending process will no longer see updates.  That way
> lies infinite loops.

Of course, but this looks simple... is_cow_mapping() == F so FOLL_FORCE
won't work anyway?

>  - The implementation could be odd.  The vma is either VM_MIXEDMAP or
> VM_PFNMAP, and I don't see any practical way to change that.
>
>  - The HPET and perhaps pvclock stuff.  The HPET probably doesn't have
> a struct page at all, so you can't possibly get_user_pages it.

Yes, this is true. OK, lets not dump it. I'll probably send a patch which
changes vma_dump_size() to check VM_DONTDUMP first...

But this leads to another question: why do we want to expose this
"vvar" vma at all?

For the moment, forget about compat 32-bit applications running under
64-bit kernel.

Can't we simply add FIX_VVAR_PAGE into fixed_addresses{}, map it into
init_mm via set_fixmap(FIX_VVAR_PAGE, __PAGE_USER) and change __vdso.*
functions to use fix_to_virt() address?

I don't really understand the low-level details, I'd like to understand
if this can work or not. And if it can work, why this is undesirable.

As for 32-bit applications. Yes, this can't work because 32-bit simply
can't access this "high" memory. But you know, it would be very nice to
have the fixmap-like "global" area in init_mm which is also visible to
compat applications. If we had it, uprobes could work without xol vma's.

Oleg.
  

On Thu, Mar 12, 2015 at 9:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/12, Andy Lutomirski wrote:
>>
>> On Thu, Mar 12, 2015 at 7:34 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > What is not clear: do we really want gup() to fail? Or it is not trivial
>> > to turn __vvar_page into the "normal" page? (to simplify the discussion,
>> > lets ignore hpet mapping for now).
>>
>> We could presumably fiddle with the vma to allow get_user_pages to
>> work on at least the first vvar page.  There are some decently large
>> caveats, though:
>>
>>  - We don't want to COW it.  If someone pokes at that page with
>> ptrace, for example, and it gets COWed, everything will stop working
>> because the offending process will no longer see updates.  That way
>> lies infinite loops.
>
> Of course, but this looks simple... is_cow_mapping() == F so FOLL_FORCE
> won't work anyway?
>
>>  - The implementation could be odd.  The vma is either VM_MIXEDMAP or
>> VM_PFNMAP, and I don't see any practical way to change that.
>>
>>  - The HPET and perhaps pvclock stuff.  The HPET probably doesn't have
>> a struct page at all, so you can't possibly get_user_pages it.
>
> Yes, this is true. OK, lets not dump it. I'll probably send a patch which
> changes vma_dump_size() to check VM_DONTDUMP first...
>
> But this leads to another question: why do we want to expose this
> "vvar" vma at all?
>
> For the moment, forget about compat 32-bit applications running under
> 64-bit kernel.
>
> Can't we simply add FIX_VVAR_PAGE into fixed_addresses{}, map it into
> init_mm via set_fixmap(FIX_VVAR_PAGE, __PAGE_USER) and change __vdso.*
> functions to use fix_to_virt() address?
>
> I don't really understand the low-level details, I'd like to understand
> if this can work or not. And if it can work, why this is undesirable.
>
> As for 32-bit applications. Yes, this can't work because 32-bit simply
> can't access this "high" memory. But you know, it would be very nice to
> have the fixmap-like "global" area in init_mm which is also visible to
> compat applications. If we had it, uprobes could work without xol vma's.
>

It could work for 32-bit native, but not for 32-bit compat.  Also, I
have grand plans to add per-task vvar overrides for seccomp and such.
And RIP-relative addressing is a bit nicer than absolute :)

It used to work that way, but we changed it in 3.15 IIRC.

On a related note, I'm hoping to rework the mm part pretty heavily:

http://lkml.kernel.org/r/cover.1414629045.git.luto@amacapital.net

--Andy

> Oleg.
>
  

On Thursday, March 12 2015, Oleg Nesterov wrote:

>> Probably gdb doesn't need to dump this vma, but see below.
>
> Probably yes. Note that it has VM_DONTDUMP ("dd" in "VmFlags:" field).

The fact that the region has VM_DONTDUMP is enough for GDB to ignore
it.  IMO, as discussed in the other thread with Andy, the Linux kernel
is bogus in this case and should also be ignoring this.

> However. If (for any reason) you decide to dump this region, gdb can
> look into /proc/self/maps, find its own "vvar" mapping, and simply read
> this memory. Unlike "vdso", "vvar" has the same content for every process.

Yeah, but I don't think this is worth the effort.  As Pedro mentioned,
things can get more complicated when we consider remote scenarios.

> (just in case, "vdso" is the same too but it is MAYWRITE, so it can have
>  anonymous pages. Say, breakpoints installed by gdb).

Also, [vdso] doesn't have the VM_DONTDUMP flag.  My patch is already
dumping it inconditionally.
  

On 03/12, Andy Lutomirski wrote:
>
> On Thu, Mar 12, 2015 at 9:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 03/12, Andy Lutomirski wrote:
> >>
> > As for 32-bit applications. Yes, this can't work because 32-bit simply
> > can't access this "high" memory. But you know, it would be very nice to
> > have the fixmap-like "global" area in init_mm which is also visible to
> > compat applications. If we had it, uprobes could work without xol vma's.
> >
> It could work for 32-bit native, but not for 32-bit compat.

Yes, yes, I meant 32-bit compat apps. Once again, it would be nice if we
had the "low" fixmaps in init_mm. But unlikely this is possible...

> On a related note, I'm hoping to rework the mm part pretty heavily:
>
> http://lkml.kernel.org/r/cover.1414629045.git.luto@amacapital.net

OK... not that I really understand this email.

Well. Speaking of vdso. I understand that unlikely we can do this, but
for uprobes it would be nice to have a anon-inode file behind this mapping,
so that vma_interval_tree_foreach() could work, etc. OK, this is completely
off-topic, please forget.



And I noticed that I didn't read your previous email carefully enough...

> That sounds reasonable to me.  I'll write the patch later today.

Sure, please send a patch if you want to do this.

> gdb will still need changes, though, right?

This is up to gdb developers. To me, it should simply skip this
VM_DONTDUMP vma.

Oleg.
  

On Thursday, March 12 2015, Oleg Nesterov wrote:

>> gdb will still need changes, though, right?
>
> This is up to gdb developers. To me, it should simply skip this
> VM_DONTDUMP vma.

If I understood this discussion correctly (and thanks Andy and Oleg for,
*ahem*, dumping all this useful information for us!), GDB will not need
modifications in the Linux kernel in this area.  In fact, my patch
already implements the "ignore VM_DONTDUMP mappings" part, so we're
pretty much covered.

Thanks,
  

On 03/12, Oleg Nesterov wrote:
>
> Yes, this is true. OK, lets not dump it.

OTOH. We can probably add ->access() into special_mapping_vmops, this
way __access_remote_vm() could work even if gup() fails ?

Jan, Sergio. How much do we want do dump this area ? The change above
should be justified.

Oleg.
  

On Thu, Mar 12, 2015 at 10:46 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/12, Oleg Nesterov wrote:
>>
>> Yes, this is true. OK, lets not dump it.
>
> OTOH. We can probably add ->access() into special_mapping_vmops, this
> way __access_remote_vm() could work even if gup() fails ?

Let's wait until my special_mapping vmops rework lands to do that.
I'll dust it off and resubmit it.

>
> Jan, Sergio. How much do we want do dump this area ? The change above
> should be justified.
>
> Oleg.
>
  

On Thu, Mar 12, 2015 at 10:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/12, Andy Lutomirski wrote:
>>
>> On Thu, Mar 12, 2015 at 9:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 03/12, Andy Lutomirski wrote:
>> >>
>> > As for 32-bit applications. Yes, this can't work because 32-bit simply
>> > can't access this "high" memory. But you know, it would be very nice to
>> > have the fixmap-like "global" area in init_mm which is also visible to
>> > compat applications. If we had it, uprobes could work without xol vma's.
>> >
>> It could work for 32-bit native, but not for 32-bit compat.
>
> Yes, yes, I meant 32-bit compat apps. Once again, it would be nice if we
> had the "low" fixmaps in init_mm. But unlikely this is possible...
>
>> On a related note, I'm hoping to rework the mm part pretty heavily:
>>
>> http://lkml.kernel.org/r/cover.1414629045.git.luto@amacapital.net
>
> OK... not that I really understand this email.
>
> Well. Speaking of vdso. I understand that unlikely we can do this, but
> for uprobes it would be nice to have a anon-inode file behind this mapping,
> so that vma_interval_tree_foreach() could work, etc. OK, this is completely
> off-topic, please forget.

Couldn't you do that directly in the uprobes code?  That is, create an
anon_inode file and just map it the old-fashioned way?

--Andy

>
>
>
> And I noticed that I didn't read your previous email carefully enough...
>
>> That sounds reasonable to me.  I'll write the patch later today.
>
> Sure, please send a patch if you want to do this.
>
>> gdb will still need changes, though, right?
>
> This is up to gdb developers. To me, it should simply skip this
> VM_DONTDUMP vma.
>
> Oleg.
>
  

On 03/12, Sergio Durigan Junior wrote:
>
> On Thursday, March 12 2015, Oleg Nesterov wrote:
>
> >> gdb will still need changes, though, right?
> >
> > This is up to gdb developers. To me, it should simply skip this
> > VM_DONTDUMP vma.
>
> If I understood this discussion correctly (and thanks Andy and Oleg for,
> *ahem*, dumping all this useful information for us!), GDB will not need
> modifications in the Linux kernel in this area.  In fact, my patch
> already implements the "ignore VM_DONTDUMP mappings" part, so we're
> pretty much covered.

OK, thanks.

And it seems that we all agree that the kernel should not dump this vma
too. Could you confirm that this is fine from gdb pov just in case?


However. Even if we do not want it in the coredump, this can confuse gdb
users which might want to read this memory during debugging. So perhaps
we still can add ->access() to "fix" PTRACE_PEEK/access_remote_vm later.

But I see another email from Andy, so lets forget about this for now.

Oleg.
  

On 03/12, Andy Lutomirski wrote:
>
> On Thu, Mar 12, 2015 at 10:46 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 03/12, Oleg Nesterov wrote:
> >>
> >> Yes, this is true. OK, lets not dump it.
> >
> > OTOH. We can probably add ->access() into special_mapping_vmops, this
> > way __access_remote_vm() could work even if gup() fails ?
>
> Let's wait until my special_mapping vmops rework lands to do that.
> I'll dust it off and resubmit it.

OK. Please CC me. Not that I think I can help, just I want to understand
what you are going to do.

Although currently I do not even read most of emails I get ;)

Oleg.
  

On 03/12, Andy Lutomirski wrote:
>
> On Thu, Mar 12, 2015 at 10:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Well. Speaking of vdso. I understand that unlikely we can do this, but
> > for uprobes it would be nice to have a anon-inode file behind this mapping,
> > so that vma_interval_tree_foreach() could work, etc. OK, this is completely
> > off-topic, please forget.
>
> Couldn't you do that directly in the uprobes code?  That is, create an
> anon_inode file and just map it the old-fashioned way?

This won't help. Uprobes wants this file mmaped by all applications, so
that build_map_info() can find mm's, vma's, etc to install the system-wide
breakpoint. But again, this is off-topic and unlikely possible.

Oleg.
  

On 03/12/2015 05:46 PM, Oleg Nesterov wrote:
> On 03/12, Oleg Nesterov wrote:
>>
>> Yes, this is true. OK, lets not dump it.
> 
> OTOH. We can probably add ->access() into special_mapping_vmops, this
> way __access_remote_vm() could work even if gup() fails ?
> 
> Jan, Sergio. How much do we want do dump this area ? The change above
> should be justified.

Memory mappings that weren't touched since they were initially mapped can
be retrieved from the program binary and the shared libraries, even if
the core dump is moved to another machine.  However, in vvar case,
sounds like  there's nowhere to read it from offline?  In that case,
it could be justified to dump it.

Thanks,
Pedro Alves
  

On Thursday, March 12 2015, Oleg Nesterov wrote:

>> Let's wait until my special_mapping vmops rework lands to do that.
>> I'll dust it off and resubmit it.
>
> OK. Please CC me. Not that I think I can help, just I want to understand
> what you are going to do.

If you think your work will impact the core dumping part, please include
me in the Cc as well.
  

On Thu, Mar 12, 2015 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
> On 03/12/2015 05:46 PM, Oleg Nesterov wrote:
>> On 03/12, Oleg Nesterov wrote:
>>>
>>> Yes, this is true. OK, lets not dump it.
>>
>> OTOH. We can probably add ->access() into special_mapping_vmops, this
>> way __access_remote_vm() could work even if gup() fails ?
>>
>> Jan, Sergio. How much do we want do dump this area ? The change above
>> should be justified.
>
> Memory mappings that weren't touched since they were initially mapped can
> be retrieved from the program binary and the shared libraries, even if
> the core dump is moved to another machine.  However, in vvar case,
> sounds like  there's nowhere to read it from offline?  In that case,
> it could be justified to dump it.

This is why we currently dump the vdso text.  On arm64 (the only other
architecture that uses a real vma for vvar data IIRC), we use a more
normal vma and we dump it.  x86 is the odd one out here.

We could just leave the kernel alone.  The data that gets dumped is of
dubious value, but it could be slightly helpful when debugging vdso
crashes*, but, of course, dumping it is inherently racy.


* The vdso never crashes :)

--Andy

>
> Thanks,
> Pedro Alves
>
  

On Thursday, March 12 2015, Oleg Nesterov wrote:

>> If I understood this discussion correctly (and thanks Andy and Oleg for,
>> *ahem*, dumping all this useful information for us!), GDB will not need
>> modifications in the Linux kernel in this area.  In fact, my patch
>> already implements the "ignore VM_DONTDUMP mappings" part, so we're
>> pretty much covered.
>
> OK, thanks.
>
> And it seems that we all agree that the kernel should not dump this vma
> too. Could you confirm that this is fine from gdb pov just in case?

Yes, this is what we expect from the GDB side.  This mapping is marked
as "dd", so it does not make sense to dump it.

While I have you guys, would it be possible for the Linux kernel to
include a new flag on VmFlags to uniquely identify an anonymous mapping?
Currently, there is no easy way to do that from userspace.  My patch
implements the following heuristic on GDB:

  if (pathname == "/dev/zero (deleted)"
      || pathname == "/SYSV%08x (deleted)"
      || pathname == "<file> (deleted)"
      || "Anonymous:" field is > 0 kB
      || "AnonHugePages:" field is > 0 kB)
    mapping is anonymous;

However, this can be fragile.  The Linux kernel checks for i_nlink == 0,
but there is no easy way for GDB to check this on userspace (as Jan
mentioned, one could look at /proc/PID/map_files/, but they are only
accessible by root).  That's why I think it would be good to provide
this info right away in /proc/PID/smaps...

Thanks,
  

On 03/13, Sergio Durigan Junior wrote:
>
> On Thursday, March 12 2015, Oleg Nesterov wrote:
>
> > And it seems that we all agree that the kernel should not dump this vma
> > too. Could you confirm that this is fine from gdb pov just in case?
>
> Yes, this is what we expect from the GDB side.  This mapping is marked
> as "dd", so it does not make sense to dump it.

OK.

> While I have you guys, would it be possible for the Linux kernel to
> include a new flag on VmFlags to uniquely identify an anonymous mapping?

Note that "anonymous" is not the right term here... I mean it is a bit
confusing. Lets discuss this again on debug-list, then we will see if
gdb needs more info from kernel.

> Currently, there is no easy way to do that from userspace.  My patch
> implements the following heuristic on GDB:
>
>   if (pathname == "/dev/zero (deleted)"
>       || pathname == "/SYSV%08x (deleted)"
>       || pathname == "<file> (deleted)"

And for example, this is not anonymous mapping. But,

>     mapping is anonymous;

I agree, gdb should treat it as anonymous.

> However, this can be fragile.  The Linux kernel checks for i_nlink == 0,

Yes, as we already disccussed, I think the kernel should be changed.

It should do something like shmem_mapping() || d_unlinked(), I think.
But this needs another discussion on lkml, and in another thread.

Oleg.
  

On 03/12, Oleg Nesterov wrote:
>
> OTOH. We can probably add ->access() into special_mapping_vmops, this
> way __access_remote_vm() could work even if gup() fails ?

So I tried to think how special_mapping_vmops->access() can work, it
needs to rely on ->vm_pgoff.

But afaics this logic is just broken. Lets even forget about vvar vma
which uses remap_file_pages(). Lets look at "[vdso]" which uses the
"normal" pages.

The comment in special_mapping_fault() says

	 * special mappings have no vm_file, and in that case, the mm
	 * uses vm_pgoff internally.

Yes. But afaics mm/ doesn't do this correctly. So

	 * do not copy this code into drivers!

looks like a good recommendation ;)

I think that this logic is wrong even if ARRAY_SIZE(pages) == 1, but I am
not sure. But since vdso use 2 pages, it is trivial to show that this logic
is wrong. To verify, I changed show_map_vma() to expose pgoff even if !file,
but this test-case can show the problem too:

	#include <stdio.h>
	#include <unistd.h>
	#include <stdlib.h>
	#include <string.h>
	#include <sys/mman.h>
	#include <assert.h>

	void *find_vdso_vaddr(void)
	{
		FILE *perl;
		char buf[32] = {};

		perl = popen("perl -e 'open STDIN,qq|/proc/@{[getppid]}/maps|;"
				"/^(.*?)-.*vdso/ && print hex $1 while <>'", "r");
		fread(buf, sizeof(buf), 1, perl);
		fclose(perl);

		return (void *)atol(buf);
	}

	#define PAGE_SIZE	4096

	int main(void)
	{
		void *vdso = find_vdso_vaddr();
		assert(vdso);

		// of course they should differ, and they do so far
		printf("vdso pages differ: %d\n",
			!!memcmp(vdso, vdso + PAGE_SIZE, PAGE_SIZE));

		// split into 2 vma's
		assert(mprotect(vdso, PAGE_SIZE, PROT_READ) == 0);

		// force another fault on the next check
		assert(madvise(vdso, 2 * PAGE_SIZE, MADV_DONTNEED) == 0);

		// now they no longer differ, the 2nd vm_pgoff is wrong
		printf("vdso pages differ: %d\n",
			!!memcmp(vdso, vdso + PAGE_SIZE, PAGE_SIZE));

		return 0;
	}

output:

	vdso pages differ: 1
	vdso pages differ: 0

And not only "split_vma" is wrong, I think that "move_vma" is not right too.
Note this check in copy_vma(),

	/*
	 * If anonymous vma has not yet been faulted, update new pgoff
	 * to match new location, to increase its chance of merging.
	 */
	if (unlikely(!vma->vm_file && !vma->anon_vma)) {
		pgoff = addr >> PAGE_SHIFT;
		faulted_in_anon_vma = false;
	}

I can easily misread this code. But it doesn't look right too. If vdso was cow'ed
(breakpoint installed by gdb) and sys_nremap()'ed, then the new pgoff will be wrong
too after, say, MADV_DONTNEED.

Or I am totally confused?

Oleg.
  

[cc: linux-mm]

On Mon, Mar 16, 2015 at 12:01 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/12, Oleg Nesterov wrote:
>>
>> OTOH. We can probably add ->access() into special_mapping_vmops, this
>> way __access_remote_vm() could work even if gup() fails ?
>
> So I tried to think how special_mapping_vmops->access() can work, it
> needs to rely on ->vm_pgoff.
>
> But afaics this logic is just broken. Lets even forget about vvar vma
> which uses remap_file_pages(). Lets look at "[vdso]" which uses the
> "normal" pages.
>
> The comment in special_mapping_fault() says
>
>          * special mappings have no vm_file, and in that case, the mm
>          * uses vm_pgoff internally.
>
> Yes. But afaics mm/ doesn't do this correctly. So
>
>          * do not copy this code into drivers!
>
> looks like a good recommendation ;)
>
> I think that this logic is wrong even if ARRAY_SIZE(pages) == 1, but I am
> not sure. But since vdso use 2 pages, it is trivial to show that this logic
> is wrong. To verify, I changed show_map_vma() to expose pgoff even if !file,
> but this test-case can show the problem too:
>
>         #include <stdio.h>
>         #include <unistd.h>
>         #include <stdlib.h>
>         #include <string.h>
>         #include <sys/mman.h>
>         #include <assert.h>
>
>         void *find_vdso_vaddr(void)
>         {
>                 FILE *perl;
>                 char buf[32] = {};
>
>                 perl = popen("perl -e 'open STDIN,qq|/proc/@{[getppid]}/maps|;"
>                                 "/^(.*?)-.*vdso/ && print hex $1 while <>'", "r");
>                 fread(buf, sizeof(buf), 1, perl);
>                 fclose(perl);
>
>                 return (void *)atol(buf);
>         }
>
>         #define PAGE_SIZE       4096
>
>         int main(void)
>         {
>                 void *vdso = find_vdso_vaddr();
>                 assert(vdso);
>
>                 // of course they should differ, and they do so far
>                 printf("vdso pages differ: %d\n",
>                         !!memcmp(vdso, vdso + PAGE_SIZE, PAGE_SIZE));
>
>                 // split into 2 vma's
>                 assert(mprotect(vdso, PAGE_SIZE, PROT_READ) == 0);
>
>                 // force another fault on the next check
>                 assert(madvise(vdso, 2 * PAGE_SIZE, MADV_DONTNEED) == 0);

I really hope this doesn't do anything (or fails) on the vvar page,
which is a pfnmap.

>
>                 // now they no longer differ, the 2nd vm_pgoff is wrong
>                 printf("vdso pages differ: %d\n",
>                         !!memcmp(vdso, vdso + PAGE_SIZE, PAGE_SIZE));
>
>                 return 0;
>         }
>
> output:
>
>         vdso pages differ: 1
>         vdso pages differ: 0
>
> And not only "split_vma" is wrong, I think that "move_vma" is not right too.
> Note this check in copy_vma(),
>
>         /*
>          * If anonymous vma has not yet been faulted, update new pgoff
>          * to match new location, to increase its chance of merging.
>          */
>         if (unlikely(!vma->vm_file && !vma->anon_vma)) {
>                 pgoff = addr >> PAGE_SHIFT;
>                 faulted_in_anon_vma = false;
>         }
>
> I can easily misread this code. But it doesn't look right too. If vdso was cow'ed
> (breakpoint installed by gdb) and sys_nremap()'ed, then the new pgoff will be wrong
> too after, say, MADV_DONTNEED.
>
> Or I am totally confused?

Ick, you're probably right.  For what it's worth, the vdso *seems* to
be okay (on 64-bit only, and only if you don't poke at it too hard) if
you mremap it in one piece.  CRIU does that.

What does the mm code do with vm_pgoff for vmas with no vm_file?  I'm
mystified.  There's this comment:

 * The way we recognize COWed pages within VM_PFNMAP mappings is through the
 * rules set up by "remap_pfn_range()": the vma will have the VM_PFNMAP bit
 * set, and the vm_pgoff will point to the first PFN mapped: thus every special
 * mapping will always honor the rule
 *
 *    pfn_of_page == vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT)

Is that referring to special mappings in the install_special_mapping
sense or to something else.  FWIW, the vdso ins't a VM_PFNMAP at all.

--Andy
  

Thanks for looking over all this, guys.  Really appreciated.

On 03/16/2015 07:01 PM, Oleg Nesterov wrote:
> is wrong. To verify, I changed show_map_vma() to expose pgoff even if !file,
> but this test-case can show the problem too:

Might be good to add tests like this to selftests/ once all
this is sorted.

Pedro Alves
  

On 03/16, Andy Lutomirski wrote:
>
> Ick, you're probably right.  For what it's worth, the vdso *seems* to
> be okay (on 64-bit only, and only if you don't poke at it too hard) if
> you mremap it in one piece.  CRIU does that.

I need to run away till tomorrow, but looking at this code even if "one piece"
case doesn't look right if it was cow'ed. I'll verify tomorrow.

Oleg.