From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] x86/p2m-pt: delay freeing of intermediate page tables Date: Tue, 29 Sep 2015 11:54:40 +0100 Message-ID: <560A6DF0.9040502@citrix.com> References: <560A896202000078000A68F4@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZgsYa-0004k7-6Z for xen-devel@lists.xenproject.org; Tue, 29 Sep 2015 10:54:48 +0000 In-Reply-To: <560A896202000078000A68F4@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: George Dunlap , Andrew Cooper , Wei Liu List-Id: xen-devel@lists.xenproject.org On 29/09/15 11:51, Jan Beulich wrote: > Old intermediate page tables must be freed only after IOMMU side > updates/flushes have got carried out. > > Signed-off-by: Jan Beulich Reviewed-by: George Dunlap > --- > This was split out from a previously submitted patch. > > --- a/xen/arch/x86/mm/p2m-pt.c > +++ b/xen/arch/x86/mm/p2m-pt.c > @@ -487,8 +487,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > /* XXX -- this might be able to be faster iff current->domain == d */ > void *table; > unsigned long i, gfn_remainder = gfn; > - l1_pgentry_t *p2m_entry; > - l1_pgentry_t entry_content; > + l1_pgentry_t *p2m_entry, entry_content; > + /* Intermediate table to free if we're replacing it with a superpage. */ > + l1_pgentry_t intermediate_entry = l1e_empty(); > l2_pgentry_t l2e_content; > l3_pgentry_t l3e_content; > int rc; > @@ -538,7 +539,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > */ > if ( page_order == PAGE_ORDER_1G ) > { > - l1_pgentry_t old_entry = l1e_empty(); > p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn, > L3_PAGETABLE_SHIFT - PAGE_SHIFT, > L3_PAGETABLE_ENTRIES); > @@ -548,7 +548,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > { > /* We're replacing a non-SP page with a superpage. Make sure to > * handle freeing the table properly. */ > - old_entry = *p2m_entry; > + intermediate_entry = *p2m_entry; > } > > ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct); > @@ -566,10 +566,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > > p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3); > /* NB: paging_write_p2m_entry() handles tlb flushes properly */ > - > - /* Free old intermediate tables if necessary */ > - if ( l1e_get_flags(old_entry) & _PAGE_PRESENT ) > - p2m_free_entry(p2m, &old_entry, page_order); > } > else > { > @@ -610,7 +606,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > } > else if ( page_order == PAGE_ORDER_2M ) > { > - l1_pgentry_t old_entry = l1e_empty(); > p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn, > L2_PAGETABLE_SHIFT - PAGE_SHIFT, > L2_PAGETABLE_ENTRIES); > @@ -622,7 +617,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > { > /* We're replacing a non-SP page with a superpage. Make sure to > * handle freeing the table properly. */ > - old_entry = *p2m_entry; > + intermediate_entry = *p2m_entry; > } > > ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct); > @@ -643,10 +638,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > > p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2); > /* NB: paging_write_p2m_entry() handles tlb flushes properly */ > - > - /* Free old intermediate tables if necessary */ > - if ( l1e_get_flags(old_entry) & _PAGE_PRESENT ) > - p2m_free_entry(p2m, &old_entry, page_order); > } > > /* Track the highest gfn for which we have ever had a valid mapping */ > @@ -674,6 +665,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > } > } > > + /* > + * Free old intermediate tables if necessary. This has to be the > + * last thing we do, after removal from the IOMMU tables, so as to > + * avoid a potential use-after-free. > + */ > + if ( l1e_get_flags(intermediate_entry) & _PAGE_PRESENT ) > + p2m_free_entry(p2m, &intermediate_entry, page_order); > + > out: > unmap_domain_page(table); > return rc; > > >