Re: [PATCH v17 1/2] mem_sharing: fix sharability check during fork reset

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

On Fri, Apr 24, 2020 at 06:18:24AM -0600, Tamas K Lengyel wrote:
> On Fri, Apr 24, 2020 at 3:44 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Thu, Apr 23, 2020 at 08:30:06AM -0700, Tamas K Lengyel wrote:
> > > When resetting a VM fork we ought to only remove pages that were 
> > > allocated for
> > > the fork during it's execution and the contents copied over from the 
> > > parent.
> > > This can be determined if the page is sharable as special pages used by 
> > > the
> > > fork for other purposes will not pass this test. Unfortunately during the 
> > > fork
> > > reset loop we only partially check whether that's the case. A page's type 
> > > may
> > > indicate it is sharable (pass p2m_is_sharable) but that's not a sufficient
> > > check by itself. All checks that are normally performed before a page is
> > > converted to the sharable type need to be performed to avoid removing 
> > > pages
> > > from the p2m that may be used for other purposes. For example, currently 
> > > the
> > > reset loop also removes the vcpu info pages from the p2m, potentially 
> > > putting
> > > the guest into infinite page-fault loops.
> > >
> > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks!
> 
> >
> > I've been looking however and I'm not able to spot where you copy the
> > shared_info data, which I think it's also quite critical for the
> > domain, as it contains the info about event channels (when using L2).
> > Also for HVM forks the shared info should be mapped at the same gfn as
> > the parent, or else the child trying to access it will trigger an EPT
> > fault that won't be able to populate such gfn, because the shared_info
> > on the parent is already shared between Xen and the parent.
> 
> Pages that cause an EPT fault but can't be made shared get a new page
> allocated for them and copied in mem_sharing_fork_page. There are many
> pages like that, mostly due to QEMU mapping them and thus holding an
> extra reference. That said, shared_info is manually copied during
> forking in copy_special_pages, at the end of the function you will
> find:
> 
> old_mfn = _mfn(virt_to_mfn(d->shared_info));
> new_mfn = _mfn(virt_to_mfn(cd->shared_info));
> 
> copy_domain_page(new_mfn, old_mfn);

Yes, that's indeed fine, you need to copy the contents of the shared
info page, but for HVM you also need to make sure the shared info page
is mapped at the same gfn as the parent. HVM guest issue a hypercall
to request the mapping of the shared info page to a specific gfn,
since it's not added to the guest physmap by default.
XENMAPSPACE_shared_info is used in order to request the shared info
page to be mapped at a specific gfn.

AFAICT during fork such shared info mapping is not replicated to the
child, and hence the child trying to access the gfn of the shared info
page would just result in EPT faults that won't be fixed (because the
parent shared info page cannot be shared with the child)?

You should be able to use get_gpfn_from_mfn in order to get the parent
gfn where the shared info mfn is mapped (if any), and then replicate
this in the child using it's own shared info.

On fork reset you should check if the child shared info gfn != parent
shared info gfn and reinstate the parent state if different from the
child.

Roger.