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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Aug 23, 2012 at 06:13:46PM +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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

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