Re: [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.02.2020 19:46, Andrew Cooper wrote:
> Each function takes an unsigned count, and loops based on a signed i.  This
> causes problems when between 2 and 4 billion operations are requested.

I can see problems, but not (as the title says) with comparison issues
(the signed i gets converted to unsigned for the purpose of the
comparison).

> In practice, signed-ness issues aren't possible because count exceeding
> INT_MAX is excluded earlier in do_grant_op(), but the code reads as if it is
> buggy, and GCC obviously can't spot this either.
> 
> Bloat-o-meter reports:
>   add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-95 (-95)
>   Function                                     old     new   delta
>   do_grant_table_op                           7155    7140     -15
>   gnttab_transfer                             2732    2716     -16
>   gnttab_unmap_grant_ref                       771     739     -32
>   gnttab_unmap_and_replace                     771     739     -32
>   Total: Before=2996364, After=2996269, chg -0.00%
> 
> and inspection of gnttab_unmap_grant_ref() at least reveals one fewer local
> variables on the stack.

This is a good thing for x86. However, having started to familiarize
myself a tiny bit with RISC-V, I'm no longer certain our present way
of preferring unsigned int for array indexing variable and alike is
actually a good thing without further abstraction. Nevertheless, this
isn't an immediate issue, so ...

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit preferably with the title adjusted, and with one more
suggestion:

> @@ -1568,13 +1568,14 @@ static long
>  gnttab_unmap_grant_ref(
>      XEN_GUEST_HANDLE_PARAM(gnttab_unmap_grant_ref_t) uop, unsigned int count)
>  {
> -    int i, c, partial_done, done = 0;
> +    unsigned int i, partial_done, done = 0;
>      struct gnttab_unmap_grant_ref op;
>      struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
>  
>      while ( count != 0 )
>      {
> -        c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
> +        unsigned int c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);

Seeing this and another instance further down, would you mind
dropping the cast on this occasion, in favor of adding a U
suffix to GNTTAB_UNMAP_BATCH_SIZE's definition?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel