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>]

Adding Ian J for his opinion.

I think this is a candidate for 4.2.0, although I would like to get rc2
under our belts first.

On Wed, 2012-08-08 at 04:44 +0100, Wangzhenguo wrote:
> > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> > 
> > I don't think we should expect it to be valid to keep an xc interface
> > handle open after a fork. The child should open a fresh handle if it
> > wants to keep interacting with xc.
> OK, I see xs, libxl and python open a fresh handle to interact with xc in 
> child process. 
> I modified the patch as follow:
> 
> # HG changeset patch
> # Parent a5dfd924fcdb173a154dad9f37073c1de1302065
> libxc: Add VM_DONTCOPY flag of the VMA of the hypercall buffer, to avoid the 
> hypercall buffer becoming COW on hypercall.
> 
> In multi-threads and multi-processes environment, e.g. the process has two 
> threads, thread A 
> may call hypercall, thread B may call fork() to create child process. After 
> forking, all pages 
> of the process including hypercall buffers are cow. The hypervisor calls 
> copy_to_user in hypercall 
> in thread A context, will cause a write protection and return EFAULT error.
> 
> Fix:
> 1. Before hypercall: use MADV_DONTFORK of madvise syscall to make the 
> hypercall buffer
>    not to be copied to child process after fork. 
> 2. After hypercall: undo the effect of MADV_DONTFORK for the hypercall buffer 
> by 
>    using MADV_DOFORK of madvise syscall.
> 3. Use mmap/nunmap for memory alloc/free instead of malloc/free to bypass 
> libc.
> 
> Note: 
> Chlid process do not use xc interface handle which is opend by parent 
> process, it should open 
> a fresh handle if it wants to keep interacting with xc. Otherwise, it may 
> cause segment fault 
> to access hypercall buffer cache in the handle.

It would be good to write this in a comment next to each of the
xc_{interface,evtchn,gnttab,gntshr}_open() prototypes in the header
(assuming it applies to all of them, since they all make hypercalls I
expect it does and in any case it is easy to relax this restriction in
the future if not)

Otherwise:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> 
> Signed-off-by: Zhenguo Wang <wangzhenguo@xxxxxxxxxx>
> Signed-off-by: Xiaowei Yang <xiaowei.yang@xxxxxxxxxx>
> 
> diff -r a5dfd924fcdb tools/libxc/xc_linux_osdep.c
> --- a/tools/libxc/xc_linux_osdep.c    Tue Aug 07 13:52:10 2012 +0800
> +++ b/tools/libxc/xc_linux_osdep.c    Wed Aug 08 11:33:38 2012 +0800
> @@ -93,22 +93,20 @@ static void *linux_privcmd_alloc_hyperca
>      size_t size = npages * XC_PAGE_SIZE;
>      void *p;
>  
> -    p = xc_memalign(xch, XC_PAGE_SIZE, size);
> -    if (!p)
> -        return NULL;
> +    /* Address returned by mmap is page aligned. */
> +    p = mmap(NULL, size, PROT_READ|PROT_WRITE, 
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_LOCKED, -1, 0);
>  
> -    if ( mlock(p, size) < 0 )
> -    {
> -        free(p);
> -        return NULL;
> -    }
> +    /* Do not copy the VMA to child process on fork. Avoid the page being 
> COW on hypercall. */
> +    madvise(ptr, npages * XC_PAGE_SIZE, MADV_DONTFORK);
>      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);
> +    /* Recover the VMA flags. Maybe it's not necessary */
> +    madvise(ptr, npages * XC_PAGE_SIZE, MADV_DOFORK);
> +    
> +    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