[PATCH] get_user_pages(..., write==1, ...) may return with readable pte.

[PATCH] get_user_pages(..., write==1, ...) may return with readable pte.

[Robin Holt]

Handle the case in get_user_pages() when a call to __handle_mm_fault()
inserts a writable pte, and a process doing dup_mmap converts it
to readable before get_user_pages() does the subsequent request to
follow_page().


Signed-off-by: Robin Holt <holt@sgi.com>

---

Hugh, Nick, and Linus,

I think I have tripped over another flavor of a get_user_pages bug
we addressed back in 2005.  I do not have a test case to prove it is
the issue I am trying to address, but I have done as thorough a code
walk-through as I can.

Assume a pte is currently empty.  A first pthread is in the kernel on
a call path which is leading to get_user_pages.  A second pthread is
in the process of doing a fork.  The process doing get_user_pages()
gets into __handle_mm_fault() and grabs ptl just before the process
doing a fork attempts to grab the ptl to convert the pages to COW.
__handle_mm_fault() will insert the writable pte and unlock ptl then
return with VM_FAULT_WRITE set.  The process doing a fork then gets
the lock and starts converting the pte to RO/COW.  The get_user_pages()
process then clears FOLL_WRITE from foll_flags and calls follow_page()
without write, adds to the map count for the page, but does not have a
writable mapping.


Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c	2006-10-06 12:06:25.000000000 -0500
+++ linux-2.6/mm/memory.c	2006-10-13 15:06:38.286230638 -0500
@@ -1063,15 +1063,7 @@ int get_user_pages(struct task_struct *t
 				int ret;
 				ret = __handle_mm_fault(mm, vma, start,
 						foll_flags & FOLL_WRITE);
-				/*
-				 * The VM_FAULT_WRITE bit tells us that do_wp_page has
-				 * broken COW when necessary, even if maybe_mkwrite
-				 * decided not to set pte_write. We can thus safely do
-				 * subsequent page lookups as if they were reads.
-				 */
-				if (ret & VM_FAULT_WRITE)
-					foll_flags &= ~FOLL_WRITE;
-				
+
 				switch (ret & ~VM_FAULT_WRITE) {
 				case VM_FAULT_MINOR:
 					tsk->min_flt++;

Re: [PATCH] get_user_pages(..., write==1, ...) may return with readable pte.

[Nick Piggin]

On Fri, Oct 13, 2006 at 03:33:42PM -0500, Robin Holt wrote:
> Handle the case in get_user_pages() when a call to __handle_mm_fault()
> inserts a writable pte, and a process doing dup_mmap converts it
> to readable before get_user_pages() does the subsequent request to
> follow_page().
> 
> 
> Signed-off-by: Robin Holt <holt@sgi.com>
> 
> ---
> 
> Hugh, Nick, and Linus,
> 
> I think I have tripped over another flavor of a get_user_pages bug
> we addressed back in 2005.  I do not have a test case to prove it is
> the issue I am trying to address, but I have done as thorough a code
> walk-through as I can.
> 
> Assume a pte is currently empty.  A first pthread is in the kernel on
> a call path which is leading to get_user_pages.  A second pthread is
> in the process of doing a fork.  The process doing get_user_pages()
> gets into __handle_mm_fault() and grabs ptl just before the process
> doing a fork attempts to grab the ptl to convert the pages to COW.
> __handle_mm_fault() will insert the writable pte and unlock ptl then
> return with VM_FAULT_WRITE set.  The process doing a fork then gets
> the lock and starts converting the pte to RO/COW.  The get_user_pages()
> process then clears FOLL_WRITE from foll_flags and calls follow_page()
> without write, adds to the map count for the page, but does not have a
> writable mapping.

Hi Robin,

dup_mmap holds mmap_sem for write. get_user_pages caller must hold it
for read.

So it think it is OK? But if not, then you can't just get rid of this
FOLL_WRITE bit, because then we get infinite loops when a 'force'
write access (eg. ptrace setting a breakpoint in text).

Re: [PATCH] get_user_pages(..., write==1, ...) may return with readable pte.

[Nick Piggin]

On Sat, Oct 14, 2006 at 06:53:06AM +0200, Nick Piggin wrote:
> On Fri, Oct 13, 2006 at 03:33:42PM -0500, Robin Holt wrote:
> > Handle the case in get_user_pages() when a call to __handle_mm_fault()
> > inserts a writable pte, and a process doing dup_mmap converts it
> > to readable before get_user_pages() does the subsequent request to
> > follow_page().
> > 
> > 
> > Signed-off-by: Robin Holt <holt@sgi.com>
> > 
> > ---
> > 
> > Hugh, Nick, and Linus,
> > 
> > I think I have tripped over another flavor of a get_user_pages bug
> > we addressed back in 2005.  I do not have a test case to prove it is
> > the issue I am trying to address, but I have done as thorough a code
> > walk-through as I can.
> > 
> > Assume a pte is currently empty.  A first pthread is in the kernel on
> > a call path which is leading to get_user_pages.  A second pthread is
> > in the process of doing a fork.  The process doing get_user_pages()
> > gets into __handle_mm_fault() and grabs ptl just before the process
> > doing a fork attempts to grab the ptl to convert the pages to COW.
> > __handle_mm_fault() will insert the writable pte and unlock ptl then
> > return with VM_FAULT_WRITE set.  The process doing a fork then gets
> > the lock and starts converting the pte to RO/COW.  The get_user_pages()
> > process then clears FOLL_WRITE from foll_flags and calls follow_page()
> > without write, adds to the map count for the page, but does not have a
> > writable mapping.
> 
> Hi Robin,
> 
> dup_mmap holds mmap_sem for write. get_user_pages caller must hold it
> for read.
> 
> So it think it is OK? But if not, then you can't just get rid of this
> FOLL_WRITE bit, because then we get infinite loops when a 'force'
> write access (eg. ptrace setting a breakpoint in text).

What problem are you seeing, BTW?

Re: [PATCH] get_user_pages(..., write==1, ...) may return with readable pte.

[Robin Holt]

> dup_mmap holds mmap_sem for write. get_user_pages caller must hold it
> for read.

I could have sworn I checked for that and found a down_read(), but
now that I look when I have some time, it is clearly a down_write().
Sorry for the distraction.

It is a user job that is passing data between hosts.  The host is
under heavy memory pressure and one rank of the MPI job gets silent
data corruption.

Thanks and sorry for wasting your time,
Robin

Re: [PATCH] get_user_pages(..., write==1, ...) may return with readable pte.

[Nick Piggin]

On Sat, Oct 14, 2006 at 05:15:07AM -0500, Robin Holt wrote:
> > dup_mmap holds mmap_sem for write. get_user_pages caller must hold it
> > for read.
> 
> I could have sworn I checked for that and found a down_read(), but
> now that I look when I have some time, it is clearly a down_write().
> Sorry for the distraction.

I'm one to do the same thing, don't worry ;)

> It is a user job that is passing data between hosts.  The host is
> under heavy memory pressure and one rank of the MPI job gets silent
> data corruption.

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=016eb4a0ed06a3677d67a584da901f0e9a63c666

It could be this? inode reclaim calls invalidate_inode_pages, so this
one is a memory corrupter under heavy reclaim pressure.


> 
> Thanks and sorry for wasting your time,

Not at all, my pleasure.