Re: [PATCH 3/3] xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Aug 23, 2012 at 06:13:46PM +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> PRIVCMD_MMAPBATCH_V2 extends PRIVCMD_MMAPBATCH with an additional
> field for reporting the error code for every frame that could not be
> mapped.  libxc prefers PRIVCMD_MMAPBATCH_V2 over PRIVCMD_MMAPBATCH.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/xen/privcmd.c |   54 +++++++++++++++++++++++++++++++++++++++----------
>  include/xen/privcmd.h |   10 +++++++++
>  2 files changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index f8c1b6d..4f97160 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -248,7 +248,8 @@ struct mmap_batch_state {
>  	struct vm_area_struct *vma;
>  	int err;
>  
> -	xen_pfn_t __user *user;
> +	xen_pfn_t __user *user_mfn;
> +	int __user *user_err;
>  };
>  
>  static int mmap_batch_fn(void *data, void *state)
> @@ -275,34 +276,58 @@ static int mmap_return_errors(void *data, void *state)
>  {
>  	xen_pfn_t *mfnp = data;
>  	struct mmap_batch_state *st = state;
> +	int ret = 0;
>  
> -	return put_user(*mfnp, st->user++);
> +	if (st->user_err) {
> +		if ((*mfnp & 0xf0000000U) == 0xf0000000U)
> +			ret = -ENOENT;
> +		else if ((*mfnp & 0xf0000000U) == 0x80000000U)
> +			ret = -EINVAL;

Yikes. Any way those 0xf000.. and 0x8000 can at least be #defined

> +		else
> +			ret = 0;
> +		return __put_user(ret, st->user_err);
> +	} else
> +		return __put_user(*mfnp, st->user_mfn++);
>  }
>  
>  static struct vm_operations_struct privcmd_vm_ops;
>  
> -static long privcmd_ioctl_mmap_batch(void __user *udata)
> +static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  {
>  	int ret;
> -	struct privcmd_mmapbatch m;
> +	struct privcmd_mmapbatch_v2 m;
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	unsigned long nr_pages;
>  	LIST_HEAD(pagelist);
>  	struct mmap_batch_state state;
>  
> +	printk("%s(%d)\n", __func__, version);
> +

Hehe.
>  	if (!xen_initial_domain())
>  		return -EPERM;
>  
> -	if (copy_from_user(&m, udata, sizeof(m)))
> -		return -EFAULT;
> +	if (version == 1) {
> +		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
> +			return -EFAULT;
> +		if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> +			return -EFAULT;

That is new..
> +		m.err = NULL;
> +	} else {

Not elseif (version == 2) ?
.. what if version 3 comes around?

> +		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> +			return -EFAULT;
> +		if (!access_ok(VERIFY_READ, m.arr, m.num * sizeof(*m.arr)))
> +			return -EFAULT;
> +		if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> +			return -EFAULT;

I think the VERIFY_WRITE can cover both versions?
> +	}
>  
>  	nr_pages = m.num;
>  	if ((m.num <= 0) || (nr_pages > (LONG_MAX >> PAGE_SHIFT)))
>  		return -EINVAL;
>  
>  	ret = gather_array(&pagelist, m.num, sizeof(xen_pfn_t),
> -			   m.arr);
> +			   (xen_pfn_t *)m.arr);
>  
>  	if (ret || list_empty(&pagelist))
>  		goto out;
> @@ -331,10 +356,11 @@ static long privcmd_ioctl_mmap_batch(void __user *udata)
>  	up_write(&mm->mmap_sem);
>  
>  	if (state.err > 0) {
> -		state.user = m.arr;
> +		state.user_mfn = (xen_pfn_t *)m.arr;
> +		state.user_err = m.err;
>  		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> -			       &pagelist,
> -			       mmap_return_errors, &state);
> +				     &pagelist,
> +				     mmap_return_errors, &state);
>  	}
>  
>  out:
> @@ -359,7 +385,13 @@ static long privcmd_ioctl(struct file *file,
>  		break;
>  
>  	case IOCTL_PRIVCMD_MMAPBATCH:
> -		ret = privcmd_ioctl_mmap_batch(udata);
> +		ret = privcmd_ioctl_mmap_batch(udata, 1);
> +		printk("%s() batch ret = %d\n", __func__, ret);

Pfff...
> +		break;
> +
> +	case IOCTL_PRIVCMD_MMAPBATCH_V2:
> +		ret = privcmd_ioctl_mmap_batch(udata, 2);
> +		printk("%s() batch ret = %d\n", __func__, ret);
>  		break;
>  
>  	default:
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index 17857fb..9fa27c4 100644
> --- a/include/xen/privcmd.h
> +++ b/include/xen/privcmd.h
> @@ -62,6 +62,14 @@ struct privcmd_mmapbatch {
>  	xen_pfn_t __user *arr; /* array of mfns - top nibble set on err */
>  };
>  
> +struct privcmd_mmapbatch_v2 {
> +	unsigned int num; /* number of pages to populate */

unsigend int? Not 'u32'?
> +	domid_t dom;      /* target domain */
> +	__u64 addr;       /* virtual address */
> +	const xen_pfn_t __user *arr; /* array of mfns */
> +	int __user *err;  /* array of error codes */

int? Not a specific type?
> +};
> +
>  /*
>   * @cmd: IOCTL_PRIVCMD_HYPERCALL
>   * @arg: &privcmd_hypercall_t
> @@ -73,5 +81,7 @@ struct privcmd_mmapbatch {
>  	_IOC(_IOC_NONE, 'P', 2, sizeof(struct privcmd_mmap))
>  #define IOCTL_PRIVCMD_MMAPBATCH					\
>  	_IOC(_IOC_NONE, 'P', 3, sizeof(struct privcmd_mmapbatch))
> +#define IOCTL_PRIVCMD_MMAPBATCH_V2				\
> +	_IOC(_IOC_NONE, 'P', 4, sizeof(struct privcmd_mmapbatch_v2))
>  
>  #endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
> -- 
> 1.7.2.5