install_special_mapping && vm_pgoff (Was: vvar, gup && coredump)

[Oleg Nesterov <oleg at redhat dot com>]

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.