/proc/pid/pagemap update

Matt, this patch set replaces the two patches I sent earlier and contains additional fixes. I've done some reasonably rigorous testing on x86_64, but not on a 32 bit arch. I'm pretty sure this isn't worse than what's in mm right now, which has some user-space corruption and a nasty infinite kernel loop. YMMV.

This is a series of patches to update /proc/pid/pagemap, to simplify the code, and fix bugs which caused userspace memory corruption.

Since it is just in -mm, we should probably roll all of these together and just update it all at once, or send a simple drop-on replacement patch. These patches are all here mostly to help with review.

Matt, if you're OK with these, do you mind if I send the update into -mm, or would you like to do it?

--
From: Matt Mackall <[email protected]>

Quoted 6 lines · Dave Boutcher

Matt, this patch set replaces the two patches I sent earlier and contains additional fixes. I've done some reasonably rigorous testing on x86_64, but not on a 32 bit arch. I'm pretty sure this isn't worse than what's in mm right now, which has some user-space corruption and a nasty infinite kernel loop. YMMV.

Dave, here's my current work-in-progress patch to deal with a couple locking issues, primarily a possible deadlock on the mm semaphore that can occur if someone unmaps the target buffer while we're walking the tree. It currently hangs on my box and I haven't had any free cycles to finish debugging it, but you might want to take a peek at it.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |  121 ++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 66 deletions(-)

diff -puN fs/proc/task_mmu.c~Re-_PATCH_0_3_proc_pid_pagemap_fixes fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~Re-_PATCH_0_3_proc_pid_pagemap_fixes +55 -66

@@ -501,37 +501,21 @@ const struct file_operations proc_clear_
 };
 
 struct pagemapread {
-	struct mm_struct *mm;
 	unsigned long next;
-	unsigned long *buf;
-	pte_t *ptebuf;
 	unsigned long pos;
 	size_t count;
 	int index;
-	char __user *out;
+	unsigned long __user *out;
 };
 
-static int flush_pagemap(struct pagemapread *pm)
-{
-	int n = min(pm->count, pm->index * sizeof(unsigned long));
-	if (copy_to_user(pm->out, pm->buf, n))
-		return -EFAULT;
-	pm->out += n;
-	pm->pos += n;
-	pm->count -= n;
-	pm->index = 0;
-	cond_resched();
-	return 0;
-}
-
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,
 			  struct pagemapread *pm)
 {
-	pm->buf[pm->index++] = pfn;
+	__put_user(pfn, pm->out);
+	pm->out++;
+	pm->pos += sizeof(unsigned long);
+	pm->count -= sizeof(unsigned long);
 	pm->next = addr + PAGE_SIZE;
-	if (pm->index * sizeof(unsigned long) >= PAGE_SIZE ||
-	    pm->index * sizeof(unsigned long) >= pm->count)
-		return flush_pagemap(pm);
 	return 0;
 }
 

@@ -543,14 +527,6 @@ static int pagemap_pte_range(pmd_t *pmd,
 	int err;
 
 	pte = pte_offset_map(pmd, addr);
-
-#ifdef CONFIG_HIGHPTE
-	/* copy PTE directory to temporary buffer and unmap it */
-	memcpy(pm->ptebuf, pte, PAGE_ALIGN((unsigned long)pte) - (unsigned long)pte);
-	pte_unmap(pte);
-	pte = pm->ptebuf;
-#endif
-
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
 		if (addr < pm->next)
 			continue;

@@ -560,11 +536,12 @@ static int pagemap_pte_range(pmd_t *pmd,
 			err = add_to_pagemap(addr, pte_pfn(*pte), pm);
 		if (err)
 			return err;
+		if (pm->count == 0)
+			break;
 	}
-
-#ifndef CONFIG_HIGHPTE
 	pte_unmap(pte - 1);
-#endif
+
+	cond_resched();
 
 	return 0;
 }

@@ -573,7 +550,7 @@ static int pagemap_fill(struct pagemapre
 {
 	int ret;
 
-	while (pm->next != end) {
+	while (pm->next != end && pm->count > 0) {
 		ret = add_to_pagemap(pm->next, -1UL, pm);
 		if (ret)
 			return ret;

@@ -608,15 +585,16 @@ static ssize_t pagemap_read(struct file 
 {
 	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
 	unsigned long src = *ppos;
-	unsigned long *page;
-	unsigned long addr, end, vend, svpfn, evpfn;
+	struct page **pages, *page;
+	unsigned long addr, end, vend, svpfn, evpfn, uaddr, uend;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
 	struct pagemapread pm;
+	int pagecount;
 	int ret = -ESRCH;
 
 	if (!task)
-		goto out_no_task;
+		goto out;
 
 	ret = -EACCES;
 	if (!ptrace_may_attach(task))

@@ -628,39 +606,43 @@ static ssize_t pagemap_read(struct file 
 	if ((svpfn + 1) * sizeof(unsigned long) != src)
 		goto out;
 	evpfn = min((src + count) / sizeof(unsigned long) - 1,
-		    ((~0UL) >> PAGE_SHIFT) + 1);
+		    ((~0UL) >> PAGE_SHIFT) + 1) - 1;
 	count = (evpfn - svpfn) * sizeof(unsigned long);
 	end = PAGE_SIZE * evpfn;
-
-	ret = -ENOMEM;
-	page = kzalloc(PAGE_SIZE, GFP_USER);
-	if (!page)
-		goto out;
-
-#ifdef CONFIG_HIGHPTE
-	pm.ptebuf = kzalloc(PAGE_SIZE, GFP_USER);
-	if (!pm.ptebuf)
-		goto out_free;
-#endif
+	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
 
 	ret = 0;
 	mm = get_task_mm(task);
 	if (!mm)
-		goto out_freepte;
+		goto out;
+
+	ret = -ENOMEM;
+	uaddr = (unsigned long)buf & ~(PAGE_SIZE-1);
+	uend = (unsigned long)(buf + count);
+	pagecount = (uend - uaddr + PAGE_SIZE-1) / PAGE_SIZE;
+	pages = kmalloc(pagecount * sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		goto out_task;
+
+	down_read(&current->mm->mmap_sem);
+	ret = get_user_pages(current, current->mm, uaddr, pagecount,
+			     1, 0, pages, NULL);
+	up_read(&current->mm->mmap_sem);
+
+	//printk("%x(%x):%x %d@%ld (%d pages) -> %d\n", uaddr, buf, uend, count, src, pagecount, ret);
+	if (ret < 0)
+		goto out_free;
 
-	pm.mm = mm;
 	pm.next = addr;
-	pm.buf = page;
 	pm.pos = src;
 	pm.count = count;
-	pm.index = 0;
-	pm.out = buf;
+	pm.out = (unsigned long __user *)buf;
 
 	if (svpfn == -1) {
-		((char *)page)[0] = (ntohl(1) != 1);
-		((char *)page)[1] = PAGE_SHIFT;
-		((char *)page)[2] = sizeof(unsigned long);
-		((char *)page)[3] = sizeof(unsigned long);
+		put_user((char)(ntohl(1) != 1), buf);
+		put_user((char)PAGE_SHIFT, buf + 1);
+		put_user((char)sizeof(unsigned long), buf + 2);
+		put_user((char)sizeof(unsigned long), buf + 3);
 		add_to_pagemap(pm.next, page[0], &pm);
 	}
 

@@ -669,7 +651,8 @@ static ssize_t pagemap_read(struct file 
 	while (pm.count > 0 && vma) {
 		if (!ptrace_may_attach(task)) {
 			ret = -EIO;
-			goto out_mm;
+			up_read(&mm->mmap_sem);
+			goto out_release;
 		}
 		vend = min(vma->vm_end - 1, end - 1) + 1;
 		ret = pagemap_fill(&pm, vend);

@@ -682,23 +665,29 @@ static ssize_t pagemap_read(struct file 
 	}
 	up_read(&mm->mmap_sem);
 
+	//printk("before fill at %ld\n", pm.pos);
 	ret = pagemap_fill(&pm, end);
 
+	printk("after fill at %ld\n", pm.pos);
 	*ppos = pm.pos;
 	if (!ret)
 		ret = pm.pos - src;
 
-out_mm:
+out_release:
+	printk("releasing pages\n");
+	for (; pagecount; pagecount--) {
+		page = pages[pagecount-1];
+		if (!PageReserved(page))
+			SetPageDirty(page);
+		page_cache_release(page);
+	}
 	mmput(mm);
-out_freepte:
-#ifdef CONFIG_HIGHPTE
-	kfree(pm.ptebuf);
 out_free:
-#endif
-	kfree(page);
-out:
+	kfree(pages);
+out_task:
 	put_task_struct(task);
-out_no_task:
+out:
+	printk("returning\n");
 	return ret;
 }
 

_

The /proc/<pid>/pagemap file has a header containing:

 * first byte:   0 for big endian, 1 for little
 * second byte:  page shift (eg 12 for 4096 byte pages)
 * third byte:   entry size in bytes (currently either 4 or 8)
 * fourth byte:  header size

The endianness is only useful when examining a raw dump of pagemap from a different machine when you don't know the source of the file. This is pretty rare, and the programs or scripts doing the copying off-machine can certainly be made to hold this information.

The page size is available in userspace at least with libc's getpagesize(). This will also never vary across processes, so putting it in a per-process file doesn't make any difference. If we need a "kernel's page size" exported to userspace, perhaps we can put it in /proc/meminfo.

The entry size is the really tricky one. This can't just be sizeof(unsigned long) from userspace because we can have 32-bit processes on 64-bit kernels. But, userspace can certainly derive this value if it lseek()s to the end of the file, and divides the file position by the size of its virtual address space.

In any case, I believe this information is redundant, and can be removed.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-no-header fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-no-header +3 -11

@@ -601,12 +601,12 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EIO;
-	svpfn = src / sizeof(unsigned long) - 1;
+	svpfn = src / sizeof(unsigned long);
 	addr = PAGE_SIZE * svpfn;
-	if ((svpfn + 1) * sizeof(unsigned long) != src)
+	if (svpfn * sizeof(unsigned long) != src)
 		goto out;
 	evpfn = min((src + count) / sizeof(unsigned long) - 1,
-		    ((~0UL) >> PAGE_SHIFT) + 1) - 1;
+		    ((~0UL) >> PAGE_SHIFT) + 1);
 	count = (evpfn - svpfn) * sizeof(unsigned long);
 	end = PAGE_SIZE * evpfn;
 	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);

@@ -638,14 +638,6 @@ static ssize_t pagemap_read(struct file 
 	pm.count = count;
 	pm.out = (unsigned long __user *)buf;
 
-	if (svpfn == -1) {
-		put_user((char)(ntohl(1) != 1), buf);
-		put_user((char)PAGE_SHIFT, buf + 1);
-		put_user((char)sizeof(unsigned long), buf + 2);
-		put_user((char)sizeof(unsigned long), buf + 3);
-		add_to_pagemap(pm.next, page[0], &pm);
-	}
-
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, pm.next);
 	while (pm.count > 0 && vma) {

_

Use existing macros (PAGE_MASK/PAGE_ALIGN()) instead of open-coding them.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-use-PAGE_MASK fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-use-PAGE_MASK +2 -2

@@ -617,9 +617,9 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -ENOMEM;
-	uaddr = (unsigned long)buf & ~(PAGE_SIZE-1);
+	uaddr = (unsigned long)buf & PAGE_MASK;
 	uend = (unsigned long)(buf + count);
-	pagecount = (uend - uaddr + PAGE_SIZE-1) / PAGE_SIZE;
+	pagecount = (PAGE_ALIGN(uend) - uaddr) / PAGE_SIZE;
 	pages = kmalloc(pagecount * sizeof(struct page *), GFP_KERNEL);
 	if (!pages)
 		goto out_task;

_

I think the code gets easier to read when we give symbolic names to some of the operations we're performing. I was sure we needed this when I saw the header being built like this:

...
buf[2] = sizeof(unsigned long)
buf[3] = sizeof(unsigned long)

I really couldn't remember what either field did ;(

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-use-ENTRY_SIZE fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-use-ENTRY_SIZE +7 -5

@@ -508,14 +508,16 @@ struct pagemapread {
 	unsigned long __user *out;
 };
 
+#define PM_ENTRY_BYTES sizeof(unsigned long)
+
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,
 			  struct pagemapread *pm)
 {
 	__put_user(pfn, pm->out);
 	pm->out++;
-	pm->pos += sizeof(unsigned long);
-	pm->count -= sizeof(unsigned long);
 	pm->next = addr + PAGE_SIZE;
+	pm->pos += PM_ENTRY_BYTES;
+	pm->count -= PM_ENTRY_BYTES;
 	return 0;
 }
 

@@ -601,13 +603,13 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EIO;
-	svpfn = src / sizeof(unsigned long);
+	svpfn = src / PM_ENTRY_BYTES;
 	addr = PAGE_SIZE * svpfn;
-	if (svpfn * sizeof(unsigned long) != src)
+	if (svpfn * PM_ENTRY_BYTES != src)
 		goto out;
 	evpfn = min((src + count) / sizeof(unsigned long) - 1,
 		    ((~0UL) >> PAGE_SHIFT) + 1);
-	count = (evpfn - svpfn) * sizeof(unsigned long);
+	count = (evpfn - svpfn) * PM_ENTRY_BYTES;
 	end = PAGE_SIZE * evpfn;
 	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
 

_

Quoted 3 lines

Matt, if you're OK with these, do you mind if I send the update into -mm, or would you like to do it?

Hmmm, is the below working for you? I was having trouble with it.

-1 is a magic number in /proc/$pid/pagemap. It means that there was no pte present for a particular page. We're going to be refining that a bit shortly, so give this a real name for now.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -puN fs/proc/task_mmu.c~give_-1s_a_name fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~give_-1s_a_name +2 -1

@@ -509,6 +509,7 @@ struct pagemapread {
 };
 
 #define PM_ENTRY_BYTES sizeof(unsigned long)
+#define PM_NOT_PRESENT ((unsigned long)-1)
 
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,
 			  struct pagemapread *pm)

@@ -533,7 +534,7 @@ static int pagemap_pte_range(pmd_t *pmd,
 		if (addr < pm->next)
 			continue;
 		if (!pte_present(*pte))
-			err = add_to_pagemap(addr, -1, pm);
+			err = add_to_pagemap(addr, PM_NOT_PRESENT, pm);
 		else
 			err = add_to_pagemap(addr, pte_pfn(*pte), pm);
 		if (err)

_

In addition to understanding which physical pages are used by a process, it would also be very nice to enumerate how much swap space a process is using.

This patch enables /proc/<pid>/pagemap to display swap ptes. In the process, it also changes the constant that we used to indicate non-present ptes before.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |   29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-export-swap-ptes fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-export-swap-ptes +8 -2

@@ -7,6 +7,8 @@
 #include <linux/pagemap.h>
 #include <linux/ptrace.h>
 #include <linux/mempolicy.h>
+#include <linux/swap.h>
+#include <linux/swapops.h>
 
 #include <asm/elf.h>
 #include <asm/uaccess.h>

@@ -506,9 +508,13 @@ struct pagemapread {
 	int index;
 	unsigned long __user *out;
 };
-
 #define PM_ENTRY_BYTES sizeof(unsigned long)
-#define PM_NOT_PRESENT ((unsigned long)-1)
+#define PM_RESERVED_BITS	3
+#define PM_RESERVED_OFFSET	(BITS_PER_LONG-PM_RESERVED_BITS)
+#define PM_RESERVED_MASK	(((1<<PM_RESERVED_BITS)-1) << PM_RESERVED_OFFSET)
+#define PM_SPECIAL(nr)		(((nr) << PM_RESERVED_OFFSET) | PM_RESERVED_MASK)
+#define PM_NOT_PRESENT	PM_SPECIAL(1)
+#define PM_SWAP		PM_SPECIAL(2)
 #define PAGEMAP_END_OF_BUFFER 1
 
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-export-swap-ptes +2 -0

+			pfn = swap_pte_to_pagemap_entry(*pte);
+		else if (pte_present(*pte))
 			pfn = pte_pfn(*pte);
 		err = add_to_pagemap(addr, pfn, pm);
 		if (err)

_

I tried to do this a bit more incrementally, but it ended up just looking like an even worse mess. So, this does a a couple of different things.

1. use page walker pte_hole() helper, which
2. gets rid of the "next" value in "struct pagemapread"
3. allow 1-3 byte reads from pagemap. This at least ensures that we don't write over user memory if they ask us for 1 bytes and we tried to write 4.
4. Instead of trying to calculate what ranges of pages we are going to walk, simply start walking them, then return PAGEMAP_END_OF_BUFFER at the end of the buffer, error out, and stop walking.
5. enforce that reads must be algined to PM_ENTRY_BYTES

Note that, despite these functional additions, and some nice new comments, this patch still removes more code than it adds.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |  123 ++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 67 deletions(-)

diff -puN fs/proc/task_mmu.c~bail-instead-of-tracking fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~bail-instead-of-tracking +1 -1

@@ -501,7 +501,6 @@ const struct file_operations proc_clear_
 };
 
 struct pagemapread {
-	unsigned long next;
 	unsigned long pos;
 	size_t count;
 	int index;

@@ -510,58 +509,64 @@ struct pagemapread {
 
 #define PM_ENTRY_BYTES sizeof(unsigned long)
 #define PM_NOT_PRESENT ((unsigned long)-1)
+#define PAGEMAP_END_OF_BUFFER 1

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~bail-instead-of-tracking +51 -63

 	pm->pos += PM_ENTRY_BYTES;
 	pm->count -= PM_ENTRY_BYTES;
+	if (pm->count <= 0)
+		return PAGEMAP_END_OF_BUFFER;
 	return 0;
 }
 
+static int pagemap_pte_hole(unsigned long start, unsigned long end,
+				void *private)
+{
+	struct pagemapread *pm = private;
+	unsigned long addr;
+	int err = 0;
+	for (addr = start; addr < end; addr += PAGE_SIZE) {
+		err = add_to_pagemap(addr, PM_NOT_PRESENT, pm);
+		if (err)
+			break;
+	}
+	return err;
+}
+
 static int pagemap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 			     void *private)
 {
 	struct pagemapread *pm = private;
 	pte_t *pte;
-	int err;
+	int err = 0;
 
 	pte = pte_offset_map(pmd, addr);
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
-		if (addr < pm->next)
-			continue;
-		if (!pte_present(*pte))
-			err = add_to_pagemap(addr, PM_NOT_PRESENT, pm);
-		else
-			err = add_to_pagemap(addr, pte_pfn(*pte), pm);
+		unsigned long pfn = PM_NOT_PRESENT;
+		if (pte_present(*pte))
+			pfn = pte_pfn(*pte);
+		err = add_to_pagemap(addr, pfn, pm);
 		if (err)
 			return err;
-		if (pm->count == 0)
-			break;
 	}
 	pte_unmap(pte - 1);
 
 	cond_resched();
 
-	return 0;
+	return err;
 }
 
-static int pagemap_fill(struct pagemapread *pm, unsigned long end)
+static struct mm_walk pagemap_walk =
 {
-	int ret;
-
-	while (pm->next != end && pm->count > 0) {
-		ret = add_to_pagemap(pm->next, -1UL, pm);
-		if (ret)
-			return ret;
-	}
-	return 0;
-}
-
-static struct mm_walk pagemap_walk = { .pmd_entry = pagemap_pte_range };
+	.pmd_entry = pagemap_pte_range,
+	.pte_hole = pagemap_pte_hole
+};
 
 /*
  * /proc/pid/pagemap - an array mapping virtual pages to pfns

@@ -589,9 +594,8 @@ static ssize_t pagemap_read(struct file 
 	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
 	unsigned long src = *ppos;
 	struct page **pages, *page;
-	unsigned long addr, end, vend, svpfn, evpfn, uaddr, uend;
+	unsigned long uaddr, uend;
 	struct mm_struct *mm;
-	struct vm_area_struct *vma;
 	struct pagemapread pm;
 	int pagecount;
 	int ret = -ESRCH;

@@ -603,16 +607,10 @@ static ssize_t pagemap_read(struct file 
 	if (!ptrace_may_attach(task))
 		goto out;
 
-	ret = -EIO;
-	svpfn = src / PM_ENTRY_BYTES;
-	addr = PAGE_SIZE * svpfn;
-	if (svpfn * PM_ENTRY_BYTES != src)
+	ret = -EINVAL;
+	/* file position must be aligned */
+	if (*ppos % PM_ENTRY_BYTES)
 		goto out;
-	evpfn = min((src + count) / sizeof(unsigned long) - 1,
-		    ((~0UL) >> PAGE_SHIFT) + 1);
-	count = (evpfn - svpfn) * PM_ENTRY_BYTES;
-	end = PAGE_SIZE * evpfn;
-	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
 
 	ret = 0;
 	mm = get_task_mm(task);

@@ -632,44 +630,36 @@ static ssize_t pagemap_read(struct file 
 			     1, 0, pages, NULL);
 	up_read(&current->mm->mmap_sem);
 
-	//printk("%x(%x):%x %d@%ld (%d pages) -> %d\n", uaddr, buf, uend, count, src, pagecount, ret);
 	if (ret < 0)
 		goto out_free;
 
-	pm.next = addr;
 	pm.pos = src;
 	pm.count = count;
 	pm.out = (unsigned long __user *)buf;
 
-	down_read(&mm->mmap_sem);
-	vma = find_vma(mm, pm.next);
-	while (pm.count > 0 && vma) {
-		if (!ptrace_may_attach(task)) {
-			ret = -EIO;
-			up_read(&mm->mmap_sem);
-			goto out_release;
-		}
-		vend = min(vma->vm_end - 1, end - 1) + 1;
-		ret = pagemap_fill(&pm, vend);
-		if (ret || !pm.count)
-			break;
-		vend = min(vma->vm_end - 1, end - 1) + 1;
-		ret = walk_page_range(mm, vma->vm_start, vend,
-				      &pagemap_walk, &pm);
-		vma = vma->vm_next;
+	if (!ptrace_may_attach(task)) {
+		ret = -EIO;
+	} else {
+		unsigned long src = *ppos;
+		unsigned long svpfn = src / PM_ENTRY_BYTES;
+		unsigned long start_vaddr = svpfn << PAGE_SHIFT;
+		unsigned long end_vaddr = TASK_SIZE_OF(task);
+		/*
+		 * The odds are that this will stop walking way
+		 * before end_vaddr, because the length of the
+		 * user buffer is tracked in "pm", and the walk
+		 * will stop when we hit the end of the buffer.
+		 */
+		ret = walk_page_range(mm, start_vaddr, end_vaddr,
+					&pagemap_walk, &pm);
+		if (ret == PAGEMAP_END_OF_BUFFER)
+			ret = 0;
+		/* don't need mmap_sem for these, but this looks cleaner */
+		*ppos = pm.pos;
+		if (!ret)
+			ret = pm.pos - src;
 	}
-	up_read(&mm->mmap_sem);
-
-	//printk("before fill at %ld\n", pm.pos);
-	ret = pagemap_fill(&pm, end);
-
-	printk("after fill at %ld\n", pm.pos);
-	*ppos = pm.pos;
-	if (!ret)
-		ret = pm.pos - src;
 
-out_release:
-	printk("releasing pages\n");
 	for (; pagecount; pagecount--) {
 		page = pages[pagecount-1];
 		if (!PageReserved(page))

@@ -682,7 +672,6 @@ out_free:
 out_task:
 	put_task_struct(task);
 out:
-	printk("returning\n");
 	return ret;
 }
 

_

This is a series of patches to update /proc/pid/pagemap, to simplify the code, and fix bugs which caused userspace memory corruption.

Since it is just in -mm, we should probably roll all of these together and just update it all at once, or send a simple drop-on replacement patch. These patches are all here mostly to help with review.

Matt, if you're OK with these, do you mind if I send the update into -mm, or would you like to do it?

--
From: Matt Mackall <[email protected]>

Quoted 6 lines · Dave Boutcher

Matt, this patch set replaces the two patches I sent earlier and contains additional fixes. I've done some reasonably rigorous testing on x86_64, but not on a 32 bit arch. I'm pretty sure this isn't worse than what's in mm right now, which has some user-space corruption and a nasty infinite kernel loop. YMMV.

Dave, here's my current work-in-progress patch to deal with a couple locking issues, primarily a possible deadlock on the mm semaphore that can occur if someone unmaps the target buffer while we're walking the tree. It currently hangs on my box and I haven't had any free cycles to finish debugging it, but you might want to take a peek at it.

Signed-off-by Dave Hansen <[email protected]>

---

 lxc-dave/fs/proc/task_mmu.c |  121 ++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 66 deletions(-)

diff -puN fs/proc/task_mmu.c~Re-_PATCH_0_3_proc_pid_pagemap_fixes fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~Re-_PATCH_0_3_proc_pid_pagemap_fixes +55 -66

@@ -501,37 +501,21 @@ const struct file_operations proc_clear_
 };
 
 struct pagemapread {
-	struct mm_struct *mm;
 	unsigned long next;
-	unsigned long *buf;
-	pte_t *ptebuf;
 	unsigned long pos;
 	size_t count;
 	int index;
-	char __user *out;
+	unsigned long __user *out;
 };
 
-static int flush_pagemap(struct pagemapread *pm)
-{
-	int n = min(pm->count, pm->index * sizeof(unsigned long));
-	if (copy_to_user(pm->out, pm->buf, n))
-		return -EFAULT;
-	pm->out += n;
-	pm->pos += n;
-	pm->count -= n;
-	pm->index = 0;
-	cond_resched();
-	return 0;
-}
-
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,
 			  struct pagemapread *pm)
 {
-	pm->buf[pm->index++] = pfn;
+	__put_user(pfn, pm->out);
+	pm->out++;
+	pm->pos += sizeof(unsigned long);
+	pm->count -= sizeof(unsigned long);
 	pm->next = addr + PAGE_SIZE;
-	if (pm->index * sizeof(unsigned long) >= PAGE_SIZE ||
-	    pm->index * sizeof(unsigned long) >= pm->count)
-		return flush_pagemap(pm);
 	return 0;
 }
 

@@ -543,14 +527,6 @@ static int pagemap_pte_range(pmd_t *pmd,
 	int err;
 
 	pte = pte_offset_map(pmd, addr);
-
-#ifdef CONFIG_HIGHPTE
-	/* copy PTE directory to temporary buffer and unmap it */
-	memcpy(pm->ptebuf, pte, PAGE_ALIGN((unsigned long)pte) - (unsigned long)pte);
-	pte_unmap(pte);
-	pte = pm->ptebuf;
-#endif
-
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
 		if (addr < pm->next)
 			continue;

@@ -560,11 +536,12 @@ static int pagemap_pte_range(pmd_t *pmd,
 			err = add_to_pagemap(addr, pte_pfn(*pte), pm);
 		if (err)
 			return err;
+		if (pm->count == 0)
+			break;
 	}
-
-#ifndef CONFIG_HIGHPTE
 	pte_unmap(pte - 1);
-#endif
+
+	cond_resched();
 
 	return 0;
 }

@@ -573,7 +550,7 @@ static int pagemap_fill(struct pagemapre
 {
 	int ret;
 
-	while (pm->next != end) {
+	while (pm->next != end && pm->count > 0) {
 		ret = add_to_pagemap(pm->next, -1UL, pm);
 		if (ret)
 			return ret;

@@ -608,15 +585,16 @@ static ssize_t pagemap_read(struct file 
 {
 	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
 	unsigned long src = *ppos;
-	unsigned long *page;
-	unsigned long addr, end, vend, svpfn, evpfn;
+	struct page **pages, *page;
+	unsigned long addr, end, vend, svpfn, evpfn, uaddr, uend;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
 	struct pagemapread pm;
+	int pagecount;
 	int ret = -ESRCH;
 
 	if (!task)
-		goto out_no_task;
+		goto out;
 
 	ret = -EACCES;
 	if (!ptrace_may_attach(task))

@@ -628,39 +606,43 @@ static ssize_t pagemap_read(struct file 
 	if ((svpfn + 1) * sizeof(unsigned long) != src)
 		goto out;
 	evpfn = min((src + count) / sizeof(unsigned long) - 1,
-		    ((~0UL) >> PAGE_SHIFT) + 1);
+		    ((~0UL) >> PAGE_SHIFT) + 1) - 1;
 	count = (evpfn - svpfn) * sizeof(unsigned long);
 	end = PAGE_SIZE * evpfn;
-
-	ret = -ENOMEM;
-	page = kzalloc(PAGE_SIZE, GFP_USER);
-	if (!page)
-		goto out;
-
-#ifdef CONFIG_HIGHPTE
-	pm.ptebuf = kzalloc(PAGE_SIZE, GFP_USER);
-	if (!pm.ptebuf)
-		goto out_free;
-#endif
+	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
 
 	ret = 0;
 	mm = get_task_mm(task);
 	if (!mm)
-		goto out_freepte;
+		goto out;
+
+	ret = -ENOMEM;
+	uaddr = (unsigned long)buf & ~(PAGE_SIZE-1);
+	uend = (unsigned long)(buf + count);
+	pagecount = (uend - uaddr + PAGE_SIZE-1) / PAGE_SIZE;
+	pages = kmalloc(pagecount * sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		goto out_task;
+
+	down_read(&current->mm->mmap_sem);
+	ret = get_user_pages(current, current->mm, uaddr, pagecount,
+			     1, 0, pages, NULL);
+	up_read(&current->mm->mmap_sem);
+
+	//printk("%x(%x):%x %d@%ld (%d pages) -> %d\n", uaddr, buf, uend, count, src, pagecount, ret);
+	if (ret < 0)
+		goto out_free;
 
-	pm.mm = mm;
 	pm.next = addr;
-	pm.buf = page;
 	pm.pos = src;
 	pm.count = count;
-	pm.index = 0;
-	pm.out = buf;
+	pm.out = (unsigned long __user *)buf;
 
 	if (svpfn == -1) {
-		((char *)page)[0] = (ntohl(1) != 1);
-		((char *)page)[1] = PAGE_SHIFT;
-		((char *)page)[2] = sizeof(unsigned long);
-		((char *)page)[3] = sizeof(unsigned long);
+		put_user((char)(ntohl(1) != 1), buf);
+		put_user((char)PAGE_SHIFT, buf + 1);
+		put_user((char)sizeof(unsigned long), buf + 2);
+		put_user((char)sizeof(unsigned long), buf + 3);
 		add_to_pagemap(pm.next, page[0], &pm);
 	}
 

@@ -669,7 +651,8 @@ static ssize_t pagemap_read(struct file 
 	while (pm.count > 0 && vma) {
 		if (!ptrace_may_attach(task)) {
 			ret = -EIO;
-			goto out_mm;
+			up_read(&mm->mmap_sem);
+			goto out_release;
 		}
 		vend = min(vma->vm_end - 1, end - 1) + 1;
 		ret = pagemap_fill(&pm, vend);

@@ -682,23 +665,29 @@ static ssize_t pagemap_read(struct file 
 	}
 	up_read(&mm->mmap_sem);
 
+	//printk("before fill at %ld\n", pm.pos);
 	ret = pagemap_fill(&pm, end);
 
+	printk("after fill at %ld\n", pm.pos);
 	*ppos = pm.pos;
 	if (!ret)
 		ret = pm.pos - src;
 
-out_mm:
+out_release:
+	printk("releasing pages\n");
+	for (; pagecount; pagecount--) {
+		page = pages[pagecount-1];
+		if (!PageReserved(page))
+			SetPageDirty(page);
+		page_cache_release(page);
+	}
 	mmput(mm);
-out_freepte:
-#ifdef CONFIG_HIGHPTE
-	kfree(pm.ptebuf);
 out_free:
-#endif
-	kfree(page);
-out:
+	kfree(pages);
+out_task:
 	put_task_struct(task);
-out_no_task:
+out:
+	printk("returning\n");
 	return ret;
 }
 

_

Quoted 5 lines · Dave Hansen

In addition to understanding which physical pages are used by a process, it would also be very nice to enumerate how much swap space a process is using.

Quoted 4 lines

This patch enables /proc/<pid>/pagemap to display swap ptes. In the process, it also changes the constant that we used to indicate non-present ptes before.

Nice. Can you update the doc comment on pagemap_read to match?

fs/proc/task_mmu.c static int pagemap_pte_hole(unsigned lon · line 550

Quoted 3 lines

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-export-swap-ptes +3 -0

+unsigned long swap_pte_to_pagemap_entry(pte_t pte)
+{
+	unsigned long ret = 0;

Unused assignment?

fs/proc/task_mmu.c static int pagemap_pte_hole(unsigned lon · line 558

Quoted 8 lines

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-export-swap-ptes +8 -0

+	swp_entry_t entry = pte_to_swp_entry(pte);
+	unsigned long offset;
+	unsigned long swap_file_nr;
+
+	offset = swp_offset(entry);
+	swap_file_nr = swp_type(entry);
+	ret = PM_SWAP | swap_file_nr | (offset << MAX_SWAPFILES_SHIFT);
+	return ret;

How about just return <expression>?

This is a little problematic as we've added another not very visible magic number to the mix. We're also not masking off swp_offset to avoid colliding with our reserved bits. And we're also unpacking an arch-independent value (swp_entry_t) just to repack it in more or less the same shape? Or are we reversing the fields?

fs/proc/task_mmu.c static int pagemap_pte_range(pmd_t *pmd, · line 573

Quoted 9 lines

static int pagemap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~pagemap-export-swap-ptes +1 -1

 			     void *private)
 {

@@ -549,7 +570,9 @@ static int pagemap_pte_range(pmd_t *pmd,
 	pte = pte_offset_map(pmd, addr);
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
 		unsigned long pfn = PM_NOT_PRESENT;
-		if (pte_present(*pte))
+		if (is_swap_pte(*pte))

Hmm, unlikely?

fs/proc/task_mmu.c struct pagemapread { · line 520

Quoted 1 line

fs/proc/task_mmu.c renamed from fs/proc/task_mmu.c~bail-instead-of-tracking +1 -0

+	copy_to_user(pm->out, &pfn, out_len);

And I think we want to keep the put_user in the fast path.

Quoted 7 lines · Dave Hansen

lib/pagewalk.c renamed from lib/pagewalk.c~pagewalk-empty-ranges +1 -1

@@ -27,25 +23,23 @@ static int walk_pmd_range(pud_t *pud, un
 {
 	pmd_t *pmd;
 	unsigned long next;
-	int err;
+	int err = 0;
 

lib/pagewalk.c static int walk_pmd_range(pud_t *pud, un · line 30

Quoted 3 lines

lib/pagewalk.c renamed from lib/pagewalk.c~pagewalk-empty-ranges +0 -0

 	for (pmd = pmd_offset(pud, addr); addr != end;
 	     pmd++, addr = next) {
 		next = pmd_addr_end(addr, end);

While you're there, do you mind fixing the actual page table walking so that it follows the normal form?