[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 15 Jan 2021 14:26:44 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ICUvZtjDqtpibEfBWO1fKJyyaw+aNbSXYbWF73FoXeo=; b=frR5BMZpHi3c9QyFeW7GlcXxtIb6zKZ9ZVp5gGlklNiT9hZIZER4uRfVV2b5WE0k4Nse5Luab84eH54oNpse1tDHLB+ddmcChs239q0w2GUdwmcAf+cvVCfNpK6QM/BmPn/TGWpiMK2JzJHX9UFI6TYygxnBo449IsY3iJ21TfNynHyHa5EZdHV9c4vPlxh7Qs+xn49MkISicPQOzcOnaKSIiU7lPeXS6ghZXoCFk9wX4/htSl9FNobMdkIGJGoABOFUR7mx1cBWFVODXyiNwe0Yi7zkK8PfDFOOH/DI3Uy8z9CPsnAbHoHvANKkAg2OXqRpmWzHi/LdCJZBVnIfUg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FQczv/k36Itan4PM62fxFQH2MWnVcQOlL/B3uxA1StKLVnn26JjdRMxXirr/VWRgcsu+Md1Ha3u12Y6p+Oer4P6gWfm6TJo/ntoKpHO/8Yn2hV9wfa+NIPmc4Fz5ezhLa+L5kOJnh8KEmDS5qbieS053ox9sks2owK2fXy48NfSeaBSaZsrOcCUCjLqeMKEW70EmWajHZJjTQEP4crKyMwBVZqinazeiEzP7c0pAOgIi/cqLGYMeCkWJmcoIroRReF5G381bJp97LCYGT94CsCiu0yvlKgHoTH0QxlSjqxRvfvezUoorGW43NKSN4bk9oezxa6p5+Zqk2ce8XVYIug==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 15 Jan 2021 14:27:01 +0000
  • Ironport-sdr: I3HwSakoEXx2Qz7PI40Nv7+b8H+yUHXoyNu+aMFhs1wW6u3YlbFTZDbXt/AzZeFqt1QzBNvn9/ UmC665C0YtmVO+0djjzgvAgJlDQ5FH53YtssSpmmIXzOpMmpYRobRyy88mv+evLV0CGhdt+iVO 7w2H3ifHRtX7MLRXw0C90ZyFJejV8rmTCVQfuGd6tHVmEyXETPngp9b8dKlo4IIiQbnLaqvpIR LsJYpyTkXBmmkmU4JKOv4d4hOxDGgFpn5gtJsYsQ0UcvFVKoFWMQMmMLJtbC7yC6RDoJHes3fA OxQ=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.