Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2022 17:42, Roger Pau Monné wrote:
> On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
>> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't even be
>> an attempt to unshare the referenced page, without any indication to the
>> caller (e.g. -EAGAIN). Note that guests have no direct control over
>> which of their pages are shared (or paged out), and hence they have no
>> way to make sure all on their own that the subsequent obtaining of a
>> writable type reference can actually succeed.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> ---
>> Really I wonder whether the function wouldn't better use
>> check_get_page_from_gfn() _and_ permit p2m_ram_rw only. Otoh the P2M
>> type is stale by the time it is being looked at, so all depends on the
>> subsequent obtaining of a writable type reference anyway ...
>>
>> A similar issue then apparently exists in guest_wrmsr_xen() when writing
>> the hypercall page. Interestingly there p2m_is_paging() is being checked
>> for (but shared pages aren't).
> 
> Doesn't guest_wrmsr_xen() also needs to use UNSHARE?

I think so (hence I did say "A similar issue then apparently exists ...").
With the one caveat that a page that was already initialized as a hypercall
one (and was shared afterwards) wouldn't need to be unshared.

> I wonder if it would be helpful to introduce some kind of helper so
> that all functions can use it, get_guest_writable_page() or similar.

Maybe. Using check_get_page_from_gfn() would already help, I guess.

>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1484,7 +1484,7 @@ int map_vcpu_info(struct vcpu *v, unsign
>>      if ( (v != current) && !(v->pause_flags & VPF_down) )
>>          return -EINVAL;
>>  
>> -    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>> +    page = get_page_from_gfn(d, gfn, NULL, P2M_UNSHARE);
> 
> I had to go look up that P2M_UNSHARE implies P2M_ALLOC for the users
> of the parameter, it would be helpful to add a comment in p2m.h that
> UNSHARE implies ALLOC.

Same here, plus I needed to further figure out that the same implication
missing on Arm is okay merely because they ignore the respective argument.

Jan