Re: 2.6.23 regression: accessing invalid mmap'ed memory from gdb causes unkillable spinning

[Nick Piggin <npiggin@suse.de>]

On Thu, Nov 01, 2007 at 08:14:47AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 1 Nov 2007, Nick Piggin wrote:
> 
> > On Wed, Oct 31, 2007 at 04:08:21PM -0700, Linus Torvalds wrote:
> > > 
> > > We made much bigger changes to ptrace support when we disallowed writing 
> > > to read-only shared memory areas (we used to do the magic per-page COW 
> > > thing).
> > 
> > Really? No, we still do that magic COW thing which creates anonymous
> > pages in MAP_SHARED vmas, don't we?
> 
> No, we don't. I'm pretty sure. It didn't work with the VM cleanups, since 
> the MAP_SHARED vma's won't be on the anonymous list any more, and cannot 
> be swapped out.
> 
> So now, if you try to write to a read-only shared page through ptrace, 
> you'll get "Unable to access".
 
No, it COWs it (the file is RW).

I believe do_wp_page will still attach an anon_vma to the vma, which
will make the pte discoverable via rmap.


> Of course, I didn't really look closely, so maybe I just don't remember 
> things right..
> 
> > > access_vm_pages() (things like core-dumping comes to mind - although I 
> > > think we don't dump pure file mappings at all, do we?) it would certainly 
> > > be good to run any such tests on the current -git tree...
> > 
> > We do for MAP_SHARED|MAP_ANONYMOUS, by the looks.
> 
> Well, as we should. There's no way for a debugger to get those pages back. 
> So that all looks sane.
> 
> > -			vm_flags |= VM_SHARED | VM_MAYSHARE;
> > -			if (!(file->f_mode & FMODE_WRITE))
> > -				vm_flags &= ~(VM_MAYWRITE | VM_SHARED);
> > +			vm_flags |= VM_MAYSHARE;
> > +			if (file->f_mode & FMODE_WRITE)
> > +				vm_flags |= VM_SHARED;
> > +			if (!(vm_flags & VM_WRITE))
> > +				vm_flags &= ~VM_MAYWRITE;
> 
> This looks totally bogus. What was the intent of this patch?
> 
> The VM_MAYWRITE flag is *not* supposed to track the VM_WRITE flag: that 
> would defeat the whole purpose of it! The whole point of that flag is to 
> say whether mprotect() could turn it into a VM_WRITE mapping, and it 
> depends on the file mode, not VM_WRITE!

Yeah of course that won't work, stupid...

The intent is to stop get_user_pages from proceeding with a write fault (and
subsequent COW) to readonly shared mappings, when force is set. I think it
can be done simply via get_user_pages(), which is what I should have done
to begin with.

Untested patch follows
---
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c
+++ linux-2.6/mm/memory.c
@@ -1031,7 +1031,9 @@ int get_user_pages(struct task_struct *t
 		}
 
 		if (!vma || (vma->vm_flags & (VM_IO | VM_PFNMAP))
-				|| !(vm_flags & vma->vm_flags))
+				|| !(vm_flags & vma->vm_flags)
+				|| (write && ((vma->vm_flags &
+				      (VM_SHARED|VM_MAYSHARE)) == VM_MAYSHARE)))
 			return i ? : -EFAULT;
 
 		if (is_vm_hugetlb_page(vma)) {
@@ -1563,13 +1565,11 @@ static int do_wp_page(struct mm_struct *
 			reuse = can_share_swap_page(old_page);
 			unlock_page(old_page);
 		}
-	} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
-					(VM_WRITE|VM_SHARED))) {
+	} else if (unlikely((vma->vm_flags & VM_SHARED))) {
 		/*
-		 * Only catch write-faults on shared writable pages,
-		 * read-only shared pages can get COWed by
-		 * get_user_pages(.write=1, .force=1).
+		 * Only catch write-faults on shared writable pages.
 		 */
+		BUG_ON(!(vma->vm_flags & VM_WRITE));
 		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 			/*
 			 * Notify the address space that the page is about to