Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

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

On 16/11/12 15:37, Andres Lagar-Cavilla wrote:
>> This is a regression introduced by ceb90fa0 (xen/privcmd: add
>> PRIVCMD_MMAPBATCH_V2 ioctl).  It broke xentrace as it used
>> xc_map_foreign() instead of xc_map_foreign_bulk().
>>
>> Most code-paths prefer the MMAPBATCH_V2, so this wasn't very obvious
>> that it broke. The return value is set early on to -EINVAL, and if all
>> goes well, the "set top bits of the MFN's" never gets called, so the
>> return value is still EINVAL when the function gets to the end, causing
>> the caller to think it went wrong (which it didn't!)
>>
>> Signed off by: Mats Petersson <mats.petersson@citrix.com>
>> Acked-by: David Vrabel <david.vrabel@citrix.com>
> 
> Uggh. What a complicated API. Good catch, thanks.
> 
> Now, isn't this a simpler fix? (and by only changing ret to non-zero
> in error paths, less prone to allow for inadvertent errors in the future)

I had considered this, but I think Mats patch is clearer overall as it
makes the v1 and the v2 paths more similar.

David

> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index ef63895..4a6bcb2 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -361,13 +361,13 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>         down_write(&mm->mmap_sem);
>  
>         vma = find_vma(mm, m.addr);
> -       ret = -EINVAL;
>         if (!vma ||
>             vma->vm_ops != &privcmd_vm_ops ||
>             (m.addr != vma->vm_start) ||
>             ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
>             !privcmd_enforce_singleshot_mapping(vma)) {
>                 up_write(&mm->mmap_sem);
> +        ret = -EINVAL;
>                 goto out;
>         }
>  
> 
>>
>> ---
>> drivers/xen/privcmd.c |   16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>> index 8adb9cc..24aec2f 100644
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
>> @@ -383,12 +383,16 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>>
>> 	up_write(&mm->mmap_sem);
>>
>> -	if (state.global_error && (version == 1)) {
>> -		/* Write back errors in second pass. */
>> -		state.user_mfn = (xen_pfn_t *)m.arr;
>> -		state.err      = err_array;
>> -		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
>> -				     &pagelist, mmap_return_errors_v1, &state);
>> +	if (version == 1) {
>> +		if (state.global_error) {
>> +			/* Write back errors in second pass. */
>> +			state.user_mfn = (xen_pfn_t *)m.arr;
>> +			state.err      = err_array;
>> +			ret = traverse_pages(m.num, sizeof(xen_pfn_t),
>> +					     &pagelist, mmap_return_errors_v1, &state);
>> +		} else
>> +			ret = 0;
>> +
>> 	} else if (version == 2) {
>> 		ret = __copy_to_user(m.err, err_array, m.num * sizeof(int));
>> 		if (ret)
>> -- 
>> 1.7.9.5
>