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

[David Vrabel <david.vrabel@citrix.com>]

On 30/08/12 19:32, Andres Lagar-Cavilla wrote:
> Second repost of my version, heavily based on David's. 

Doing another allocation that may be larger than a page (and its
associated additional error paths) seems to me to be a hammer to crack
the (admittedly bit wonky) casting nut.

That said, it's up to Konrad which version he prefers.

There are also some minor improvements you could make if you respin this
patch.

> Complementary to this patch, on the xen tree I intend to add
> PRIVCMD_MMAPBATCH_*_ERROR into the libxc header files, and remove
> XEN_DOMCTL_PFINFO_PAGEDTAB from domctl.h

Yes, a good idea.  There's no correspondence between the ioctl's error
reporting values and the DOMCTL_PFINFO flags.

> commit 3f40e8d79b7e032527ee207a97499ddbc81ca12b
> Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Date:   Thu Aug 30 12:23:33 2012 -0400
> 
>     xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl
>     
>     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.
>     
>     Also expand PRIVCMD_MMAPBATCH to return appropriate error-encoding top nibble
>     in the mfn array.
>     
>     Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
[...]
> -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);
> +	int *err_array;

int *err_array = NULL;

and you could avoid the additional jump label as kfree(NULL) is safe.

>  	struct mmap_batch_state state;
>  
>  	if (!xen_initial_domain())
>  		return -EPERM;
>  
> -	if (copy_from_user(&m, udata, sizeof(m)))
> -		return -EFAULT;
> +	switch (version) {
> +	case 1:
> +		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
> +			return -EFAULT;
> +		/* Returns per-frame error in m.arr. */
> +		m.err = NULL;
> +		if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> +			return -EFAULT;
> +		break;
> +	case 2:
> +		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> +			return -EFAULT;
> +		/* Returns per-frame error code in m.err. */
> +		if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> +			return -EFAULT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	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);
> +	ret = gather_array(&pagelist, m.num, sizeof(xen_pfn_t), m.arr);
>  
>  	if (ret || list_empty(&pagelist))
> -		goto out;
> +		goto out_no_err_array;
> +
> +	err_array = kmalloc_array(m.num, sizeof(int), GFP_KERNEL);

kcalloc() (see below).

> +	if (err_array == NULL)
> +	{

Style: if (err_array == NULL) {

> +	if (state.global_error) {
> +		int efault;
> +
> +		if (state.global_error == -ENOENT)
> +			ret = -ENOENT;
> +
> +		/* Write back errors in second pass. */
> +		state.user_mfn = (xen_pfn_t *)m.arr;
> +		state.user_err = m.err;
> +		state.err      = err_array;
> +		efault = traverse_pages(m.num, sizeof(xen_pfn_t),
> +					 &pagelist, mmap_return_errors, &state);
> +		if (efault)
> +			ret = efault;
> +	} else if (m.err)
> +		ret = __clear_user(m.err, m.num * sizeof(*m.err));

Since you have an array of errors already there's no need to iterate
through all the MFNs again for V2.  A simple copy_to_user() is
sufficient provided err_array was zeroed with kcalloc().

if (m.err)
    ret = __copy_to_user(m.err, err_array, m.num * sizeof(*m.err));
else {
    /* Write back errors in second pass. */
    state.user_mfn = (xen_pfn_t *)m.arr;
    state.user_err = m.err;
    state.err      = err_array;
    ret = traverse_pages(m.num, sizeof(xen_pfn_t),
            &pagelist, mmap_return_errors, &state);
}

if (ret == 0 && state.global_error == -ENOENT)
    ret = -ENOENT;

David