[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/grants: fix hypercall continuation for GNTTABOP_cache_flush



On 22.04.2020 18:31, Stefano Stabellini wrote:
> On Wed, 22 Apr 2020, Jan Beulich wrote:
>> On 22.04.2020 15:07, Juergen Gross wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -3626,12 +3626,12 @@ do_grant_table_op(
>>>          if ( unlikely(!guest_handle_okay(cflush, count)) )
>>>              goto out;
>>>          rc = gnttab_cache_flush(cflush, &opaque_in, count);
>>> -        if ( rc > 0 )
>>> +        if ( rc >= 0 )
>>>          {
>>>              guest_handle_add_offset(cflush, rc);
>>>              uop = guest_handle_cast(cflush, void);
>>> +            opaque_out = opaque_in;
>>>          }
>>> -        opaque_out = opaque_in;
>>>          break;
>>>      }
>>>  
>>> @@ -3641,7 +3641,7 @@ do_grant_table_op(
>>>      }
>>>  
>>>    out:
>>> -    if ( rc > 0 || opaque_out != 0 )
>>> +    if ( rc > 0 || (opaque_out != 0 && rc == 0) )
>>
>> I disagree with this part - opaque_out shouldn't end up non-zero
>> when rc < 0, and it won't anymore with the change in the earlier
>> hunk.
> 
> But opaque_out could end up being non-zero when rc == 0.

Which is the case the original code meant to deal with. (I still
think it is unfortunate behavior of the cache-flush implementation
to assign meaning other than "success, done" to rc == 0.)

> I think it is a
> good improvement that we explicitly prevent rc < 0 from entering this
> if condition. This is why this new version of the patch is my favorite:
> it is the one that leads to the code most robust. 
> 
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

Well, looks like I'm outvoted then. Nevertheless thanks ...

> That said, as I mentioned before, I would be OK even without the last
> part because it would still be enough to fix the bug.

.. for also clarifying this.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.