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

Re: [PATCH v5 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 7 Jul 2021 16:48:46 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=06Th48dZbeVUBJurg+bDQkzqgN7ZY/biMHtK3UQk77s=; b=fMPVy7G6V/fcLoVeLlHKdH4pHoFC/rkobMSj7oyfiiK44naG81Qo7lXCFlsiwHrppxll/NRTbhPaTT5muT0PRBm21JSkmcyfDPVMQRWkPQI3bUJHWs96im02SaEQT/UD83NdJTh5qec7U2qrLMUBvsJC8/shnE46LmVE/YvDaDuO0I51ITcSpln5vMLV8uGZXjw3t0cZ40z1DVCHDj19RBypzAMlb1fo07t2Zf0cYZewMBv6v3LOhIncMz3BzAWE817Xtc1TzRJHi2e9FX7LHvAO7iAoBaewNaX+FiDezjaOxJK/iWZbbtee4gvsClVyK+9xSiwj9FX8mBKXmhGW2g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BUr0jS02B1lcUZ+FJJC0MTjY2kAZPMsuGM2LRxjOvvsLNnsaCfFNlfVbMtT4C1JLpFMRAVgdrxMiQPNHsvPh5pvPvCVwoz76qMrAuy0owUWt1kwXSqmZU/R/WeIjV3M1qIHLxeeq8OA1GU7LvBCadvTpmKt42cIoPiu1aCu5SaW9UW9CkSDTAjRmDhQERwdMKCplVlY5hIdg+tjDLeCOI1IkcgXSuuTHKPClxz8691yHAwfSnjxnUaxB2pm5j0q++14Qbq64JtTAFfNDU8Wjb+wDyvgKn4npJGQwnO0vNAjj/htSW/0H8TfVcbLdix2XQA07GvgWZpWJ4lAT149yPw==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Julien Grall <jgrall@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 07 Jul 2021 14:48:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 07.07.2021 16:21, Julien Grall wrote:
> On 07/07/2021 15:06, Jan Beulich wrote:
>> On 07.07.2021 15:39, Julien Grall wrote:
>>> On 05/07/2021 09:41, Jan Beulich wrote:
>>>> On 03.07.2021 19:11, Julien Grall wrote:
>>>>> Changes in v5:
>>>>>       - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>>>>>       - Used paging_mode_translate() rather than is_pv_domain()
>>>> Is there a particular reason you use this in favor of steal_page()'s
>>>> paging_mode_external()?
>>> This is what you suggested in v4 [1]. I can switch to
>>> paging_mode_external() if this is what you now prefer.
>> Well, I did say this would be better than is_pv_*(). I probably didn't
>> pay enough attention to you already pointing out paging_mode_external()
>> in the description; I'm sorry. On x86 both are in sync anyway, and I
>> have to admit I don't see clearly enough which of the two would be the
>> right one to use here, partly because I'm afraid I also don't see why
>> steal_page() has such a restriction in the first place (which you now
>> build upon).
>  From a quick git blame, I have found this:
> commit fae7d5be8bb8b7a5b7005c4f3b812a47661a721e
> Author: Jan Beulich <jbeulich@xxxxxxxx>
> Date:   Tue Jun 20 14:29:51 2017 +0200
>      x86/mm: disallow page stealing from HVM domains
>      The operation's success can't be controlled by the guest, as the device
>      model may have an active mapping of the page. If we nevertheless
>      permitted this operation, we'd have to add further TLB flushing to
>      prevent scenarios like
>      "Domains A (HVM), B (PV), C (PV); B->target==A
>       Steps:
>       1. B maps page X from A as writable
>       2. B unmaps page X without a TLB flush
>       3. A sends page X to C via GNTTABOP_transfer
>       4. C maps page X as pagetable (potentially causing a TLB flush in C,
>       but not in B)
>       At this point, X would be mapped as a pagetable in C while being
>       writable through a stale TLB entry in B."
>      A similar scenario could be constructed for A using XENMEM_exchange and
>      some arbitrary PV domain C then having this page allocated.
>      This is XSA-217.

Well, yes, this was to repair the damage which could result. But it doesn't
explain why page stealing couldn't be made work for translated guests as

>>>>> @@ -815,6 +812,9 @@ static long 
>>>>> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>>>>        if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>>>>>            rc = -EFAULT;
>>>> I'm afraid that for correctness of the interface you need to keep
>>>> this part even in the !PV case.
>>> Xen never initializes the field nr_exchanged. Instead, it expects the
>>> guest to set to 0. So I am not quite to sure why we would need to keep
>>> this line.
>> Hmm, the public header is wrong then, as it documents the field as
>> [OUT] only _despite_ the shouting warning in point 5 of the comment.
> That's confusing... I will look to update the doc.
>> I guess I never really understood why this sub-op differs from
>> others in where the continuation indicator lives.
> I am guessing the continuation was added after the fact without 
> coordination?

I don't think so, no. I rather expect someone to have been in a different
mood that day.




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