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 <[email protected]> > 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
