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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2021 14:35, Andrew Cooper wrote:
> On 15/01/2021 13:26, Jan Beulich wrote:
>> On 15.01.2021 14:09, Andrew Cooper wrote:
>>> On 14/01/2021 15:23, Jan Beulich wrote:
>>>> @@ -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.
> 
> Hmm - I see your point.  I'd missed the fact that a previous operation
> could leave this timebomb waiting for us.  (Probably wants a note to
> this effect in the commit message).

I've added half a sentence.

> However we go about fixing it, "pin_incr << (GNTPIN_cntr_width - 1)" is
> too obscure IMO.  Perhaps all we need is a
> 
> #define GNTPIN_overflow_mask(x) ((x) << (GNTPIN_cntr_width - 1))
> 
> but it does occur to me that this logic only works to being with when
> pin_incr is strictly 1 in the relevant bytes.

Perhaps

#define GNTPIN_overflow_mask(x) ({ \
    ASSERT(!((x) & ~(GNTPIN_hstw_inc | GNTPIN_hstr_inc | \
                     GNTPIN_devw_inc | GNTPIN_devr_inc))); \
    (x) << (GNTPIN_cntr_width - 1); \
})

? And maybe name the whole thing e.g. GNTPIN_incr2oflow_mask()
to clarify the input is an increment (which I personally take
to mean consisting of only values of 1, but I realize views
may vary)?

Jan