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

Re: [PATCH v8 06/16] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 1 Feb 2021 14:04:31 +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=hW4ZU4gQ3UGSA87v9AW0hVKlpA7k7ugSAiZ7f1Na3Rw=; b=bQogFHT8CzckcJ0u99myTq12/naZ0XOj90D3v7AO5D/SaqYMv4+dYKf4Q5ZxCQvTOg0croaxPkfaY5XbqAuym6nPGYDO9lRwLKpi1WRDuK5P51uyEiDB3w6ZV3Ey/6KM9ZSFDizrYYcp1/F1mIYSlOTrpkmPcAAJJflU0tzc/fqVI2u1MriKvneoffhxQkjve1eeLItuYrglg8zV/tTtpTc7lSHPMJuF24Hk+DXL8J0wNgTpjooce+PcnYQemDnGOl9zRGPtTA4TldVmr1p+yBX17SUrAy9FUFBAPr6RdYsRaHXKcyGG+fHZhQ7dGeqzagiMzyrufqfIHs7uAv/PcQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CZDWpxHR7ZlY8muUBnINGWIga4vxBZJQuSFSsZb89b5jB5APwUJUmZ5mIp5NTE+RFNLfb3UBqMzAFcXqyC8u+OMz+3bd6pAIb1RBOVrN8aaeEdEo1//UxAR/9upEd6UUr8MkxsOQCP9PAzGnpfc64NAsx544XhEZqi2BHfg9S4Tsq3cHeUsqoFHE4TwX59vcvEhwmHSCdS5ylXU9faGtfvmgXEekDTBUdKq2F9auRteDYsWZcom9WZDIKIzPtAXVX31iczFNzkHKCPYxAlaGgUQefFcgxfdVUx+sebXo/I6OlQOCM4/3qzGSD1IC/CRBk7fRXMa8kdBgQDvxkO4xkQ==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, Michał Leszczyński <michal.leszczynski@xxxxxxx>, Hubert Jasudowicz <hubert.jasudowicz@xxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Mon, 01 Feb 2021 14:04:59 +0000
  • Ironport-sdr: ekg0SIFOziXn37hvBr3A0eEAPIA57Il8D9Eqso+E+AhA0yQjQrX7Hv6KdHyZWx2H2qg0ekAFln NTutp79kenxuG0jv+cmiOGjmJvBVkjjmj0wEAxVLL65FPTzGcJ3KbJRX7DBxAu+LJELjFjRYoQ cTaTbZ3qTRFncnW658YHYSDNuppu0Ek3kYRSamC3l4AYJ8Zj6gAIkWJpZd7sUAeB8n74YYxGlB 8nJyCRmCAVpgA402Ot0uoNPeQ93h/ym5NHsPArSew6iy7yUaNO1xBFQy+b8u8b3rEzbdGnPNan JaI=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 01/02/2021 13:03, Jan Beulich wrote:
> On 01.02.2021 12:11, Andrew Cooper wrote:
>> On 01/02/2021 10:10, Roger Pau Monné wrote:
>>> On Sat, Jan 30, 2021 at 02:58:42AM +0000, Andrew Cooper wrote:
>>>> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd, 
>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>>                      compat_frame_list[i] = frame;
>>>>                  }
>>>>  
>>>> -                if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
>>>> +                if ( __copy_to_compat_offset(cmp.mar.frame_list, 
>>>> start_extent,
>>>>                                               compat_frame_list,
>>>> -                                             cmp.mar.nr_frames) )
>>>> +                                             done) )
>>>>                      return -EFAULT;
>>> Is it fine to return with a possibly pending continuation already
>>> encoded here?
>>>
>>> Other places seem to crash the domain when that happens.
>> Hmm.  This is all a total mess.  (Elsewhere the error handling is also
>> broken - a caller who receives an error can't figure out how to recover)
>>
>> But yes - I think you're right - the only thing we can do here is `goto
>> crash;` and woe betide any 32bit kernel which passes a pointer to a
>> read-only buffer.
> I'd like to ask you to reconsider the "goto crash", both the one
> you mention above and the other one already present in the patch.
> Wiring all the cases where we mean to crash the guest into a
> single domain_crash() invocation has the downside that when
> observing such a case one can't remotely know which path has led
> there. Therefore I'd like to suggest individual domain_crash()
> invocations on every affected path. Elsewhere in the file there
> already is such an instance, commented "Cannot cancel the
> continuation...".

But they're all logically the same, are they not?

~Andrew



 


Rackspace

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