Re: [Xen-devel] [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Nov 16, 2012, at 10:43 AM, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:

> 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@xxxxxxxxxx>
>>> Acked-by: David Vrabel <david.vrabel@xxxxxxxxxx>
>> 
>> 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.

You mean the code structure becoming similar to a switch (version) { … }, 
instead of collapsing multiple conditions in the first if?

Ok, I guess.

Both patches are fine, I

acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

Mats's patch, and they should be merged imho to make the logic clearer.

Andres
> 
> 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
>> 
> 


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