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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>

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

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

Thanks, Roger.