Re: [PATCH 1/3] gnttab: adjust pin count overflow checks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2021 14:09, Andrew Cooper wrote:
> On 14/01/2021 15:23, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -323,22 +323,25 @@ shared_entry_header(struct grant_table *
>>  /* Active grant entry - used for shadowing GTF_permit_access grants. */
>>  struct active_grant_entry {
>>      uint32_t      pin;    /* Reference count information:             */
>> +                          /* Width of the individual counter fields.  */
>> +#define GNTPIN_cntr_width    8
>> +#define GNTPIN_cntr_mask     ((1U << GNTPIN_cntr_width) - 1)
>>                            /* Count of writable host-CPU mappings.     */
>>  #define GNTPIN_hstw_shift    0
>>  #define GNTPIN_hstw_inc      (1U << GNTPIN_hstw_shift)
>> -#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
>> +#define GNTPIN_hstw_mask     (GNTPIN_cntr_mask << GNTPIN_hstw_shift)
>>                            /* Count of read-only host-CPU mappings.    */
>> -#define GNTPIN_hstr_shift    8
>> +#define GNTPIN_hstr_shift    (GNTPIN_hstw_shift + GNTPIN_cntr_width)
> 
> While this patch is by-and-large an improvement, it unfortunately
> further hides how the pin field works, which is already clear-as-mud.
> 
> I'd recommend replacing the "Reference count information:" comment with:
> 
> /*
>  * 4x byte-wide reference counts, for {host,device}{read,write}
>  * mappings, implemented as a single 32bit presumably to optimise
>  * checking for any reference.
>  */
> uint32_t      pin;

Sure, added.

>> @@ -1052,19 +1063,19 @@ map_grant_ref(
>>      shah = shared_entry_header(rgt, ref);
>>      act = active_entry_acquire(rgt, ref);
>>  
>> -    /* Make sure we do not access memory speculatively */
>> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
>> -                                                 : &status_entry(rgt, ref);
>> -
>>      /* If already pinned, check the active domid and avoid refcnt overflow. 
>> */
>>      if ( act->pin &&
>>           ((act->domid != ld->domain_id) ||
>> -          (act->pin & 0x80808080U) != 0 ||
>> +          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||
> 
> This, I'm afraid, is not an improvement.  What we want is:
> 
> #define GNTPIN_overflow_mask 0x80808080U
> 
> The reason for checking all at once is defence in depth (not strictly
> necessary, but also not a problem),

How is this not a problem? There is absolutely no reason to
reject a request just because some unrelated field may be
about to overflow. In fact I now think that I didn't even
leverage the full potential - the "act->pin != old_pin" check
could also be relaxed this way, I think. Just that it sits on
a path we probably don't really care about very much.

> and in this specific example, using
> a constant results in better code because pin_incr doesn't need
> unspilling from stack and manipulated.

That's, I think, an acceptable price to pay for getting the
checking correct.

> If you're happy with both of these suggestions, then Reviewed-by: Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx> to avoid a repost.

Thanks, but as per above - not yet. Plus if I made this 2nd
suggested change, the patch would change significantly (the
new local variable would then become pointless in at least
acquire_grant_for_copy()), which I think would better involve
re-posting then.

Jan