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 Wed, 2012-08-08 at 12:36 +0100, Wangzhenguo wrote:
> > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> > 
> > 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>
> 
> I added a comment and your acked-by in patch 
> 
> The patch is
> 
> # 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 

"Child" and "opened" (you made these typos pretty consistently
throughout ;-))

Please can you try and keep the commit message to 75-80 characters wide.

> 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.
> 
> Signed-off-by: Zhenguo Wang <wangzhenguo@xxxxxxxxxx>
> Signed-off-by: Xiaowei Yang <xiaowei.yang@xxxxxxxxxx>
> Acked-by: Ian Campbell <ian.campbell@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 19:31:53 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)
> diff -r a5dfd924fcdb tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h   Tue Aug 07 13:52:10 2012 +0800
> +++ b/tools/libxc/xenctrl.h   Wed Aug 08 19:31:53 2012 +0800
> @@ -131,8 +131,11 @@ typedef enum xc_error_code xc_error_code
>  
>  /**
>   * This function opens a handle to the hypervisor interface.  This function 
> can
> - * be called multiple times within a single process.  Multiple processes can
> - * have an open hypervisor interface at the same time.
> + * be called multiple times within a single process.  Multiple processes can 
> not
> + * have an open hypervisor interface at the same time. Chlid process do not 
> + * use xc interface handle which is opend by parent process, it should open

This would be more naturally written as "Child processes must not
use..."

> + * 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.
>   *
>   * Each call to this function should have a corresponding call to
>   * xc_interface_close().
> @@ -908,6 +911,11 @@ int xc_evtchn_status(xc_interface *xch, 
>   * Return a handle to the event channel driver, or -1 on failure, in which 
> case
>   * errno will be set appropriately.
>   *
> + * Note:
> + * Chlid process do not use xc evtchn 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.
> + *
>   * Before Xen pre-4.1 this function would sometimes report errors with 
> perror.
>   */
>  xc_evtchn *xc_evtchn_open(xentoollog_logger *logger,
> @@ -1339,9 +1347,12 @@ int xc_domain_subscribe_for_suspend(
>  
>  /*
>   * These functions sometimes log messages as above, but not always.
> - */
> -
> -/*
> + *
> + * Note:
> + * Chlid process do not use xc gnttab 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.
> + *
>   * Return an fd onto the grant table driver.  Logs errors.
>   */
>  xc_gnttab *xc_gnttab_open(xentoollog_logger *logger,
> @@ -1458,6 +1469,12 @@ grant_entry_v2_t *xc_gnttab_map_table_v2
>  
>  /*
>   * Return an fd onto the grant sharing driver.  Logs errors.
> + *
> + * Note:
> + * Chlid process do not use xc gntshr 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.
> + *
>   */
>  xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
>                         unsigned open_flags);



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