get_user_pages vs mmap MAP_FIXED bug

get_user_pages vs mmap MAP_FIXED bug

[Rusty Russell]

This bug is in 2.6.21-rc7-mm2, but not 2.6.21.  Haven't tested
2.6.21-mm1 yet.

The following kernel module uses get_user_pages() to look at user
memory, and gets zeroed pages if they're not touched by userspace first.
The test case seems a little sensitive to change (eg. mapping at a
non-fixed address seems to work).

Usage:
	(build kernel)
	gcc -o examine_mmap examine_mmap.c
	sudo insmod kernel/examiner.ko
	sudo ./examine_mmap
		examine_mmap: For 0x1000: kernel sees 0, I see 1179403647
	sudo ./examine_mmap --touch
		OK: Both saw 1179403647

Anyone?
Rusty.

diff -r 142e4ca8c069 examine_mmap.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/examine_mmap.c	Sun May 06 16:21:19 2007 +1000
@@ -0,0 +1,38 @@
+/* Make with gcc -o examine_mmap examine_mmap.c */
+#include <unistd.h>
+#include <string.h>
+#include <stdio.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+int main(int argc, char *argv[])
+{
+	int *p;
+	char addrstr[80];
+	int res, fd;
+
+	/* Grab some file to mmap. */
+	fd = open("./vmlinux", O_RDONLY);
+	if (fd < 0)
+		err(1, "open");
+
+	/* Map one page of it at 4096 */
+	p = (void *)4096;
+	if (mmap(p, 4096, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_PRIVATE|MAP_FIXED, fd, 0) != p)
+		err(1, "mmap");
+
+	/* This makes it work. */
+	if (argc > 1 && strcmp(argv[1], "--touch") == 0)
+		printf("Before: %p == %i\n", p, *p);
+
+	/* Find out what the kernel thinks it is with get_user_pages() */
+	fd = open("/proc/examiner", O_RDWR);
+	sprintf(addrstr, "%p", p);
+	res = write(fd, addrstr, strlen(addrstr));
+	if (res != *p)
+		errx(1, "For %p: kernel sees %i, I see %i\n", p, res, *p);
+	printf("OK: Both saw %i\n", res);
+	return 0;
+}
diff -r 142e4ca8c069 kernel/Makefile
--- a/kernel/Makefile	Sat May 05 22:01:39 2007 +1000
+++ b/kernel/Makefile	Sun May 06 10:38:27 2007 +1000
@@ -53,6 +53,7 @@ obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o
 obj-$(CONFIG_TASKSTATS) += taskstats.o tsacct.o
+obj-m += examiner.o
 
 ifneq ($(CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff -r 142e4ca8c069 kernel/examiner.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/kernel/examiner.c	Sun May 06 16:15:24 2007 +1000
@@ -0,0 +1,45 @@
+#include <linux/module.h>
+#include <linux/proc_fs.h>
+#include <linux/highmem.h>
+#include <asm/uaccess.h>
+
+ssize_t examiner_write(struct file *f, const char __user *u, size_t s, loff_t *o)
+{
+	char addrstr[20];
+	unsigned long addr, res = 0xBADFEED;
+	struct page *page;
+
+	copy_from_user(addrstr, u, sizeof(addrstr));
+	addr = simple_strtoul(addrstr, NULL, 0);
+
+	down_read(&current->mm->mmap_sem);
+	if (get_user_pages(current, current->mm, addr,
+			   1, 0, 1, &page, NULL) == 1) {
+		int *p = kmap(page);
+		res = *p;
+		kunmap(page);
+	}
+	up_read(&current->mm->mmap_sem);
+	return res;
+}
+
+static const struct file_operations examiner_fops = {
+	.write		= examiner_write,
+};
+
+
+static int examiner(void)
+{
+	struct proc_dir_entry *p;
+
+	p = create_proc_entry("examiner", 0600, NULL);
+	p->proc_fops = &examiner_fops;
+	return 0;
+}
+
+static void unexaminer(void)
+{
+	remove_proc_entry("examiner", NULL);
+}
+module_init(examiner);
+module_exit(unexaminer);

Re: get_user_pages vs mmap MAP_FIXED bug

[Rusty Russell]

On Sun, 2007-05-06 at 17:01 +1000, Rusty Russell wrote:
> This bug is in 2.6.21-rc7-mm2, but not 2.6.21.  Haven't tested
> 2.6.21-mm1 yet.

OK, 2.6.21-mm1 fails too.  2.6.21-git6 ... is fine.

Here's a standalone test using ptrace.  No kernel module req'd.

	rusty@debussy:~/linux-2.6.21-mm1$ ../examiner 
	ptrace says 0, child says 0x464c457f

Any clues Andrew?  Or should I take your patches and do a binary search?

Cheers,
Rusty.

/* Make with gcc -o examine_mmap examine_mmap.c */
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ptrace.h>
#include <signal.h>
#include <err.h>
#include <errno.h>

/* Compare ptrace (ie. get_user_pages()) result with what process
 * actually sees. */
int main(int argc, char *argv[])
{
	long int *p, res1, res2;
	int fd, child;
	int child2parent[2], parent2child[2];
	char c = 0;

	pipe(child2parent);
	pipe(parent2child);

	if ((child = fork()) == 0) {

		/* Grab some file to mmap. */
		fd = open(argv[0], O_RDONLY);
		if (fd < 0)
			err(1, "open");

		/* Map one page of it at 4096 */
		p = (void *)4096;
		if (mmap(p, 4096, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_PRIVATE|MAP_FIXED, fd, 0) != p)
			err(1, "mmap");

		/* This makes it work. */
		if (argc > 1 && strcmp(argv[1], "--touch") == 0)
			printf("Before: %p == %li\n", p, *p);

		/* Allow parent to trace me. */
		if (ptrace(PTRACE_TRACEME, 0, 0, 0) != 0)
			err(1, "PTRACE_TRACEME");

		/* Tell parent to look at memory, and wait for response. */
		write(child2parent[1], &c, 1);
		read(parent2child[0], &c, 1);

		/* Write what we see to pipe */
		write(child2parent[1], p, sizeof(*p));
		exit(0);
	}

	/* Wait for child to be ready. */
	read(child2parent[0], &c, 1);

	/* Child needs to be stopped.  What a POS. */
	kill(child, SIGSTOP);
	sleep(1);

	/* Get ptrace to tell us what the kernel thinks the value is */
	res1 = ptrace(PTRACE_PEEKDATA, child, (void *)4096, NULL);

	/* Continue the child. */
	ptrace(PTRACE_DETACH, child, NULL, NULL);

	/* Tell the child to continue. */
	write(parent2child[1], &c, 1);

	/* Get the child's opinion of what the value is. */
	read(child2parent[0], &res2, sizeof(res2));

	printf("ptrace says %#lx, child says %#lx\n", res1, res2);
	exit(res1 == res2 ? 0 : 1);
}

Re: get_user_pages vs mmap MAP_FIXED bug

[Andrew Morton]

On Mon, 07 May 2007 11:26:44 +1000 Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Sun, 2007-05-06 at 17:01 +1000, Rusty Russell wrote:
> > This bug is in 2.6.21-rc7-mm2, but not 2.6.21.  Haven't tested
> > 2.6.21-mm1 yet.
> 
> OK, 2.6.21-mm1 fails too.  2.6.21-git6 ... is fine.
> 
> Here's a standalone test using ptrace.  No kernel module req'd.
> 
> 	rusty@debussy:~/linux-2.6.21-mm1$ ../examiner 
> 	ptrace says 0, child says 0x464c457f

thanks.

> Any clues Andrew?  Or should I take your patches and do a binary search?

I've been assuming that Nick's vm_operations.fault changes have broken
get_user_pages()'s manual faulting.  The next step is to wait for Nick to
turn up.

Re: get_user_pages vs mmap MAP_FIXED bug

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

Andrew Morton wrote:
> On Mon, 07 May 2007 11:26:44 +1000 Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> 
>>On Sun, 2007-05-06 at 17:01 +1000, Rusty Russell wrote:
>>
>>>This bug is in 2.6.21-rc7-mm2, but not 2.6.21.  Haven't tested
>>>2.6.21-mm1 yet.
>>
>>OK, 2.6.21-mm1 fails too.  2.6.21-git6 ... is fine.
>>
>>Here's a standalone test using ptrace.  No kernel module req'd.
>>
>>	rusty@debussy:~/linux-2.6.21-mm1$ ../examiner 
>>	ptrace says 0, child says 0x464c457f
> 
> 
> thanks.
> 
> 
>>Any clues Andrew?  Or should I take your patches and do a binary search?
> 
> 
> I've been assuming that Nick's vm_operations.fault changes have broken
> get_user_pages()'s manual faulting.  The next step is to wait for Nick to
> turn up.

Fancy that! Thanks for catching it Rusty, does the following work for
you?

-- 
SUSE Labs, Novell Inc.

[-- Attachment #2: mm-fix-for-fault.patch --]
[-- Type: text/plain, Size: 1485 bytes --]

Fix a couple of places that should be testing fault as well as nopage.

Signed-off-by: Nick Piggin <npiggin@suse.de>

Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c	2007-04-24 15:02:51.000000000 +1000
+++ linux-2.6/mm/memory.c	2007-05-08 15:23:55.000000000 +1000
@@ -1049,7 +1049,8 @@
 		if (pages)
 			foll_flags |= FOLL_GET;
 		if (!write && !(vma->vm_flags & VM_LOCKED) &&
-		    (!vma->vm_ops || !vma->vm_ops->nopage))
+		    (!vma->vm_ops || (!vma->vm_ops->nopage &&
+					!vma->vm_ops->fault)))
 			foll_flags |= FOLL_ANON;
 
 		do {
Index: linux-2.6/mm/rmap.c
===================================================================
--- linux-2.6.orig/mm/rmap.c	2007-04-24 08:53:50.000000000 +1000
+++ linux-2.6/mm/rmap.c	2007-05-08 15:24:27.000000000 +1000
@@ -640,8 +640,10 @@
 			printk (KERN_EMERG "  page->count = %x\n", page_count(page));
 			printk (KERN_EMERG "  page->mapping = %p\n", page->mapping);
 			print_symbol (KERN_EMERG "  vma->vm_ops = %s\n", (unsigned long)vma->vm_ops);
-			if (vma->vm_ops)
+			if (vma->vm_ops) {
 				print_symbol (KERN_EMERG "  vma->vm_ops->nopage = %s\n", (unsigned long)vma->vm_ops->nopage);
+				print_symbol (KERN_EMERG "  vma->vm_ops->fault = %s\n", (unsigned long)vma->vm_ops->fault);
+			}
 			if (vma->vm_file && vma->vm_file->f_op)
 				print_symbol (KERN_EMERG "  vma->vm_file->f_op->mmap = %s\n", (unsigned long)vma->vm_file->f_op->mmap);
 			BUG();

Re: get_user_pages vs mmap MAP_FIXED bug

[Rusty Russell]

On Tue, 2007-05-08 at 17:41 +1000, Nick Piggin wrote:
> Fancy that! Thanks for catching it Rusty, does the following work for
> you?

Hi Nick,

	After 24 hours, it seems to be holding up (the original problem showed
up in lguest, which uses get_user_pages extensively).

Thanks!
Rusty.

Re: get_user_pages vs mmap MAP_FIXED bug

[Nick Piggin]

Hi Rusty!

Rusty Russell wrote:
> On Tue, 2007-05-08 at 17:41 +1000, Nick Piggin wrote:
> 
>>Fancy that! Thanks for catching it Rusty, does the following work for
>>you?
> 
> 
> Hi Nick,
> 
> 	After 24 hours, it seems to be holding up (the original problem showed
> up in lguest, which uses get_user_pages extensively).
> 
> Thanks!
> Rusty.

No, thank _you_ for finding the bug :)

-- 
SUSE Labs, Novell Inc.