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