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

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

On Sat, Apr 25, 2020 at 06:31:46AM -0600, Tamas K Lengyel wrote:
> On Sat, Apr 25, 2020 at 3:01 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > 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.
> 
> OK, I see what you mean. In the way things set up currently the EPT
> fault-loop problem doesn't happen since the page does get copied, it
> just gets copied to a new mfn not the one d->shared_info points to. So
> whatever issue that may bring it must be subtle because we haven't
> noticed any instability.

In any case I think it would be good if you could add such mapping on
domain fork and reset, as you already partially handle the shared info
page by copying the data from the parent into the child. Or at least
add a FIXME comment to note the fact that a child trying to access the
shared info page won't work.

> Also, looking at the save/restore code in libxc it seems to me that
> shared_info is actually a PV specific page and it doesn't get
> saved/restored with an HVM domain. The hvm code paths don't process
> REC_TYPE_SHARED_INFO at all. So since forks are exclusively for HVM
> domains, do we really need it and if so, why doesn't HVM save/restore
> need it?

HVM domains on suspend/resume are aware of the need to re-instate the
shared info mapping, as it's part of the protocol and the guest is
actively cooperating on such actions. The same happens with the vcpu
info pages or the PV timers for example: they are not saved during a
suspend/resume and the guest needs to setup them again on restore.

Fork however is completely transparent from a guest PoV, and hence
there's no way to signal the child it needs to setup the shared info
mapping (or any other mapping or interface FWIW).

Roger.