[PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Thomas Tuttle]

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

While fiddling with pagemap, I discovered a bug in add_to_pagemap.
When it is copying an entry that is not at the end of the buffer, it
uses put_user to copy a u64 into a char* buffer.  The problem is that
put_user determines how much to copy based on the size of the
*destination*, not the source, so it only copied one byte.  To fix
this, I replaced the call to put_user with a call to copy_to_user, as
is used when copying the last (possibly partial) PFN into the buffer.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Modified-add_to_pagemap-to-use-copy_to_user-instead.patch --]
[-- Type: text/x-patch; name=0001-Modified-add_to_pagemap-to-use-copy_to_user-instead.patch, Size: 866 bytes --]

From 3240be0d489e914bbb5a559c21413e39889934ef Mon Sep 17 00:00:00 2001
From: Thomas Tuttle <ttuttle@google.com>
Date: Thu, 5 Jun 2008 09:37:24 -0400
Subject: [PATCH] Modified add_to_pagemap to use copy_to_user instead of put_user.
 Using put_user only copies the low byte of the pagecount, because the
 destination buffer is a char*.

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

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 88717c0..9915202 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -531,7 +531,7 @@ static int add_to_pagemap(unsigned long addr, u64 pfn,
 		return PM_END_OF_BUFFER;
 	}
 
-	if (put_user(pfn, pm->out))
+	if (copy_to_user(pm->out, &pfn, PM_ENTRY_BYTES))
 		return -EFAULT;
 	pm->out += PM_ENTRY_BYTES;
 	return 0;
-- 
1.5.3.6

Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Matt Mackall]

On Thu, 2008-06-05 at 11:04 -0400, Thomas Tuttle wrote:
> While fiddling with pagemap, I discovered a bug in add_to_pagemap.
> When it is copying an entry that is not at the end of the buffer, it
> uses put_user to copy a u64 into a char* buffer.  The problem is that
> put_user determines how much to copy based on the size of the
> *destination*, not the source, so it only copied one byte.  To fix
> this, I replaced the call to put_user with a call to copy_to_user, as
> is used when copying the last (possibly partial) PFN into the buffer.

This looks fine to me, so:

Acked-by: Matt Mackall <mpm@selenic.com>

But your 3/5 undoes this, right? So we should just take one or the other
route. While I like going the simplifying route, it's not very pretty
from the user interface point of view. But it does have plenty of
precedent in direct-I/O-like things.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Thomas Tuttle]

On Thu, Jun 5, 2008 at 11:57 AM, Matt Mackall <mpm@selenic.com> wrote:
>
> On Thu, 2008-06-05 at 11:04 -0400, Thomas Tuttle wrote:
>> While fiddling with pagemap, I discovered a bug in add_to_pagemap.
>> When it is copying an entry that is not at the end of the buffer, it
>> uses put_user to copy a u64 into a char* buffer.  The problem is that
>> put_user determines how much to copy based on the size of the
>> *destination*, not the source, so it only copied one byte.  To fix
>> this, I replaced the call to put_user with a call to copy_to_user, as
>> is used when copying the last (possibly partial) PFN into the buffer.0
>
> This looks fine to me, so:
>
> Acked-by: Matt Mackall <mpm@selenic.com>
>
> But your 3/5 undoes this, right? So we should just take one or the other
> route. While I like going the simplifying route, it's not very pretty
> from the user interface point of view. But it does have plenty of
> precedent in direct-I/O-like things.

No, my 3/5 further keeps the use of copy_to_user instead of put_user,
but eliminates entirely the code for partial reads.  3/5 depends on
1/5.

--ttuttle

Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Matt Mackall]

On Thu, 2008-06-05 at 12:01 -0400, Thomas Tuttle wrote:
> On Thu, Jun 5, 2008 at 11:57 AM, Matt Mackall <mpm@selenic.com> wrote:
> >
> > On Thu, 2008-06-05 at 11:04 -0400, Thomas Tuttle wrote:
> >> While fiddling with pagemap, I discovered a bug in add_to_pagemap.
> >> When it is copying an entry that is not at the end of the buffer, it
> >> uses put_user to copy a u64 into a char* buffer.  The problem is that
> >> put_user determines how much to copy based on the size of the
> >> *destination*, not the source, so it only copied one byte.  To fix
> >> this, I replaced the call to put_user with a call to copy_to_user, as
> >> is used when copying the last (possibly partial) PFN into the buffer.0
> >
> > This looks fine to me, so:
> >
> > Acked-by: Matt Mackall <mpm@selenic.com>
> >
> > But your 3/5 undoes this, right? So we should just take one or the other
> > route. While I like going the simplifying route, it's not very pretty
> > from the user interface point of view. But it does have plenty of
> > precedent in direct-I/O-like things.
> 
> No, my 3/5 further keeps the use of copy_to_user instead of put_user,
> but eliminates entirely the code for partial reads.  3/5 depends on
> 1/5.

Hmm, I read this backwards the first time through then. Rather than
finding a small bug at the end of the buffer, you've found a REALLY BIG
WEAR-A-BROWN-PAPER-BAG-ON-MY-HEAD BUG.

Sigh. Some clever person (me?) probably removed a cast or changed a
pointer type somewhere along the way and didn't notice the fallout
because the MSBs still came through.

I'm pretty sure we want to do the put_user because it's faster. On a
64-bit system, it boils down to a simple assignment. If we commit
ourselves to the alignment thing, then we should probably just change
all the relevant pointers to u64, and then it will just do the right
thing.

ps: perhaps put_user can be made to compare the src and dst sizes at
compile time?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Andrew Morton]

On Thu, 5 Jun 2008 11:04:21 -0400
"Thomas Tuttle" <ttuttle@google.com> wrote:

> While fiddling with pagemap, I discovered a bug in add_to_pagemap.

so...  afaict we have some fairly significant problems in here and a
version 2 of the patchset is in order?

If so, when preparing that patchset please do have a think about which
of the fixes you believe should be backported into 2.6.25.x and prepare
the patches so that those fixes come first, to ease that backporting.

Thanks.

Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Matt Mackall]

On Thu, 2008-06-05 at 12:02 -0700, Andrew Morton wrote:
> On Thu, 5 Jun 2008 11:04:21 -0400
> "Thomas Tuttle" <ttuttle@google.com> wrote:
> 
> > While fiddling with pagemap, I discovered a bug in add_to_pagemap.
> 
> so...  afaict we have some fairly significant problems in here and a
> version 2 of the patchset is in order?
> 
> If so, when preparing that patchset please do have a think about which
> of the fixes you believe should be backported into 2.6.25.x and prepare
> the patches so that those fixes come first, to ease that backporting.

For 2.6.25.x, we should probably just add a cast to the put_user dest.
Thomas?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.

[Thomas Tuttle]

On Thu, Jun 5, 2008 at 3:06 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Thu, 2008-06-05 at 12:02 -0700, Andrew Morton wrote:
>> On Thu, 5 Jun 2008 11:04:21 -0400
>> "Thomas Tuttle" <ttuttle@google.com> wrote:
>>
>> > While fiddling with pagemap, I discovered a bug in add_to_pagemap.
>>
>> so...  afaict we have some fairly significant problems in here and a
>> version 2 of the patchset is in order?
>>
>> If so, when preparing that patchset please do have a think about which
>> of the fixes you believe should be backported into 2.6.25.x and prepare
>> the patches so that those fixes come first, to ease that backporting.
>
> For 2.6.25.x, we should probably just add a cast to the put_user dest.
> Thomas?

Okay, I've written a new patchset.  Patch 1/4 simply changes things to
use u64 pointers.  It's not that big a patch, but if you're really
uncomfortable backporting it, just stick a (u64*) before pm->out in
2.6.25.x.

Other than that, I don't believe any of the other fixes need to be
backported.  They just tweak a couple of things (error code on
unaligned reads, and which values show up in kpagecount), but don't
repair any particularly broken bits.

Please take a look at the new patchset and see if it fixes things to
your satisfaction.

--ttuttle