From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] NPT: temporarily retain page table mapping in do_recalc() Date: Tue, 6 May 2014 10:54:26 +0100 Message-ID: <5368B152.4080303@citrix.com> References: <536784BB020000780000EF15@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0250388649335496571==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Whc55-0006LA-GU for xen-devel@lists.xenproject.org; Tue, 06 May 2014 09:54:35 +0000 In-Reply-To: <536784BB020000780000EF15@mail.emea.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 Cc: xen-devel , Tim Deegan List-Id: xen-devel@lists.xenproject.org --===============0250388649335496571== Content-Type: multipart/alternative; boundary="------------040506070607080504090406" --------------040506070607080504090406 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 05/05/14 11:31, Jan Beulich wrote: > Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types > on a range") neglected the fact that p2m_next_level() replaces the > previous level's mapping with the new level's one, hence dereferencing > a stale pointer the translation for which may no longer be available > (timing dependent). Add a parameter to that function allowing the > caller to request that the mapping be retained (the unmapping will be > taken care of by the caller then). > > Signed-off-by: Jan Beulich Given the new API for p2m_next_level(), it is important to note that the mapping can only be safely left in place on the success path. In the case of an error the page must be unmapped even if 'unmap' is false. While the current behaviour is safe, all it would take is an innocent refactoring of the error paths to invalidate this requirement. Is it worth leaving a comment to that effect? Functionally however, Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/mm/p2m-pt.c > +++ b/xen/arch/x86/mm/p2m-pt.c > @@ -172,7 +172,7 @@ static void p2m_add_iommu_flags(l1_pgent > static int > p2m_next_level(struct p2m_domain *p2m, void **table, > unsigned long *gfn_remainder, unsigned long gfn, u32 shift, > - u32 max, unsigned long type) > + u32 max, unsigned long type, bool_t unmap) > { > l1_pgentry_t *l1_entry; > l1_pgentry_t *p2m_entry; > @@ -280,7 +280,8 @@ p2m_next_level(struct p2m_domain *p2m, v > } > > next = map_domain_page(l1e_get_pfn(*p2m_entry)); > - unmap_domain_page(*table); > + if ( unmap ) > + unmap_domain_page(*table); > *table = next; > > return 0; > @@ -319,7 +320,7 @@ static int p2m_pt_set_recalc_range(struc > > err = p2m_next_level(p2m, &table, &gfn_remainder, first_gfn, > i * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER, > - pgt[i - 1]); > + pgt[i - 1], 1); > if ( err ) > goto out; > } > @@ -386,7 +387,7 @@ static int do_recalc(struct p2m_domain * > > err = p2m_next_level(p2m, &table, &gfn_remainder, gfn, > level * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER, > - pgt[level - 1]); > + pgt[level - 1], 0); > if ( err ) > goto out; > > @@ -416,6 +417,7 @@ static int do_recalc(struct p2m_domain * > clear_recalc(l1, e); > p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1); > } > + unmap_domain_page((void *)((unsigned long)pent & PAGE_MASK)); > } > > pent = p2m_find_entry(table, &gfn_remainder, gfn, > @@ -519,7 +521,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > table = map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m)))); > rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn, > L4_PAGETABLE_SHIFT - PAGE_SHIFT, > - L4_PAGETABLE_ENTRIES, PGT_l3_page_table); > + L4_PAGETABLE_ENTRIES, PGT_l3_page_table, 1); > if ( rc ) > goto out; > > @@ -565,7 +567,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > { > rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn, > L3_PAGETABLE_SHIFT - PAGE_SHIFT, > - L3_PAGETABLE_ENTRIES, PGT_l2_page_table); > + L3_PAGETABLE_ENTRIES, PGT_l2_page_table, 1); > if ( rc ) > goto out; > } > @@ -574,7 +576,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, > { > rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn, > L2_PAGETABLE_SHIFT - PAGE_SHIFT, > - L2_PAGETABLE_ENTRIES, PGT_l1_page_table); > + L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1); > if ( rc ) > goto out; > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------040506070607080504090406 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 05/05/14 11:31, Jan Beulich wrote:
Commit b3e024f3 ("x86/NPT: don't walk page tables when changing types
on a range") neglected the fact that p2m_next_level() replaces the
previous level's mapping with the new level's one, hence dereferencing
a stale pointer the translation for which may no longer be available
(timing dependent). Add a parameter to that function allowing the
caller to request that the mapping be retained (the unmapping will be
taken care of by the caller then).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Given the new API for p2m_next_level(), it is important to note that the mapping can only be safely left in place on the success path.  In the case of an error the page must be unmapped even if 'unmap' is false.

While the current behaviour is safe, all it would take is an innocent refactoring of the error paths to invalidate this requirement.

Is it worth leaving a comment to that effect?

Functionally however, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -172,7 +172,7 @@ static void p2m_add_iommu_flags(l1_pgent
 static int
 p2m_next_level(struct p2m_domain *p2m, void **table,
                unsigned long *gfn_remainder, unsigned long gfn, u32 shift,
-               u32 max, unsigned long type)
+               u32 max, unsigned long type, bool_t unmap)
 {
     l1_pgentry_t *l1_entry;
     l1_pgentry_t *p2m_entry;
@@ -280,7 +280,8 @@ p2m_next_level(struct p2m_domain *p2m, v
     }
 
     next = map_domain_page(l1e_get_pfn(*p2m_entry));
-    unmap_domain_page(*table);
+    if ( unmap )
+        unmap_domain_page(*table);
     *table = next;
 
     return 0;
@@ -319,7 +320,7 @@ static int p2m_pt_set_recalc_range(struc
 
         err = p2m_next_level(p2m, &table, &gfn_remainder, first_gfn,
                              i * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
-                             pgt[i - 1]);
+                             pgt[i - 1], 1);
         if ( err )
             goto out;
     }
@@ -386,7 +387,7 @@ static int do_recalc(struct p2m_domain *
 
         err = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                              level * PAGETABLE_ORDER, 1 << PAGETABLE_ORDER,
-                             pgt[level - 1]);
+                             pgt[level - 1], 0);
         if ( err )
             goto out;
 
@@ -416,6 +417,7 @@ static int do_recalc(struct p2m_domain *
             clear_recalc(l1, e);
             p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
         }
+        unmap_domain_page((void *)((unsigned long)pent & PAGE_MASK));
     }
 
     pent = p2m_find_entry(table, &gfn_remainder, gfn,
@@ -519,7 +521,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     table = map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m))));
     rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                         L4_PAGETABLE_SHIFT - PAGE_SHIFT,
-                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table);
+                        L4_PAGETABLE_ENTRIES, PGT_l3_page_table, 1);
     if ( rc )
         goto out;
 
@@ -565,7 +567,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     {
         rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                             L3_PAGETABLE_SHIFT - PAGE_SHIFT,
-                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table);
+                            L3_PAGETABLE_ENTRIES, PGT_l2_page_table, 1);
         if ( rc )
             goto out;
     }
@@ -574,7 +576,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     {
         rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                             L2_PAGETABLE_SHIFT - PAGE_SHIFT,
-                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table);
+                            L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1);
         if ( rc )
             goto out;
 






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------040506070607080504090406-- --===============0250388649335496571== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============0250388649335496571==--