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

[Julien Grall <julien@xxxxxxx>]

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.
So what's the point of the second condition?

>       {
>           /* Adjust rc, see gnttab_copy() for why this is needed. */
>           if ( cmd == GNTTABOP_copy )
Cheers,

--
Julien Grall