[PATCH 1/4] pagemap: Require reads of /proc/pid/pagemap to be multiples of 8 (v2 of series)

[PATCH 1/4] pagemap: Require reads of /proc/pid/pagemap to be multiples of 8 (v2 of series)

[Thomas Tuttle]

This matches the behavior of /proc/kpage{count,flags}, and simplifies
the logic a bit.

I also changed out and end in struct pagemapread to be u64* instead of
char*, which makes put_user work the way it was intended.  (Before, it
was only copying the bottom byte of a pagemap entry, because the target
of the copy was a char*.)

Signed-off-by: Thomas Tuttle
---
 fs/proc/task_mmu.c |   28 +++++++++-------------------
 1 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 88717c0..01f9bff 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -496,7 +496,7 @@ const struct file_operations proc_clear_refs_operations = {
 };

 struct pagemapread {
-       char __user *out, *end;
+       u64 __user *out, *end;
 };

 #define PM_ENTRY_BYTES      sizeof(u64)
@@ -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 (put_user(pfn, pm->out))
                return -EFAULT;
-       pm->out += PM_ENTRY_BYTES;
+       pm->out++;
+       if (pm->out >= pm->end)
+               return PM_END_OF_BUFFER;
        return 0;
 }

@@ -634,7 +624,7 @@ static ssize_t pagemap_read(struct file *file,
char __user * buf,

        ret = -EINVAL;
        /* file position must be aligned */
-       if (*ppos % PM_ENTRY_BYTES)
+       if ((*ppos % PM_ENTRY_BYTES) || (count % PM_ENTRY_BYTES))
                goto out_task;

        ret = 0;
@@ -664,8 +654,8 @@ static ssize_t pagemap_read(struct file *file,
char __user * buf,
                goto out_pages;
        }

-       pm.out = buf;
-       pm.end = buf + count;
+       pm.out = (u64*)buf;
+       pm.end = (u64*)(buf + count);

        if (!ptrace_may_attach(task)) {
                ret = -EIO;
@@ -690,9 +680,9 @@ static ssize_t pagemap_read(struct file *file,
char __user * buf,
                if (ret == PM_END_OF_BUFFER)
                        ret = 0;
                /* don't need mmap_sem for these, but this looks cleaner */
-               *ppos += pm.out - buf;
+               *ppos += (char*)pm.out - buf;
                if (!ret)
-                       ret = pm.out - buf;
+                       ret = (char*)pm.out - buf;
        }

 out_pages:
--
1.5.3.6

Re: [PATCH 1/4] pagemap: Require reads of /proc/pid/pagemap to be multiples of 8 (v2 of series)

[Andrew Morton]

On Thu, 5 Jun 2008 14:38:10 -0400
"Thomas Tuttle" <ttuttle@google.com> wrote:

> This matches the behavior of /proc/kpage{count,flags}, and simplifies
> the logic a bit.
> 
> I also changed out and end in struct pagemapread to be u64* instead of
> char*, which makes put_user work the way it was intended.  (Before, it
> was only copying the bottom byte of a pagemap entry, because the target
> of the copy was a char*.)

This one is for 2.6.25.x?

Is so, the maintainers are going to wonder why the heck we sent it to
them.  It does fix a bug, but it is far from clear what that bug is.

Can we have a better description please?

> Signed-off-by: Thomas Tuttle

Please include <ttuttle@google.com> in the signoffs.

Re: [PATCH 1/4] pagemap: Require reads of /proc/pid/pagemap to be multiples of 8 (v2 of series)

[Matt Mackall]

On Thu, 2008-06-05 at 12:37 -0700, Andrew Morton wrote:
> On Thu, 5 Jun 2008 14:38:10 -0400
> "Thomas Tuttle" <ttuttle@google.com> wrote:
> 
> > This matches the behavior of /proc/kpage{count,flags}, and simplifies
> > the logic a bit.
> > 
> > I also changed out and end in struct pagemapread to be u64* instead of
> > char*, which makes put_user work the way it was intended.  (Before, it
> > was only copying the bottom byte of a pagemap entry, because the target
> > of the copy was a char*.)
> 
> This one is for 2.6.25.x?

This one is for 2.6.26. Something more like this for 2.6.25.x:

Because put_user bases its copy size on the size of the target pointer,
not the source, it was copying only 1 byte rather than the intended 8.

Spotted-by: Thomas Tuttle <ttuttle@google.com>
Signed-off-by: Matt Mackall <mpm@selenic.com>

diff -r 5030869d9ded fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c	Thu Jun 05 04:01:40 2008 +0000
+++ b/fs/proc/task_mmu.c	Thu Jun 05 15:45:00 2008 -0500
@@ -531,7 +531,7 @@
 		return PM_END_OF_BUFFER;
 	}
 
-	if (put_user(pfn, pm->out))
+	if (put_user(pfn, (u64 *)pm->out))
 		return -EFAULT;
 	pm->out += PM_ENTRY_BYTES;
 	return 0;


-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1/4] pagemap: Require reads of /proc/pid/pagemap to be multiples of 8 (v2 of series)

[Andrew Morton]

On Thu, 05 Jun 2008 15:51:59 -0500
Matt Mackall <mpm@selenic.com> wrote:

> 
> On Thu, 2008-06-05 at 12:37 -0700, Andrew Morton wrote:
> > On Thu, 5 Jun 2008 14:38:10 -0400
> > "Thomas Tuttle" <ttuttle@google.com> wrote:
> > 
> > > This matches the behavior of /proc/kpage{count,flags}, and simplifies
> > > the logic a bit.
> > > 
> > > I also changed out and end in struct pagemapread to be u64* instead of
> > > char*, which makes put_user work the way it was intended.  (Before, it
> > > was only copying the bottom byte of a pagemap entry, because the target
> > > of the copy was a char*.)
> > 
> > This one is for 2.6.25.x?
> 
> This one is for 2.6.26. Something more like this for 2.6.25.x:
> 
> Because put_user bases its copy size on the size of the target pointer,
> not the source, it was copying only 1 byte rather than the intended 8.
> 
> Spotted-by: Thomas Tuttle <ttuttle@google.com>
> Signed-off-by: Matt Mackall <mpm@selenic.com>
> 
> diff -r 5030869d9ded fs/proc/task_mmu.c
> --- a/fs/proc/task_mmu.c	Thu Jun 05 04:01:40 2008 +0000
> +++ b/fs/proc/task_mmu.c	Thu Jun 05 15:45:00 2008 -0500
> @@ -531,7 +531,7 @@
>  		return PM_END_OF_BUFFER;
>  	}
>  
> -	if (put_user(pfn, pm->out))
> +	if (put_user(pfn, (u64 *)pm->out))
>  		return -EFAULT;
>  	pm->out += PM_ENTRY_BYTES;
>  	return 0;
> 
> 

OK.  I can't merge that I guess, so could someome please prepare a
formal patch for the stable guys?  Preferably one which remembers
to add __user to that cast :)