Re: [PATCHv4 13/14] xen/gntdev: mark userspace PTEs as special on x86 PV guests

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Mon, 26 Jan 2015, David Vrabel wrote:
> In an x86 PV guest, get_user_pages_fast() on a userspace address range
> containing foreign mappings does not work correctly because the M2P
> lookup of the MFN from a userspace PTE may return the wrong page.
> 
> Force get_user_pages_fast() to fail on such addresses by marking the PTEs
> as special.
> 
> If Xen has XENFEAT_gnttab_map_avail_bits (available since at least
> 4.0), we can do so efficiently in the grant map hypercall.  Otherwise,
> it needs to be done afterwards.  This is both inefficient and racy
> (the mapping is visible to the task before we fixup the PTEs), but
> will be fine for well-behaved applications that do not use the mapping
> until after the mmap() system call returns.
> 
> Guests with XENFEAT_auto_translated_physmap (ARM and x86 HVM or PVH)
> do not need this since get_user_pages() has always worked correctly
> for them.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/xen/gntdev.c                |   32 ++++++++++++++++++++++++++++++--
>  include/xen/interface/features.h    |    6 ++++++
>  include/xen/interface/grant_table.h |    7 +++++++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 494bd06..8ea10eb 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -244,11 +244,24 @@ static int find_grant_ptes(pte_t *pte, pgtable_t token,
>  	BUG_ON(pgnr >= map->count);
>  	pte_maddr = arbitrary_virt_to_machine(pte).maddr;
>  
> +	/*
> +	 * Set the PTE as special to force get_user_pages_fast() fall
> +	 * back to the slow path.  If this is not supported as part of
> +	 * the grant map, it will be done afterwards.
> +	 */
> +	if (xen_feature(XENFEAT_gnttab_map_avail_bits))
> +		flags |= (1 << _GNTMAP_guest_avail0);
> +
>  	gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr, flags,
>  			  map->grants[pgnr].ref,
>  			  map->grants[pgnr].domid);
> -	gnttab_set_unmap_op(&map->unmap_ops[pgnr], pte_maddr, flags,
> -			    -1 /* handle */);

Why are you removing the call to gnttab_set_unmap_op?


> +	return 0;
> +}
> +
> +static int set_grant_ptes_as_special(pte_t *pte, pgtable_t token,
> +				     unsigned long addr, void *data)
> +{
> +	set_pte_at(current->mm, addr, pte, pte_mkspecial(*pte));
>  	return 0;
>  }
>  
> @@ -842,6 +855,21 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma)
>  			if (err)
>  				goto out_put_map;
>  		}
> +	} else {
> +		/*
> +		 * If the PTEs were not made special by the grant map
> +		 * hypercall, do so here.
> +		 *
> +		 * This is racy since the mapping is already visible
> +		 * to userspace but userspace should be well-behaved
> +		 * enough to not touch it until the mmap() call
> +		 * returns.
> +		 */
> +		if (!xen_feature(XENFEAT_gnttab_map_avail_bits)) {
> +			apply_to_page_range(vma->vm_mm, vma->vm_start,
> +					    vma->vm_end - vma->vm_start,
> +					    set_grant_ptes_as_special, NULL);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/include/xen/interface/features.h b/include/xen/interface/features.h
> index 131a6cc..6ad3d11 100644
> --- a/include/xen/interface/features.h
> +++ b/include/xen/interface/features.h
> @@ -41,6 +41,12 @@
>  /* x86: Does this Xen host support the MMU_PT_UPDATE_PRESERVE_AD hypercall? */
>  #define XENFEAT_mmu_pt_update_preserve_ad  5
>  
> +/*
> + * If set, GNTTABOP_map_grant_ref honors flags to be placed into guest kernel
> + * available pte bits.
> + */
> +#define XENFEAT_gnttab_map_avail_bits      7
> +
>  /* x86: Does this Xen host support the HVM callback vector type? */
>  #define XENFEAT_hvm_callback_vector        8
>  
> diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
> index bcce564..56806bc 100644
> --- a/include/xen/interface/grant_table.h
> +++ b/include/xen/interface/grant_table.h
> @@ -526,6 +526,13 @@ DEFINE_GUEST_HANDLE_STRUCT(gnttab_cache_flush);
>  #define GNTMAP_contains_pte     (1<<_GNTMAP_contains_pte)
>  
>  /*
> + * Bits to be placed in guest kernel available PTE bits (architecture
> + * dependent; only supported when XENFEAT_gnttab_map_avail_bits is set).
> + */
> +#define _GNTMAP_guest_avail0    (16)
> +#define GNTMAP_guest_avail_mask ((uint32_t)~0 << _GNTMAP_guest_avail0)
> +
> +/*
>   * Values for error status returns. All errors are -ve.
>   */
>  #define GNTST_okay             (0)  /* Normal return.                        */
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>