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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


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.