From 0806e3965914990286b62c120e95f09e4962152c Mon Sep 17 00:00:00 2001 From: Tim Deegan Date: Thu, 15 Mar 2012 11:12:44 +0000 Subject: [PATCH] Memory sharing: better handling of ENOMEM while unsharing If unsharing fails with ENOMEM, we were: - leaving the list of gfns backed by the shared page in an inconsistent state - cycling forever on the hap page fault handler. - Attempting to produce a mem event (which could sleep on a wait queue) while holding locks. - Not checking, for all callers, that unshare could have indeed failed. Fix bugs above, and sanitize callers to place a ring event in an unlocked context, or without requiring to go to sleep on a wait queue. A note on the rationale for unshare error handling: 1. Unshare can only fail with ENOMEM. Any other error conditions BUG_ON() 2. We notify a potential dom0 helper through a mem_event ring. But we allow the notification to not go to sleep. If the event ring is full of ENOMEM warnings, then the helper will already have been kicked enough. 3. We cannot "just" go to sleep until the unshare is resolved, because we might be buried deep into locks (e.g. something -> copy_to_user -> __hvm_copy) 4. So, we make sure we: 4.1. return an error 4.2. do not corrupt memory shared with other guests 4.3. do not corrupt memory private to the current guest 4.4. do not corrupt the hypervisor memory sharing meta data 4.5. let the guest deal with the error, if propagation will reach that far Signed-off-by: Andres Lagar-Cavilla Acked-by: Tim Deegan Committed-by: Tim Deegan --- xen/arch/x86/hvm/hvm.c | 23 +++++++++++++- xen/arch/x86/mm.c | 8 +++-- xen/arch/x86/mm/mem_sharing.c | 52 +++++++++++++++++++------------ xen/arch/x86/mm/p2m.c | 18 ++++++++++- xen/common/grant_table.c | 11 ++++--- xen/common/memory.c | 1 + xen/include/asm-x86/mem_sharing.h | 27 +++++++++++++++- 7 files changed, 109 insertions(+), 31 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 85b18a604e..bf7d7f7bd9 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1231,6 +1231,9 @@ int hvm_hap_nested_page_fault(unsigned long gpa, struct vcpu *v = current; struct p2m_domain *p2m; int rc, fall_through = 0, paged = 0; +#ifdef __x86_64__ + int sharing_enomem = 0; +#endif mem_event_request_t *req_ptr = NULL; /* On Nested Virtualization, walk the guest page table. @@ -1340,7 +1343,8 @@ int hvm_hap_nested_page_fault(unsigned long gpa, if ( access_w && (p2mt == p2m_ram_shared) ) { ASSERT(!p2m_is_nestedp2m(p2m)); - mem_sharing_unshare_page(p2m->domain, gfn, 0); + sharing_enomem = + (mem_sharing_unshare_page(p2m->domain, gfn, 0) < 0); rc = 1; goto out_put_gfn; } @@ -1381,8 +1385,25 @@ int hvm_hap_nested_page_fault(unsigned long gpa, out_put_gfn: put_gfn(p2m->domain, gfn); out: + /* All of these are delayed until we exit, since we might + * sleep on event ring wait queues, and we must not hold + * locks in such circumstance */ if ( paged ) p2m_mem_paging_populate(v->domain, gfn); +#ifdef __x86_64__ + if ( sharing_enomem ) + { + int rv; + if ( (rv = mem_sharing_notify_enomem(v->domain, gfn, 1)) < 0 ) + { + gdprintk(XENLOG_ERR, "Domain %hu attempt to unshare " + "gfn %lx, ENOMEM and no helper (rc %d)\n", + v->domain->domain_id, gfn, rv); + /* Crash the domain */ + rc = 0; + } + } +#endif if ( req_ptr ) { mem_access_send_req(v->domain, req_ptr); diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index c7fd5f785e..006e37f3cf 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -3597,12 +3597,14 @@ int do_mmu_update( /* Unshare the page for RW foreign mappings */ if ( l1e_get_flags(l1e) & _PAGE_RW ) { - rc = mem_sharing_unshare_page(pg_owner, - l1e_get_pfn(l1e), - 0); + unsigned long gfn = l1e_get_pfn(l1e); + rc = mem_sharing_unshare_page(pg_owner, gfn, 0); if ( rc ) { put_gfn(pg_owner, l1egfn); + /* Notify helper, don't care about errors, will not + * sleep on wq, since we're a foreign domain. */ + (void)mem_sharing_notify_enomem(pg_owner, gfn, 0); break; } } diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 50d3d6ed6d..ad7738a437 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -344,34 +344,29 @@ int mem_sharing_audit(void) #endif -static void mem_sharing_notify_helper(struct domain *d, unsigned long gfn) +int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn, + bool_t allow_sleep) { struct vcpu *v = current; + int rc; mem_event_request_t req = { .gfn = gfn }; - if ( v->domain != d ) - { - /* XXX This path needs some attention. For now, just fail foreign - * XXX requests to unshare if there's no memory. This replaces - * XXX old code that BUG()ed here; the callers now BUG() - * XXX elewhere. */ - gdprintk(XENLOG_ERR, - "Failed alloc on unshare path for foreign (%d) lookup\n", - d->domain_id); - return; - } + if ( (rc = __mem_event_claim_slot(d, + &d->mem_event->share, allow_sleep)) < 0 ) + return rc; - if (mem_event_claim_slot(d, &d->mem_event->share) < 0) + if ( v->domain == d ) { - return; + req.flags = MEM_EVENT_FLAG_VCPU_PAUSED; + vcpu_pause_nosync(v); } - req.flags = MEM_EVENT_FLAG_VCPU_PAUSED; - vcpu_pause_nosync(v); - req.p2mt = p2m_ram_shared; req.vcpu_id = v->vcpu_id; + mem_event_put_request(d, &d->mem_event->share, &req); + + return 0; } unsigned int mem_sharing_get_nr_saved_mfns(void) @@ -903,7 +898,21 @@ err_out: return ret; } -int mem_sharing_unshare_page(struct domain *d, + +/* A note on the rationale for unshare error handling: + * 1. Unshare can only fail with ENOMEM. Any other error conditions BUG_ON()'s + * 2. We notify a potential dom0 helper through a mem_event ring. But we + * allow the notification to not go to sleep. If the event ring is full + * of ENOMEM warnings, then it's on the ball. + * 3. We cannot go to sleep until the unshare is resolved, because we might + * be buried deep into locks (e.g. something -> copy_to_user -> __hvm_copy) + * 4. So, we make sure we: + * 4.1. return an error + * 4.2. do not corrupt shared memory + * 4.3. do not corrupt guest memory + * 4.4. let the guest deal with it if the error propagation will reach it + */ +int __mem_sharing_unshare_page(struct domain *d, unsigned long gfn, uint16_t flags) { @@ -945,7 +954,6 @@ gfn_found: /* Do the accounting first. If anything fails below, we have bigger * bigger fish to fry. First, remove the gfn from the list. */ last_gfn = list_has_one_entry(&page->sharing->gfns); - mem_sharing_gfn_destroy(d, gfn_info); if ( last_gfn ) { /* Clean up shared state */ @@ -959,6 +967,7 @@ gfn_found: * (possibly freeing the page), and exit early */ if ( flags & MEM_SHARING_DESTROY_GFN ) { + mem_sharing_gfn_destroy(d, gfn_info); put_page_and_type(page); mem_sharing_page_unlock(page); if ( last_gfn && @@ -971,6 +980,7 @@ gfn_found: if ( last_gfn ) { + mem_sharing_gfn_destroy(d, gfn_info); /* Making a page private atomically unlocks it */ BUG_ON(page_make_private(d, page) != 0); goto private_page_found; @@ -982,7 +992,8 @@ gfn_found: { mem_sharing_page_unlock(old_page); put_gfn(d, gfn); - mem_sharing_notify_helper(d, gfn); + /* Caller is responsible for placing an event + * in the ring */ return -ENOMEM; } @@ -993,6 +1004,7 @@ gfn_found: unmap_domain_page(t); BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page)) == 0); + mem_sharing_gfn_destroy(d, gfn_info); mem_sharing_page_unlock(old_page); put_page_and_type(old_page); diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index bb54969ee5..ba47c99445 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -170,7 +170,10 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn, if ( q == p2m_unshare && p2m_is_shared(*t) ) { ASSERT(!p2m_is_nestedp2m(p2m)); - mem_sharing_unshare_page(p2m->domain, gfn, 0); + /* Try to unshare. If we fail, communicate ENOMEM without + * sleeping. */ + if ( mem_sharing_unshare_page(p2m->domain, gfn, 0) < 0 ) + (void)mem_sharing_notify_enomem(p2m->domain, gfn, 0); mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order); } #endif @@ -371,6 +374,7 @@ void p2m_teardown(struct p2m_domain *p2m) if ( mfn_valid(mfn) && (t == p2m_ram_shared) ) { ASSERT(!p2m_is_nestedp2m(p2m)); + /* Does not fail with ENOMEM given the DESTROY flag */ BUG_ON(mem_sharing_unshare_page(d, gfn, MEM_SHARING_DESTROY_GFN)); } } @@ -510,6 +514,18 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn, if ( rc ) { p2m_unlock(p2m); + /* NOTE: Should a guest domain bring this upon itself, + * there is not a whole lot we can do. We are buried + * deep in locks from most code paths by now. So, fail + * the call and don't try to sleep on a wait queue + * while placing the mem event. + * + * However, all current (changeset 3432abcf9380) code + * paths avoid this unsavoury situation. For now. + * + * Foreign domains are okay to place an event as they + * won't go to sleep. */ + (void)mem_sharing_notify_enomem(p2m->domain, gfn + i, 0); return rc; } omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, p2m_query, NULL); diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 28a256197e..e6706df53b 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -112,8 +112,7 @@ static unsigned inline int max_nr_maptrack_frames(void) p2m_type_t __p2mt; \ unsigned long __x; \ __x = mfn_x(get_gfn_unshare((_d), (_gfn), &__p2mt)); \ - BUG_ON(p2m_is_shared(__p2mt)); /* XXX fixme */ \ - if ( !p2m_is_valid(__p2mt) ) \ + if ( p2m_is_shared(__p2mt) || !p2m_is_valid(__p2mt) ) \ __x = INVALID_MFN; \ __x; }) #else @@ -155,9 +154,11 @@ static int __get_paged_frame(unsigned long gfn, unsigned long *frame, int readon else { mfn = get_gfn_unshare(rd, gfn, &p2mt); - BUG_ON(p2m_is_shared(p2mt)); - /* XXX Here, and above in gfn_to_mfn_private, need to handle - * XXX failure to unshare. */ + if ( p2m_is_shared(p2mt) ) + { + put_gfn(rd, gfn); + return GNTST_eagain; + } } if ( p2m_is_valid(p2mt) ) { diff --git a/xen/common/memory.c b/xen/common/memory.c index e0976b1f08..907c6fdbde 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -200,6 +200,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) if ( mem_sharing_unshare_page(d, gmfn, 0) ) { put_gfn(d, gmfn); + (void)mem_sharing_notify_enomem(d, gmfn, 0); return 0; } /* Maybe the mfn changed */ diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h index 37e141c073..5c5d73fe78 100644 --- a/xen/include/asm-x86/mem_sharing.h +++ b/xen/include/asm-x86/mem_sharing.h @@ -52,10 +52,35 @@ int mem_sharing_nominate_page(struct domain *d, unsigned long gfn, int expected_refcnt, shr_handle_t *phandle); + #define MEM_SHARING_DESTROY_GFN (1<<1) -int mem_sharing_unshare_page(struct domain *d, +/* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */ +int __mem_sharing_unshare_page(struct domain *d, unsigned long gfn, uint16_t flags); +static inline int mem_sharing_unshare_page(struct domain *d, + unsigned long gfn, + uint16_t flags) +{ + int rc = __mem_sharing_unshare_page(d, gfn, flags); + BUG_ON( rc && (rc != -ENOMEM) ); + return rc; +} + +/* If called by a foreign domain, possible errors are + * -EBUSY -> ring full + * -ENOSYS -> no ring to begin with + * and the foreign mapper is responsible for retrying. + * + * If called by the guest vcpu itself and allow_sleep is set, may + * sleep on a wait queue, so the caller is responsible for not + * holding locks on entry. It may only fail with ENOSYS + * + * If called by the guest vcpu itself and allow_sleep is not set, + * then it's the same as a foreign domain. + */ +int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn, + bool_t allow_sleep); int mem_sharing_sharing_resume(struct domain *d); int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec); -- 2.30.2