Re: [Xen-devel] The hypercall will fail and return EFAULT when the page becomes COW by forking process in linux

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-08-06 at 14:01 +0100, Wangzhenguo wrote:
> > -----Original Message-----
> > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> > Sent: Friday, August 03, 2012 6:10 PM
> > 
> > BTW, I think this would be a good fix to have for 4.2.0 if you are able
> > to produce a patch.
> > 
> Hi, Ian
> There is a patch that changes:
> 1. use madvise(MADV_DONTFORK) decleare that don't copy the vma when fork 
>    after allocating pages, and usr madvise(MADV_DOFORK) restore the flags 
>    of vma before freeing the pages.
> 2. use mmap/nunmap to alloc/free memory instead of malloc/free for passing 
>    through libc.
>    The free interface in libc may not really free memory, just returns the 
>    control to libc. If the memeory set not copy when call fork(), after 
> forking:
>    a, In child process, you call free() to the memory, then malloc(), 
>       the libc maybe return the same memory, if you access the memeory, 
>       and it causes segment fault.
>    b, If you not call the free() in child process, it maybe leak the memory 
>       which manages the malloc's memory in libc.
>    mmap/munmap don't those problems.
> 3. In the same thread, do not call fork() syscall between 
> xc__hypercall_buffer_alloc_pages 
>    and xc__hypercall_buffer_free_pages,otherwise it will cause segment fault 
>    when access the hypercall buffer in child process.
>   In normal context, we call alloc hypercall buffer, then call hypercall, 
> and free the hypercall buffer (or free to the cache). No one call fork() 
> between alloc and free hypercall buffers, so, I don't think it's a problem

Another thread in the process might fork though, wasn't that the main
observation you made when you first posted?

I think perhaps you mean it is forbidden to fork and then access a
hypercall buffer allocated before the fork, which sounds ok, since no
thread which allocates a hypercall buffer should fork with it still
allocated.

> .
> 
> We test the patch and it's OK on multi-threads and multi-processes context.
> 
> Thanks Ian and xiaowei for giving good ideas.

Thanks, this will need a Signed-off-by and a commit message as described
in: http://wiki.xen.org/wiki/Submitting_Xen_Patches

> diff -r 3d17148e465c tools/libxc/xc_hcall_buf.c
> --- a/tools/libxc/xc_hcall_buf.c      Thu Aug 02 11:49:37 2012 +0200
> +++ b/tools/libxc/xc_hcall_buf.c      Mon Aug 06 19:45:00 2012 +0800
> @@ -19,6 +19,7 @@

Please can you add this to your ~/.hgrc:
        [diff]
        showfunc = True

That will make "hg diff" and similar functions show the name of the
changed function here which is very useful for reviewers.

> @@ -135,6 +136,9 @@
>  
>      b->hbuf = p;
>  
> +    /*Do not copy the VMA to child process when call fork(), avoid the page 
> being COW when hyper calling*/
> +    madvise(p, nr_pages * PAGE_SIZE, MADV_DONTFORK);

madvise(2) tells me that MADV_{DO,DONT}FORK are Linux specific, so I
think this belongs in the Linux specific alloc_hypercall_buffer hook.

>      memset(p, 0, nr_pages * PAGE_SIZE);
>  
>      return b->hbuf;
> @@ -145,6 +149,8 @@
>      if ( b->hbuf == NULL )
>          return;
>  
> +    /*Recover the VMA flags, allow copy the VMA when call fork()*/
> +    madvise(b->hbuf, nr_pages * PAGE_SIZE, MADV_DOFORK) ;

Likewise I think this belongs in the free_hypercall_buffer hook.

>      if ( !hypercall_buffer_cache_free(xch, b->hbuf, nr_pages) )
>          xch->ops->u.privcmd.free_hypercall_buffer(xch, xch->ops_handle, 
> b->hbuf, nr_pages);
>  }
> diff -r 3d17148e465c tools/libxc/xc_linux_osdep.c
> --- a/tools/libxc/xc_linux_osdep.c    Thu Aug 02 11:49:37 2012 +0200
> +++ b/tools/libxc/xc_linux_osdep.c    Mon Aug 06 19:45:00 2012 +0800
> @@ -93,22 +93,14 @@
>      size_t size = npages * XC_PAGE_SIZE;
>      void *p;
>  
> -    p = xc_memalign(xch, XC_PAGE_SIZE, size);
> -    if (!p)
> -        return NULL;
> +    p = mmap(NULL, size, PROT_READ|PROT_WRITE, 
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_LOCKED, -1, 0);

I suppose this must necessarily return a page aligned result?

>  
> -    if ( mlock(p, size) < 0 )
> -    {
> -        free(p);
> -        return NULL;
> -    }
>      return p;
>  }
>  
>  static void linux_privcmd_free_hypercall_buffer(xc_interface *xch, 
> xc_osdep_handle h, void *ptr, int npages)
>  {
> -    munlock(ptr, npages * XC_PAGE_SIZE);
> -    free(ptr);
> +    munmap(ptr, npages * XC_PAGE_SIZE);
>  }
>  
>  static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, 
> privcmd_hypercall_t *hypercall)



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel