vvar, gup && coredump

Quoted 8 lines · Jan Kratochvil

Quoted 6 lines · Sergio Durigan Junior

Quoted 5 lines · Jan Kratochvil

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.

                                                                ^^^^^^^^^^^^^^

Quoted 2 lines

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.

Add cc's, change subject.

Quoted 11 lines · Oleg Nesterov

Quoted 8 lines · Jan Kratochvil

Quoted 6 lines · Sergio Durigan Junior

Quoted 5 lines · Jan Kratochvil

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.

                                                                ^^^^^^^^^^^^^^

Quoted 8 lines

Quoted 2 lines

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.

Quoted 4 lines · Oleg Nesterov

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.

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Oleg Nesterov

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

Quoted 8 lines

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?

Quoted 5 lines

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

Quoted 7 lines · Oleg Nesterov

Quoted 1 line · Andy Lutomirski

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.

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

http://lkml.kernel.org/r/[email protected]

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

gdb will still need changes, though, right?

Quoted 1 line

gdb will still need changes, though, right?

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

Quoted 2 lines · Oleg Nesterov

Quoted 1 line

gdb will still need changes, though, right?

Quoted 2 lines

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.

Quoted 5 lines

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?

Quoted 2 lines · Oleg Nesterov

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)"

    mapping is anonymous;

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

Quoted 10 lines · Andy Lutomirski

Quoted 7 lines · Oleg Nesterov

Quoted 1 line · Andy Lutomirski

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

Quoted 2 lines

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

Quoted 1 line

http://lkml.kernel.org/r/[email protected]

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

Quoted 1 line

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

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

Quoted 1 line

gdb will still need changes, though, right?

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

Oleg.

Quoted 4 lines · Oleg Nesterov

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?

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Oleg Nesterov

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.

Quoted 2 lines

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.

What's wrong with off-topic? :)

As vvar demonstrates, it's possible to add new per-process vmas that show up in all processes automatically. The trickiest part is making it work with CRIU.

Yes, this is true. OK, lets not dump it.

Quoted 2 lines · Oleg Nesterov

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.

Quoted 6 lines · Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

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.

Quoted 2 lines

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.

Quoted 2 lines · Oleg Nesterov

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.

Quoted 7 lines · Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

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 ?

Quoted 2 lines

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

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

Quoted 3 lines · Oleg Nesterov

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?

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.

Quoted 4 lines · Andy Lutomirski

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.

Quoted 9 lines · Oleg Nesterov

Quoted 4 lines · Andy Lutomirski

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.

And I am still not sure this all is 100% correct, but I got lost in this code. Probably this is fine...

But at least the bug exposed by the test-case looks clear:

        do_linear_fault:

                vmf->pgoff = (((address & PAGE_MASK) - vma->vm_start) >> PAGE_SHIFT)
                                + vma->vm_pgoff;
                ...

                special_mapping_fault:

                        pgoff = vmf->pgoff - vma->vm_pgoff;

So special_mapping_fault() can only work if this mapping starts from the first page in ->pages[].

So perhaps we need _something like_ the (wrong/incomplete) patch below...

Or, really, perhaps we can create vdso_mapping ? So that map_vdso() could simply mmap the anon_inode file...

Oleg.

mm/mmap.c +10 -2

@@ -2832,6 +2832,8 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
        return 0;
 }
 
+bool is_special_vma(struct vm_area_struct *vma);
+
 /*
  * Copy the vma structure to a new location in the same mm,
  * prior to moving page table entries, to effect an mremap move.

@@ -2851,7 +2853,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
         * 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)) {
+       if (unlikely(!vma->vm_file && !is_special_vma(vma) && !vma->anon_vma)) {
                pgoff = addr >> PAGE_SHIFT;
                faulted_in_anon_vma = false;
        }

@@ -2953,6 +2955,11 @@ static const struct vm_operations_struct legacy_special_mapping_vmops = {
        .fault = special_mapping_fault,
 };
 
+bool is_special_vma(struct vm_area_struct *vma)
+{
+       return vma->vm_ops == &special_mapping_vmops;
+}
+
 static int special_mapping_fault(struct vm_area_struct *vma,
                                struct vm_fault *vmf)
 {

@@ -2965,7 +2972,7 @@ static int special_mapping_fault(struct vm_area_struct *vma,
         * We are allowed to do this because we are the mm; do not copy
         * this code into drivers!
         */
-       pgoff = vmf->pgoff - vma->vm_pgoff;
+       pgoff = vmf->pgoff;
 
        if (vma->vm_ops == &legacy_special_mapping_vmops)
                pages = vma->vm_private_data;

@@ -3014,6 +3021,7 @@ static struct vm_area_struct *__install_special_mapping(
        if (ret)
                goto out;
 
+       vma->vm_pgoff = 0;
        mm->total_vm += len >> PAGE_SHIFT;
 
        perf_event_mmap(vma);

Quoted 17 lines · Oleg Nesterov

But at least the bug exposed by the test-case looks clear:

        do_linear_fault:

                vmf->pgoff = (((address & PAGE_MASK) - vma->vm_start) >> PAGE_SHIFT)
                                + vma->vm_pgoff;
                ...

                special_mapping_fault:

                        pgoff = vmf->pgoff - vma->vm_pgoff;

So special_mapping_fault() can only work if this mapping starts from the first page in ->pages[].

Quoted 4 lines

So perhaps we need _something like_ the (wrong/incomplete) patch below...

Or, really, perhaps we can create vdso_mapping ? So that map_vdso() could simply mmap the anon_inode file...

That's slightly tricky, I think, because it could start showing up in /proc/PID/map_files or whatever it's called, and I don't think we want that.

Your patch does look like a considerable improvement, though. Let me see if I can find some time to fold it in with the rest of my special mapping rework over the next few days.

is wrong. To verify, I changed show_map_vma() to expose pgoff even if !file, but this test-case can show the problem too: