anon-vma mprotect merging-extend fix

anon-vma mprotect merging-extend fix

[Andrea Arcangeli]

yesterday I got a crash bugreport where this BUG_ON bombed with wine.

static inline void anon_vma_page_link(struct page * page, struct
vm_area_struct
* vma,
                                      unsigned long address)
{
        unsigned long index = ((address - vma->vm_start) >> PAGE_SHIFT)
+
vma->vm_pgoff;

        BUG_ON(!vma->anon_vma);
        if (page->mapcount == 1) {
                page->index = index;
                BUG_ON(page->mapping);
                page->mapping = (struct address_space *) vma->anon_vma;
        } else {
                BUG_ON(vma->anon_vma != (anon_vma_t *) page->mapping ||
                 index != page->index);
                ^^^^^^^^^^^^^^^^^^^^^^^
        }
}

The problem is that I was not propagating the anon_vma correctly in the
extend case of mprotect-merging.

This fixes the problem:

--- sles/include/linux/objrmap.h.~1~	2004-06-17 19:25:24.000000000 +0200
+++ sles/include/linux/objrmap.h	2004-06-18 03:16:29.000000000 +0200
@@ -48,6 +48,8 @@ static inline void anon_vma_unlock(struc
 extern int FASTCALL(anon_vma_prepare(struct vm_area_struct * vma));
 extern void FASTCALL(anon_vma_merge(struct vm_area_struct * vma,
 				    struct vm_area_struct * vma_dying));
+extern void FASTCALL(anon_vma_merge_extend(struct vm_area_struct * prev,
+					   struct vm_area_struct * next));
 extern void FASTCALL(anon_vma_unlink(struct vm_area_struct * vma));
 extern void FASTCALL(anon_vma_link(struct vm_area_struct * vma));
 extern void FASTCALL(__anon_vma_link(struct vm_area_struct * vma));
--- sles/mm/objrmap.c.~1~	2004-06-17 19:25:24.000000000 +0200
+++ sles/mm/objrmap.c	2004-06-18 03:32:43.000000000 +0200
@@ -302,8 +316,10 @@ void fastcall page_add_rmap(struct page 
 	 * allowed to read PG_anon outside the page_map_lock.
 	 */
 	last_anon = PageAnon(page);
-	if (anon && !last_anon)
+	if (anon && !last_anon) {
+		BUG_ON(page->mapping);
 		SetPageAnon(page);
+	}
 	BUG_ON(!anon && last_anon);
 
 	if (!page->mapcount++)
@@ -798,6 +814,51 @@ void fastcall anon_vma_merge(struct vm_a
 	}
 }
 
+/*
+ * This is called when _prev_ is being extended. This is needed
+ * by mprotect for example. This means prev->vm_end == next->vm_start
+ * and they both get moved to an address > next->vm_start and
+ * < next->vm_end. NOTE: it's critical that "prev" is extended.
+ * This will kernel-crash if it's "next" being extended down, this
+ * only works for prev->vm_end and next->vm_start going _up_,
+ * down not.
+ */
+void fastcall anon_vma_merge_extend(struct vm_area_struct * prev,
+				    struct vm_area_struct * next)
+{
+	anon_vma_t * anon_vma = next->anon_vma;
+
+	/*
+	 * If next->anon_vma is null we've nothing to do since
+	 * next is being shrunk.
+	 */
+	if (!anon_vma)
+		return;
+
+	/*
+	 * If both prev->anon_vma != 0 and next->anon_vma != 0
+	 * it means they're equal and we've nothing to do again.
+	 */
+	if (prev->anon_vma) {
+		BUG_ON(prev->anon_vma != anon_vma);
+		return;
+	}
+
+	/*
+	 * If we get here it means we've to propagate the anon_vma
+	 * from "next" to "prev" because "prev" will now have to
+	 * include pages in the previous "next" range that can
+	 * point to "anon_vma". The setting of prev->anon_vma
+	 * is serialized by the mmap_sem rwsem taken in write mode.
+	 */
+	prev->anon_vma = anon_vma;
+
+	/* Now link the "prev" vma into the "anon_vma_head" list. */
+	spin_lock(&anon_vma->anon_vma_lock);
+	list_add(&prev->anon_vma_node, &next->anon_vma_node);
+	spin_unlock(&anon_vma->anon_vma_lock);
+}
+
 void fastcall __anon_vma_link(struct vm_area_struct * vma)
 {
 	anon_vma_t * anon_vma = vma->anon_vma;
--- sles/mm/mprotect.c.~1~	2004-06-17 19:25:25.000000000 +0200
+++ sles/mm/mprotect.c	2004-06-18 03:39:38.608807352 +0200
@@ -135,7 +135,7 @@ mprotect_attempt_merge(struct vm_area_st
 
 	prev_pgoff = vma->vm_pgoff - ((prev->vm_end - prev->vm_start) >> PAGE_SHIFT);
 	file = vma->vm_file;
-	if (!is_mergeable_vma(prev, file, newflags, prev_pgoff, NULL))
+	if (!vma->vm_private_data && !is_mergeable_vma(prev, file, newflags, prev_pgoff, NULL))
 		return 0;
 	if (!is_mergeable_anon_vma(prev, vma))
 		return 0;
@@ -215,6 +215,14 @@ mprotect_attempt_merge(struct vm_area_st
 	anon_vma_unlock(vma);
 	if (file)
 		up(i_shared_sem);
+
+	/*
+	 * No need of any lock for this (except the internal anon_vma_lock),
+	 * the whole vma->anon_vma handling is serialized enterely by the
+	 * mmap_sem rwsem taken in write mode.
+	 */
+	anon_vma_merge_extend(prev, vma);
+
 	return 1;
 }
 
@@ -241,7 +249,7 @@ mprotect_attempt_merge_final(struct vm_a
 
 	next_pgoff = prev->vm_pgoff + ((prev->vm_end - prev->vm_start) >> PAGE_SHIFT);
 	file = prev->vm_file;
-	if (!is_mergeable_vma(next, file, newflags, next_pgoff, NULL))
+	if (!prev->vm_private_data && !is_mergeable_vma(next, file, newflags, next_pgoff, NULL))
 		return;
 	if (!is_mergeable_anon_vma(prev, next))
 		return;


This adds another robustness BUG_ON to be sure we switch the PG_anon
bitflag only in unmapped pages, and secondly it makes sure not to merge
anything if vm_private_data is set on the "other" vma (the one not
checked internally in is_mergeable_vma). I considered adding another
parameter to is_mergeable_vma, but I think the above is fine too, such
construct can be copied as easily without forgetting about the "other"
vma private_data.

This is against latest 2.6-aa or sles kernel (SL9.1 is unaffected
because we didn't enable the mprotect merging there, so in short nothing
is affected because this is not a released kernel yet and I fixed this
just in time ;).

I didn't have much time to look at the status of mainline in the last
week, I assume Hugh will take care of merging this fix in a way that
will apply cleanly to mainline.

I plan to finish the fix-update for 2.4 before the weekend and to
synchronize with 2.6 early next week (sorry for being a bit out of sync
and thanks for all the help).

Re: anon-vma mprotect merging-extend fix

[Hugh Dickins]

On Fri, 18 Jun 2004, Andrea Arcangeli wrote:
> yesterday I got a crash bugreport where this BUG_ON bombed with wine.
>.... 
> The problem is that I was not propagating the anon_vma correctly in the
> extend case of mprotect-merging.

Thanks a lot for this alert, Andrea, much appreciated.

Although the 2.6.7 code looks rather different here (vma_merge now does
all mprotect's merging too), and there's a couple of reasons it's harder
to see the problem with mainline (because fork's copy_page_range uses the
quick page_dup_rmap inline, skipping the BUG_ONs in page_add_anon_rmap;
and because anon_vma_prepare is likely to correct the situation by taking
anon_vma from neighbouring vma later), I missed exactly the same point
and we do indeed need a patch for it.

> This adds another robustness BUG_ON to be sure we switch the PG_anon
> bitflag only in unmapped pages, and secondly it makes sure not to merge
> anything if vm_private_data is set on the "other" vma (the one not
> checked internally in is_mergeable_vma). I considered adding another
> parameter to is_mergeable_vma, but I think the above is fine too, such
> construct can be copied as easily without forgetting about the "other"
> vma private_data.

I hope you've not found somewhere the vm_private_data change is actually
needed - because when using that field for the non-linear swapout cursor,
the test in is_mergeable_vma prevented non-linear merges, it seemed a
superfluous test to me, and so I simply removed it.  I say superfluous
because those few vmas which set vm_private_data are already prevented
from merging by VM_SPECIAL flags tests.

> I didn't have much time to look at the status of mainline in the last
> week, I assume Hugh will take care of merging this fix in a way that
> will apply cleanly to mainline.

That's fine, yes, I've now done the patch, will post it separately now.

Thanks again,
Hugh