Re: [RFC][PATCH 1/2] vmalloc: reorder unmap and removal entry

[Amerigo Wang <xiyou.wangcong@gmail.com>]

On Wed, Jul 29, 2009 at 05:36:00PM +0900, KAMEZAWA Hiroyuki wrote:
>> I wonder fs/proc/kcore.c's vmalloc area access needs some fix. let me try.
>> 
>> Thanks,
>Finally, I wrote 2 patches. maybe 2/2 is a fix for the bug.
>but needs comments.
>
>Comaparing 2 calls, vfree() and vread()
>==
>vfree()
>  -> __vumap()
>     -> remove_vm_area()
>       -> free_unmap_vmap_area         #
>	 -> try_purge_vmap_area_lazy() #
>           ->__purge_vmap_area_lazy(); #
>             -> unmap_vmap_area()      # unmap memory here.
>       -> write_lock(&vmlist_lock)
>          remvoe vm_struct from list
>          write_unlock(&vmlist_lock).
>==
>vread()
> -> read_lock(&vmlist_lock);
>    get vm_struct -> do memcpy
>    read_unlock(&vmlist_lock);
>==


I think this is a very good catch!

Your patch looks reasonable for me.

>
>Hmm, maybe not related to original bug but above order should be fixed.
>
>From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
>vmap area should be purged after vm_struct is removed from the list
>because vread/vwrite etc...believes the range is valid while it's on
>vm_struct list.
>
>Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>

Aside, would you like to replace vmlist, a single list, with our
generic list API? :) Just as you did for kcore. Pls send it in
a seprate patch.

Thanks!


>---
> mm/vmalloc.c |   14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>Index: linux-2.6.31-rc4/mm/vmalloc.c
>===================================================================
>--- linux-2.6.31-rc4.orig/mm/vmalloc.c
>+++ linux-2.6.31-rc4/mm/vmalloc.c
>@@ -1256,17 +1256,21 @@ struct vm_struct *remove_vm_area(const v
> 	if (va && va->flags & VM_VM_AREA) {
> 		struct vm_struct *vm = va->private;
> 		struct vm_struct *tmp, **p;
>-
>-		vmap_debug_free_range(va->va_start, va->va_end);
>-		free_unmap_vmap_area(va);
>-		vm->size -= PAGE_SIZE;
>-
>+		/*
>+		 * remove from list and disallow access to this vm_struct
>+		 * before unmap. (address range confliction is maintained by
>+		 * vmap.)
>+		 */
> 		write_lock(&vmlist_lock);
> 		for (p = &vmlist; (tmp = *p) != vm; p = &tmp->next)
> 			;
> 		*p = tmp->next;
> 		write_unlock(&vmlist_lock);
> 
>+		vmap_debug_free_range(va->va_start, va->va_end);
>+		free_unmap_vmap_area(va);
>+		vm->size -= PAGE_SIZE;
>+
> 		return vm;
> 	}
> 	return NULL;
>