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

[Julien Grall <julien@xxxxxxx>]

On 22/04/2020 14:43, Julien Grall wrote:
> Hi Juergen,
>
> On 22/04/2020 14:07, Juergen Gross wrote:
> > The GNTTABOP_cache_flush hypercall has a wrong test for hypercall
> > continuation, the test today is:
> >
> >      if ( rc > 0 || opaque_out != 0 )
> >
> > Unfortunately this will be true even in case of an error (rc < 0),
> > possibly leading to very long lasting hypercalls (times of more
> > than an hour have been observed in a test case).
> >
> > Correct the test condition to result in false with rc < 0 and set
> > opaque_out only if no error occurred, to be on the safe side.
> >
> > Partially-suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   xen/common/grant_table.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index 96080b3dec..5ef7ff940d 100644
> > --- 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 don't understand this change. If you look at the implementation of 
> gnttab_flush() it is not possible to have opaque_out non-zero with rc = 0.
Hmmm... I misread the code and missed the:

if ( hypercall_preempt_check() )
  return i;

Sorry for the noise.

I am also assuming this want a Fixes tag on 
18e8d22fe750c8c7b2830fa37961352693425cf1 "introduce GNTTABOP_cache_flush".
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

--
Julien Grall