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 <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]> > --- > 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.
