anon-vma mprotect merging-extend fix

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:

@@ -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));

@@ -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).

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.

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.

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

Thanks again,
Hugh