Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.04.19 at 18:33, <julien.grall@xxxxxxx> wrote:
> Hi,
> 
> On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>>> On 09.04.19 at 11:50, <julien.grall@xxxxxxx> wrote:
>>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>>> On 08.04.19 at 13:47, <julien.grall@xxxxxxx> wrote:
>>>>> de-allocation step aside, I am not really convinced you can reuse
>>>>> guest_remove_page() here. On x86, the function will not work on certain
>>>>> p2m types. Is it what we really want?
>>>>
>>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>>> I perhaps overlooking something you see?
>>>>
>>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>>> specific)? This ought to be covered by the p2m_is_paging() and
>>>> p2m_mmio_direct special cases a few lines up from there. Other
>>>> cases with invalid MFNs would indeed represent an error condition
>>>> imo.
>>>
>>> I am referring to get_page(...) which would not work for foreign pages.
>> 
>> Ah, that's part of the de-allocation portion of the code, which I've
>> previously indicated would need to be skipped in the case here.
>> 
>>>> In the end it's actually quite the opposite that I'm thinking: For
>>>> the caller it shouldn't, for example, matter whether the
>>>> requested page was paged out. We wouldn't even call
>>>> guest_physmap_remove_page() in that case, while
>>>> guest_remove_page() would take care of it.
>>>
>>> But those pages should never be removed via physmap, I am correct?
>> 
>> The guest is unaware of paging, so as long as it's permitted to
>> use this hypercall, it should make no difference to it whether a
>> page is actually in memory at the time it issues this hypercall.
> 
> Well, I only agree with that statement if it is possible to map page 
> that can be page out with one of the physmap hypercall.
> 
> If it is not possible, then I don't think we should allow the guest to 
> remove those pages.

From the looks of it XENMAPSPACE_gmfn mapping would simply
fail for paged-out pages. Hence on one hand I agree with you
that "add" and "remove" should act similarly. Otoh though I
think we'd widen a problem here, because to me it looks like
passing a GFN of a paged out page to add-to-physmap should
work (and transparently to the guest).

> One of my main concern is a guest trying to mistakenly use 
> XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your 
> point above, the former would not do de-allocation. So you would end up 
> losing the page forever (there are no way to get the page back for 
> auto-translated guest).

Correct - to me that's implied from the sub-function name.
Just like add-to-physmap doesn't allocate anything, remove-
from-physmap doesn't free.

> I realize the issue is already present (at least on Arm) today. But if 
> we were going to modify XENME_remove_from_physmap, then a bit more 
> safety check to avoid a guest shooting its own foot would be useful.

I'm not sure I see ways of properly checking for such
situations - right after any such check the information gathered
might already be stale. And I don't think we go to great lengths
to prevent guests shooting themselves in the foot by other
means.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel