[PATCH] vm_area_struct slab corruption

[PATCH] vm_area_struct slab corruption

[Hugh Dickins]

Fix vm_area_struct slab corruption due to mremap's move_vma mistaking
(okay, okay, _my_ mistaking) how do_munmap splits vmas in one case.

This patch fits do_munmap to move_vma's expectation: you may well feel
that's the wrong way round to fix it (and not the way I promised a few
days ago), but at present I'm more comfortable with this simpler fix;
and it does seem preferable for do_munmap to reuse the existing vma.

Hugh

--- 2.5.64/mm/mmap.c	Wed Mar  5 07:26:34 2003
+++ linux/mm/mmap.c	Thu Mar  6 11:47:44 2003
@@ -1258,20 +1258,24 @@
  
 	/*
 	 * If we need to split any vma, do it now to save pain later.
+	 *
+	 * Note: mremap's move_vma VM_ACCOUNT handling assumes a partially
+	 * unmapped vm_area_struct will remain in use: so lower split_vma
+	 * places tmp vma above, and higher split_vma places tmp vma below.
 	 */
 	if (start > mpnt->vm_start) {
 		if (split_vma(mm, mpnt, start, 0))
 			return -ENOMEM;
 		prev = mpnt;
-		mpnt = mpnt->vm_next;
 	}
 
 	/* Does it split the last one? */
 	last = find_vma(mm, end);
 	if (last && end > last->vm_start) {
-		if (split_vma(mm, last, end, 0))
+		if (split_vma(mm, last, end, 1))
 			return -ENOMEM;
 	}
+	mpnt = prev? prev->vm_next: mm->mmap;
 
 	/*
 	 * Remove the vma's, and unmap the actual pages

Re: [PATCH] vm_area_struct slab corruption

[Andrew Morton]

Hugh Dickins <hugh@veritas.com> wrote:
>
> +	 * Note: mremap's move_vma VM_ACCOUNT handling assumes a partially
> +	 * unmapped vm_area_struct will remain in use: so lower split_vma
> +	 * places tmp vma above, and higher split_vma places tmp vma below.

Cough.  Would it be clearer to just return the address of the surviving vma
from do_munmap()?  Via an extra arg, or a PTR_ERR thingy?

Re: [PATCH] vm_area_struct slab corruption

[Hugh Dickins]

On Thu, 6 Mar 2003, Andrew Morton wrote:
> Hugh Dickins <hugh@veritas.com> wrote:
> >
> > +	 * Note: mremap's move_vma VM_ACCOUNT handling assumes a partially
> > +	 * unmapped vm_area_struct will remain in use: so lower split_vma
> > +	 * places tmp vma above, and higher split_vma places tmp vma below.
> 
> Cough.  Would it be clearer to just return the address of the surviving vma
> from do_munmap()?  Via an extra arg, or a PTR_ERR thingy?

Sneeze.  Address of the surviving?  I think you must mean address of
the vma now previous to what's been unmapped, NULL if none previous.

Hmm, could do that: it would make the interface between move_vma and
do_munmap less fragile.  But it wouldn't make move_vma any clearer or
easier - can't make use of that previous vma without the same analysis
of cases as before.

If adding an extra arg, then the extra arg to add would be what Alan
did in 2.4-ac, int acct to control whether it does the VM_ACCOUNTing.
I resisted adding that (changing odd distant drivers), and I may have
been wrong to do so.

I'm just reluctant to make more change here at this moment, my focus
is elsewhere.  I cannot approach move_vma without wanting to change
more than is safe to do in a rush: it should be using your newer
can_vma_merge_ stuff; does its merging have to look so complex?
needs to check if do_munmap failed and behave sanely if so.
But I can't let slab corruption and backtraces await my leisure.

Hugh

Re: [PATCH] vm_area_struct slab corruption

[Andrew Morton]

Hugh Dickins <hugh@veritas.com> wrote:
>
> If adding an extra arg, then the extra arg to add would be what Alan
> did in 2.4-ac, int acct to control whether it does the VM_ACCOUNTing.
> I resisted adding that (changing odd distant drivers), and I may have
> been wrong to do so.

This looks pretty simple?  Is it not just a matter of propagating that flag
down to unmap_region()?  I don't see where drivers and such are involved?

Re: [PATCH] vm_area_struct slab corruption

[Alan Cox]

On Fri, 2003-03-07 at 06:00, Andrew Morton wrote:
> This looks pretty simple?  Is it not just a matter of propagating that flag
> down to unmap_region()?  I don't see where drivers and such are involved?

Only two or three. i8xx DRM uses do_munmap as does ipc/shm.c. I think the 
shmem fs may also do so but I'd have to check.