[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/3] gnttab: adjust pin count overflow checks
On 15/01/2021 14:21, Jan Beulich wrote: > 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)? Yeah - lets go with this. Its a massive improvement on the current code. ~Andrew
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |