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);

Oh, sorry for the noise, I somehow missed it.

Roger.