| Message ID | 70ea4889e30ed35760329331ddfeb279fcd80786.1587655725.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v17,1/2] mem_sharing: fix sharability check during fork reset | expand |
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@intel.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> 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. Thanks, Roger.
On Fri, Apr 24, 2020 at 3:44 AM Roger Pau Monné <roger.pau@citrix.com> 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@intel.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> 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); Cheers, Tamas
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@citrix.com> 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@intel.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > 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.
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@citrix.com> 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@intel.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > 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.
On Sat, Apr 25, 2020 at 3:01 AM Roger Pau Monné <roger.pau@citrix.com> 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@citrix.com> 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@intel.com> > > > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > 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. 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? Tamas
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@citrix.com> 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@citrix.com> 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@intel.com> > > > > > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > > 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.
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index bb74595351..344a5bfb3d 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -633,31 +633,33 @@ unsigned int mem_sharing_get_nr_shared_mfns(void) /* Functions that change a page's type and ownership */ static int page_make_sharable(struct domain *d, struct page_info *page, - int expected_refcnt) + unsigned int expected_refcnt, + bool validate_only) { - bool_t drop_dom_ref; + int rc = 0; + bool drop_dom_ref = false; - spin_lock(&d->page_alloc_lock); + spin_lock_recursive(&d->page_alloc_lock); if ( d->is_dying ) { - spin_unlock(&d->page_alloc_lock); - return -EBUSY; + rc = -EBUSY; + goto out; } /* Change page type and count atomically */ if ( !get_page_and_type(page, d, PGT_shared_page) ) { - spin_unlock(&d->page_alloc_lock); - return -EINVAL; + rc = -EINVAL; + goto out; } /* Check it wasn't already sharable and undo if it was */ if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) { - spin_unlock(&d->page_alloc_lock); put_page_and_type(page); - return -EEXIST; + rc = -EEXIST; + goto out; } /* @@ -666,20 +668,26 @@ static int page_make_sharable(struct domain *d, */ if ( page->count_info != (PGC_allocated | (2 + expected_refcnt)) ) { - spin_unlock(&d->page_alloc_lock); /* Return type count back to zero */ put_page_and_type(page); - return -E2BIG; + rc = -E2BIG; + goto out; } - page_set_owner(page, dom_cow); - drop_dom_ref = !domain_adjust_tot_pages(d, -1); - page_list_del(page, &d->page_list); - spin_unlock(&d->page_alloc_lock); + if ( !validate_only ) + { + page_set_owner(page, dom_cow); + drop_dom_ref = !domain_adjust_tot_pages(d, -1); + page_list_del(page, &d->page_list); + } + +out: + spin_unlock_recursive(&d->page_alloc_lock); if ( drop_dom_ref ) put_domain(d); - return 0; + + return rc; } static int page_make_private(struct domain *d, struct page_info *page) @@ -810,7 +818,8 @@ static int debug_gref(struct domain *d, grant_ref_t ref) } static int nominate_page(struct domain *d, gfn_t gfn, - int expected_refcnt, shr_handle_t *phandle) + unsigned int expected_refcnt, bool validate_only, + shr_handle_t *phandle) { struct p2m_domain *hp2m = p2m_get_hostp2m(d); p2m_type_t p2mt; @@ -879,8 +888,8 @@ static int nominate_page(struct domain *d, gfn_t gfn, } /* Try to convert the mfn to the sharable type */ - ret = page_make_sharable(d, page, expected_refcnt); - if ( ret ) + ret = page_make_sharable(d, page, expected_refcnt, validate_only); + if ( ret || validate_only ) goto out; /* @@ -1392,13 +1401,13 @@ static int range_share(struct domain *d, struct domain *cd, * We only break out if we run out of memory as individual pages may * legitimately be unsharable and we just want to skip over those. */ - rc = nominate_page(d, _gfn(start), 0, &sh); + rc = nominate_page(d, _gfn(start), 0, false, &sh); if ( rc == -ENOMEM ) break; if ( !rc ) { - rc = nominate_page(cd, _gfn(start), 0, &ch); + rc = nominate_page(cd, _gfn(start), 0, false, &ch); if ( rc == -ENOMEM ) break; @@ -1478,7 +1487,7 @@ int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool unsharing) /* For read-only accesses we just add a shared entry to the physmap */ while ( parent ) { - if ( !(rc = nominate_page(parent, gfn, 0, &handle)) ) + if ( !(rc = nominate_page(parent, gfn, 0, false, &handle)) ) break; parent = parent->parent; @@ -1775,20 +1784,22 @@ static int mem_sharing_fork_reset(struct domain *d, struct domain *pd) spin_lock_recursive(&d->page_alloc_lock); page_list_for_each_safe(page, tmp, &d->page_list) { - p2m_type_t p2mt; - p2m_access_t p2ma; + shr_handle_t sh; mfn_t mfn = page_to_mfn(page); gfn_t gfn = mfn_to_gfn(d, mfn); - mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &p2mt, &p2ma, - 0, NULL, false); - - /* only reset pages that are sharable */ - if ( !p2m_is_sharable(p2mt) ) - continue; - - /* take an extra reference or just skip if can't for whatever reason */ - if ( !get_page(page, d) ) + /* + * We only want to remove pages from the fork here that were copied + * from the parent but could be potentially re-populated using memory + * sharing after the reset. These pages all must be regular pages with + * no extra reference held to them, thus should be possible to make + * them sharable. Unfortunately p2m_is_sharable check is not sufficient + * to test this as it doesn't check the page's reference count. We thus + * check whether the page is convertable to the shared type using + * nominate_page. In case the page is already shared (ie. a share + * handle is returned) then we don't remove it. + */ + if ( (rc = nominate_page(d, gfn, 0, true, &sh)) || sh ) continue; /* forked memory is 4k, not splitting large pages so this must work */ @@ -1797,7 +1808,7 @@ static int mem_sharing_fork_reset(struct domain *d, struct domain *pd) ASSERT(!rc); put_page_alloc_ref(page); - put_page(page); + put_page_and_type(page); } spin_unlock_recursive(&d->page_alloc_lock); @@ -1839,7 +1850,7 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg) { shr_handle_t handle; - rc = nominate_page(d, _gfn(mso.u.nominate.u.gfn), 0, &handle); + rc = nominate_page(d, _gfn(mso.u.nominate.u.gfn), 0, false, &handle); mso.u.nominate.handle = handle; } break; @@ -1854,7 +1865,7 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg) if ( rc < 0 ) goto out; - rc = nominate_page(d, gfn, 3, &handle); + rc = nominate_page(d, gfn, 3, false, &handle); mso.u.nominate.handle = handle; } break;
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@intel.com> --- v17: Changes based on feedback from Roger --- xen/arch/x86/mm/mem_sharing.c | 83 ++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 36 deletions(-)