Re: [PATCH] mm: fix possible cause of a page_mapped BUG

[Hugh Dickins]

On Wed, 6 Apr 2011, Linus Torvalds wrote:
> On Wed, Apr 6, 2011 at 7:47 AM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> >>
> >> I dunno. But that odd negative pg_off thing makes me think there is
> >> some overflow issue (ie HEAP_INDEX being pg_off + size ends up
> >> fluctuating between really big and really small). So I'd suspect THAT
> >> as the main reason.
> >
> > Yes, one of the vmas is such that the end offset (pgoff of next page
> > after) would be 0, and for the other it would be 16.  There's sure to
> > be places, inside the prio_tree code and outside it, where we rely
> > upon pgoff not wrapping around - wrap should be prevented by original
> > validation of arguments.
> 
> Well, we _do_ validate them in do_mmap_pgoff(), which is the main
> routine for all the mmap() system calls, and the main way to get a new
> mapping.
> 
> There are other ways, like do_brk(), but afaik that always sets
> vm_pgoff to the virtual address (shifted), so again the new mapping
> should be fine.
> 
> So when a new mapping is created, it should all be ok.
> 
> But I think mremap() may end up expanding it without doing the same
> overflow check.
> 
> Do you see any other way to get this situation? Does the vma dump give
> you any hint about where it came from?
> 
> Robert - here's a (UNTESTED!) patch to make mremap() be a bit more
> careful about vm_pgoff when growing a mapping. Does it make any
> difference?

I'd come to the same conclusion: the original page_mapped BUG has itself
suggested that mremap() is getting used.

I was about to send you my own UNTESTED patch: let me append it anyway,
I think it is more correct than yours (it's the offset of vm_end we need
to worry about, and there's the funny old_len,new_len stuff).  See what
you think - sorry, I'm going out now.

Hugh

--- 2.6.38/mm/mremap.c	2011-03-14 18:20:32.000000000 -0700
+++ linux/mm/mremap.c	2011-04-06 08:31:46.000000000 -0700
@@ -282,6 +282,12 @@ static struct vm_area_struct *vma_to_res
 			goto Efault;
 	}
 
+	if (vma->vm_file && new_len > old_len) {
+		pgoff_t endoff = linear_page_index(vma, vma->vm_end);
+		if (endoff + ((new_len - old_len) >> PAGE_SHIFT) < endoff)
+			goto Eoverflow;
+	}
+
 	if (vma->vm_flags & VM_LOCKED) {
 		unsigned long locked, lock_limit;
 		locked = mm->locked_vm << PAGE_SHIFT;
@@ -311,6 +317,8 @@ Enomem:
 	return ERR_PTR(-ENOMEM);
 Eagain:
 	return ERR_PTR(-EAGAIN);
+Eoverflow:
+	return ERR_PTR(-EOVERFLOW);
 }
 
 static unsigned long mremap_to(unsigned long addr,