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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
well.

>>>>> @@ -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.

Jan