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

[Nick Piggin <npiggin@suse.de>]

On Wed, Oct 31, 2007 at 04:08:21PM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 31 Oct 2007, Nick Piggin wrote:
> > 
> > No that would be great. Fingers crossed it won't cause any problems.
> 
> I actually doubt it will cause problems.
> 
> 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?

But I think that is pretty arbitrary as well. What the "force" argument to
get_user_pages on a MAP_SHARED vma effectively does is punt the permission
check from the vma permissions to the file permissions. If you've opened the
file WR but mmap()ed PROT_READ, it goes ahead and COWs, wheras if you've
opened the file RDONLY it -EFAULTs.

This seems strange wrong, because the act of COWing seems to be exactly
done in order to *absolve* us of file permissions (if we're COWing, why
should it matter if the file happened to be readonly or not).

Could we just try disallow COW for MAP_SHARED mappings?
Seeing as we're doing the break-ptrace thing this release already ;)
All the stuff I know of that requires forced COW (ie. gdb for breakpoints),
does so on MAP_PRIVATE mappings.

I know this is one of Hugh's bugbears. I don't know whether he's thought
out how to remove it, but I wonder if the appended patch is roughly in
the right direction?

> But if people have test-cases for ptrace and/or other magical users of 
> 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.

---
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c
+++ linux-2.6/mm/memory.c
@@ -1563,13 +1563,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
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c
+++ linux-2.6/mm/mmap.c
@@ -980,9 +980,11 @@ unsigned long do_mmap_pgoff(struct file 
 			if (locks_verify_locked(inode))
 				return -EAGAIN;
 
-			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;
 
 			/* fall through */
 		case MAP_PRIVATE:
@@ -998,6 +1000,7 @@ unsigned long do_mmap_pgoff(struct file 
 
 			if (!file->f_op || !file->f_op->mmap)
 				return -ENODEV;
+
 			break;
 
 		default:
@@ -1007,6 +1010,8 @@ unsigned long do_mmap_pgoff(struct file 
 		switch (flags & MAP_TYPE) {
 		case MAP_SHARED:
 			vm_flags |= VM_SHARED | VM_MAYSHARE;
+			if (!(vm_flags & VM_WRITE))
+				vm_flags &= ~VM_MAYWRITE;
 			break;
 		case MAP_PRIVATE:
 			/*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/