Re: [Xen-devel] [PATCH] xen/physmap: Propagate error rc from xenmem_add_to_physmap_one

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/16 11:43, Jan Beulich wrote:
>>>> On 07.12.16 at 02:07, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 07/12/2016 01:00, Jiandi An wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -762,6 +762,8 @@ static int xenmem_add_to_physmap_batch(struct domain *d,
>>>          rc = xenmem_add_to_physmap_one(d, xatpb->space,
>>>                                         xatpb->u,
>>>                                         idx, _gfn(gpfn));
>>> +        if ( rc < 0 )
>>> +            goto out;
>>>  
>>>          if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
>>>          {
>> This can't be correct.  You now skip writing rc into the errs[] array on
>> a failure, which means that userspace will get an overall failure but an
>> errs[] array which said that nothing went wrong.
>>
>> This code addition looks like it wants to be an "else if" on the end of
>> this if() in context.
> Why would this go elsewhere? It's unneeded - it's a property of the
> hypercall that when seeing overall success you still need to look at
> errs[].

Looking at the public header files, the error behaviour isn't even
documented.  (I'm going to try and remember to pick up on bugs like this
more closely during future review.)

Similar batch hypercalls return the index of the first failing
operation, which is a far more helpful behaviour for the caller.

Having said that, I can't actually see any in-tree callers.

~Andrew

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