[PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[Thomas Tuttle]

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

Since kpagecount and kpageflags require reads in multiples of 8, and
it simplifies add_to_pagemap significantly, I added the same
requirement to /proc/pid/pagemap.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Make-pagemap_read-enforce-reading-in-multiples-of-8.patch --]
[-- Type: text/x-patch; name=0003-Make-pagemap_read-enforce-reading-in-multiples-of-8.patch, Size: 1800 bytes --]

From 25908731b67069d6cf56d8af522b55479ea75736 Mon Sep 17 00:00:00 2001
From: Thomas Tuttle <ttuttle@google.com>
Date: Thu, 5 Jun 2008 10:45:55 -0400
Subject: [PATCH] Make pagemap_read enforce reading in multiples of 8.

Previously, /proc/pid/pagemap would allow reads of lengths that were not
multiples of 8 to succeed (and would return correct data), but had no way
of reading the remainder of the partial value returned.

/proc/kpage{count,flags} are also arrays of u64s, and they require that
reads are a multiple of 8.  This patch makes /proc/pid/pagemap consistent.

Signed-off-by: Thomas Tuttle <ttuttle@google.com>
---
 fs/proc/task_mmu.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9915202..3f6d680 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -519,21 +519,11 @@ struct pagemapread {
 static int add_to_pagemap(unsigned long addr, u64 pfn,
 			  struct pagemapread *pm)
 {
-	/*
-	 * Make sure there's room in the buffer for an
-	 * entire entry.  Otherwise, only copy part of
-	 * the pfn.
-	 */
-	if (pm->out + PM_ENTRY_BYTES >= pm->end) {
-		if (copy_to_user(pm->out, &pfn, pm->end - pm->out))
-			return -EFAULT;
-		pm->out = pm->end;
-		return PM_END_OF_BUFFER;
-	}
-
 	if (copy_to_user(pm->out, &pfn, PM_ENTRY_BYTES))
 		return -EFAULT;
 	pm->out += PM_ENTRY_BYTES;
+	if (pm->out >= pm->end)
+		return PM_END_OF_BUFFER;
 	return 0;
 }
 
@@ -633,8 +623,8 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
 		goto out_task;
 
 	ret = -EINVAL;
-	/* file position must be aligned */
-	if (*ppos % PM_ENTRY_BYTES)
+	/* file position and count must be aligned */
+	if ((*ppos % PM_ENTRY_BYTES) || (count % PM_ENTRY_BYTES))
 		goto out_task;
 
 	ret = 0;
-- 
1.5.3.6

Re: [PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[Matt Mackall]

On Thu, 2008-06-05 at 11:06 -0400, Thomas Tuttle wrote:
> Since kpagecount and kpageflags require reads in multiples of 8, and
> it simplifies add_to_pagemap significantly, I added the same
> requirement to /proc/pid/pagemap.

I'm generally fine with this. Another approach that's perhaps more
friendly is when someone tries to do a 24-byte read, do a 16-byte read,
leaving the file pointer aligned. Not sure if that's completely kosher
though.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[Thomas Tuttle]

On Thu, Jun 5, 2008 at 12:03 PM, Matt Mackall <mpm@selenic.com> wrote:
>
> On Thu, 2008-06-05 at 11:06 -0400, Thomas Tuttle wrote:
>> Since kpagecount and kpageflags require reads in multiples of 8, and
>> it simplifies add_to_pagemap significantly, I added the same
>> requirement to /proc/pid/pagemap.
>
> I'm generally fine with this. Another approach that's perhaps more
> friendly is when someone tries to do a 24-byte read, do a 16-byte read,
> leaving the file pointer aligned. Not sure if that's completely kosher
> though.

This doesn't require that they read exactly 8 bytes, just that they
read some multiple of 8 bytes.  (They can read 8, 16, 24, etc..., just
not something like 12.)

--ttuttle

Re: [PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[Matt Mackall]

On Thu, 2008-06-05 at 12:05 -0400, Thomas Tuttle wrote:
> On Thu, Jun 5, 2008 at 12:03 PM, Matt Mackall <mpm@selenic.com> wrote:
> >
> > On Thu, 2008-06-05 at 11:06 -0400, Thomas Tuttle wrote:
> >> Since kpagecount and kpageflags require reads in multiples of 8, and
> >> it simplifies add_to_pagemap significantly, I added the same
> >> requirement to /proc/pid/pagemap.
> >
> > I'm generally fine with this. Another approach that's perhaps more
> > friendly is when someone tries to do a 24-byte read, do a 16-byte read,
> > leaving the file pointer aligned. Not sure if that's completely kosher
> > though.
> 
> This doesn't require that they read exactly 8 bytes, just that they
> read some multiple of 8 bytes.  (They can read 8, 16, 24, etc..., just
> not something like 12.)

Yes, I got that, I'm just mathematically impaired this morning. Read
that as "when someone tries to do a *20* byte read, do a 16-byte read.."
In other words, round down to the nearest multiple of 8.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[Thomas Tuttle]

On Thu, Jun 5, 2008 at 12:09 PM, Matt Mackall <mpm@selenic.com> wrote:
> Yes, I got that, I'm just mathematically impaired this morning. Read
> that as "when someone tries to do a *20* byte read, do a 16-byte read.."
> In other words, round down to the nearest multiple of 8.

Oh.  I would seriously question doing that, because it would allow
reads that are improperly split up to fail silently.  What happens if
someone tries to read 40 bytes, and (for some reason) it gets split
into two 20-byte reads?  Instead of "12345" they'll get "12.34."
(where . is 4 zero or garbage bytes).  I'd rather they get an error.

--ttuttle

Re: [PATCH 3/5] pagemap: Make pagemap_read enforce reading in multiples of 8

[Matt Mackall]

On Thu, 2008-06-05 at 12:14 -0400, Thomas Tuttle wrote:
> On Thu, Jun 5, 2008 at 12:09 PM, Matt Mackall <mpm@selenic.com> wrote:
> > Yes, I got that, I'm just mathematically impaired this morning. Read
> > that as "when someone tries to do a *20* byte read, do a 16-byte read.."
> > In other words, round down to the nearest multiple of 8.
> 
> Oh.  I would seriously question doing that, because it would allow
> reads that are improperly split up to fail silently.  What happens if
> someone tries to read 40 bytes, and (for some reason) it gets split
> into two 20-byte reads?  Instead of "12345" they'll get "12.34."
> (where . is 4 zero or garbage bytes).  I'd rather they get an error.

I suppose you're right, people usually don't check those return codes.

-- 
Mathematics is the supreme nostalgia of our time.