[cc: linux-mm]
On Mon, Mar 16, 2015 at 12:01 PM, Oleg Nesterov <oleg@xxxxxxxxxx> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>