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

[Jan Beulich <jbeulich@xxxxxxxx>]

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