[PATCH] xen/physmap: Propagate error rc from xenmem_add_to_physmap_one

[PATCH] xen/physmap: Propagate error rc from xenmem_add_to_physmap_one

[Jiandi An]

Error return code from xenmem_add_to_physmap_one() is not properly
handled in xenmem_add_to_physmap_batch().  This causes do_memory_op()
to return success to guest even though the underlying memory map fails.

Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
---
 xen/common/memory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 21797ca..4e46258 100644
--- 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)) )
         {
-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

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

[Andrew Cooper]

On 07/12/2016 01:00, Jiandi An wrote:
> Error return code from xenmem_add_to_physmap_one() is not properly
> handled in xenmem_add_to_physmap_batch().  This causes do_memory_op()
> to return success to guest even though the underlying memory map fails.
>
> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
> ---
>  xen/common/memory.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 21797ca..4e46258 100644
> --- 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.

~Andrew

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

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

[Jan Beulich]

>>> On 07.12.16 at 02:07, <andrew.cooper3@citrix.com> 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[].

Jan


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

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

[Andrew Cooper]

On 07/12/16 11:43, Jan Beulich wrote:
>>>> On 07.12.16 at 02:07, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jiandi An]

On 12/07/16 05:43, Jan Beulich wrote:
>>>> On 07.12.16 at 02:07, <andrew.cooper3@citrix.com> 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[].
> 
> Jan
> 

I realized the issue is on the guest side errs[] is not being checked
after the heypercall.  Will fix on the guest side.

-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

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

[Jan Beulich]

>>> On 07.12.16 at 18:09, <andrew.cooper3@citrix.com> wrote:
> On 07/12/16 11:43, Jan Beulich wrote:
>>>>> On 07.12.16 at 02:07, <andrew.cooper3@citrix.com> 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.)

I agree it would be nice for behavior to be spelled out if it's
possibly unexpected, but ...

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

... this is not really an option here. It would make sense if the
operation stopped at that slot, but that's not the case. errs[]
needs to be scanned in full anyway to find all (and not just
the first) error slots. Furthermore positive return values are
used by the function to indicate the need for a continuation
to be invoked.

Jan


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