[PATCH v3 0/8] Simplify and fix fetch()

[PATCH v3 0/8] Simplify and fix fetch()

[Avi Kivity]

fetch() has become a monster, with a zillion continues, breaks, and gotos.
Simplify it before Xiao adds even more.

Also fix the gpte validation race.

Avi Kivity (8):
  KVM: MMU: Add link_shadow_page() helper
  KVM: MMU: Use __set_spte to link shadow pages
  KVM: MMU: Add drop_large_spte() helper
     v2: s/drop_spte_if_large/drop_large_spte/g
  KVM: MMU: Add validate_direct_spte() helper
  KVM: MMU: Add validate_indirect_spte() helper
     v2: add comments
         make validate_indirect_spte() look at spte in its own level, not one
         below (but adjust caller so no net effect)
  KVM: MMU: Simplify spte fetch() function
     v2: add comments
         update 'sptep' and 'level' for last level
  KVM: MMU: Validate all gptes during fetch, not just those used for
    new pages
     v3: fix top-level gpte validation oops
     v2: validate top-level gpte (in root page)
  KVM: MMU: Eliminate redundant temporaries in FNAME(fetch)
     v2: new patch

 arch/x86/kvm/mmu.c         |   41 ++++++++++++
 arch/x86/kvm/paging_tmpl.h |  157 +++++++++++++++++++++++---------------------
 2 files changed, 122 insertions(+), 76 deletions(-)

[PATCH v3 1/8] KVM: MMU: Add link_shadow_page() helper

[Avi Kivity]

To simplify the process of fetching an spte, add a helper that links
a shadow page to an spte.

Reviewed-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/mmu.c         |   10 ++++++++++
 arch/x86/kvm/paging_tmpl.h |    7 ++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index b93b94f..ae35853 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1482,6 +1482,16 @@ static void shadow_walk_next(struct kvm_shadow_walk_iterator *iterator)
 	--iterator->level;
 }
 
+static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
+{
+	u64 spte;
+
+	spte = __pa(sp->spt)
+		| PT_PRESENT_MASK | PT_ACCESSED_MASK
+		| PT_WRITABLE_MASK | PT_USER_MASK;
+	*sptep = spte;
+}
+
 static void kvm_mmu_page_unlink_children(struct kvm *kvm,
 					 struct kvm_mmu_page *sp)
 {
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 6daeacf..4606c88 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -309,7 +309,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 {
 	unsigned access = gw->pt_access;
 	struct kvm_mmu_page *sp;
-	u64 spte, *sptep = NULL;
+	u64 *sptep = NULL;
 	int direct;
 	gfn_t table_gfn;
 	int r;
@@ -394,10 +394,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			}
 		}
 
-		spte = __pa(sp->spt)
-			| PT_PRESENT_MASK | PT_ACCESSED_MASK
-			| PT_WRITABLE_MASK | PT_USER_MASK;
-		*sptep = spte;
+		link_shadow_page(sptep, sp);
 	}
 
 	return sptep;
-- 
1.7.1

[PATCH v3 2/8] KVM: MMU: Use __set_spte to link shadow pages

[Avi Kivity]

To avoid split accesses to 64 bit sptes on i386, use __set_spte() to link
shadow pages together.

(not technically required since shadow pages are __GFP_KERNEL, so upper 32
bits are always clear)

Reviewed-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/mmu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index ae35853..75cfb79 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1489,7 +1489,7 @@ static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
 	spte = __pa(sp->spt)
 		| PT_PRESENT_MASK | PT_ACCESSED_MASK
 		| PT_WRITABLE_MASK | PT_USER_MASK;
-	*sptep = spte;
+	__set_spte(sptep, spte);
 }
 
 static void kvm_mmu_page_unlink_children(struct kvm *kvm,
-- 
1.7.1

[PATCH v3 3/8] KVM: MMU: Add drop_large_spte() helper

[Avi Kivity]

To clarify spte fetching code, move large spte handling into a helper.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/mmu.c         |    8 ++++++++
 arch/x86/kvm/paging_tmpl.h |    5 +----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 75cfb79..747af72 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1492,6 +1492,14 @@ static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
 	__set_spte(sptep, spte);
 }
 
+static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+	if (is_large_pte(*sptep)) {
+		drop_spte(vcpu->kvm, sptep, shadow_trap_nonpresent_pte);
+		kvm_flush_remote_tlbs(vcpu->kvm);
+	}
+}
+
 static void kvm_mmu_page_unlink_children(struct kvm *kvm,
 					 struct kvm_mmu_page *sp)
 {
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 4606c88..69dcac0 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -360,10 +360,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			kvm_flush_remote_tlbs(vcpu->kvm);
 		}
 
-		if (is_large_pte(*sptep)) {
-			drop_spte(vcpu->kvm, sptep, shadow_trap_nonpresent_pte);
-			kvm_flush_remote_tlbs(vcpu->kvm);
-		}
+		drop_large_spte(vcpu, sptep);
 
 		if (level <= gw->level) {
 			direct = 1;
-- 
1.7.1

[PATCH v3 4/8] KVM: MMU: Add validate_direct_spte() helper

[Avi Kivity]

Add a helper to verify that a direct shadow page is valid wrt the required
access permissions; drop the page if it is not valid.

Reviewed-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/mmu.c         |   23 +++++++++++++++++++++++
 arch/x86/kvm/paging_tmpl.h |   27 ++++++---------------------
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 747af72..d16efbe 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1500,6 +1500,29 @@ static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
 	}
 }
 
+static void validate_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep,
+				   unsigned direct_access)
+{
+	if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep)) {
+		struct kvm_mmu_page *child;
+
+		/*
+		 * For the direct sp, if the guest pte's dirty bit
+		 * changed form clean to dirty, it will corrupt the
+		 * sp's access: allow writable in the read-only sp,
+		 * so we should update the spte at this point to get
+		 * a new sp with the correct access.
+		 */
+		child = page_header(*sptep & PT64_BASE_ADDR_MASK);
+		if (child->role.access == direct_access)
+			return;
+
+		mmu_page_remove_parent_pte(child, sptep);
+		__set_spte(sptep, shadow_trap_nonpresent_pte);
+		kvm_flush_remote_tlbs(vcpu->kvm);
+	}
+}
+
 static void kvm_mmu_page_unlink_children(struct kvm *kvm,
 					 struct kvm_mmu_page *sp)
 {
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 69dcac0..893a75c 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -338,30 +338,15 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			break;
 		}
 
-		if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep)) {
-			struct kvm_mmu_page *child;
-
-			if (level != gw->level)
-				continue;
-
-			/*
-			 * For the direct sp, if the guest pte's dirty bit
-			 * changed form clean to dirty, it will corrupt the
-			 * sp's access: allow writable in the read-only sp,
-			 * so we should update the spte at this point to get
-			 * a new sp with the correct access.
-			 */
-			child = page_header(*sptep & PT64_BASE_ADDR_MASK);
-			if (child->role.access == direct_access)
-				continue;
-
-			mmu_page_remove_parent_pte(child, sptep);
-			__set_spte(sptep, shadow_trap_nonpresent_pte);
-			kvm_flush_remote_tlbs(vcpu->kvm);
-		}
+		if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep)
+		    && level == gw->level)
+			validate_direct_spte(vcpu, sptep, direct_access);
 
 		drop_large_spte(vcpu, sptep);
 
+		if (is_shadow_present_pte(*sptep))
+			continue;
+
 		if (level <= gw->level) {
 			direct = 1;
 			access = direct_access;
-- 
1.7.1

[PATCH v3 5/8] KVM: MMU: Add validate_indirect_spte() helper

[Avi Kivity]

Move the code to validate an indirect shadow page (by verifying that the gpte
has not changed since it was fetched) into a helper.

Reviewed-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/paging_tmpl.h |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 893a75c..1ef4a6a 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -299,6 +299,23 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 		     gpte_to_gfn(gpte), pfn, true, true);
 }
 
+static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu,
+					  u64 *sptep, struct kvm_mmu_page *sp,
+					  struct guest_walker *gw, int level)
+{
+	int r;
+	pt_element_t curr_pte;
+
+	r = kvm_read_guest_atomic(vcpu->kvm,
+				  gw->pte_gpa[level - 1],
+				  &curr_pte, sizeof(curr_pte));
+	if (r || curr_pte != gw->ptes[level - 1]) {
+		kvm_mmu_put_page(sp, sptep);
+		return false;
+	}
+	return true;
+}
+
 /*
  * Fetch a shadow pte for a specific level in the paging hierarchy.
  */
@@ -312,11 +329,9 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	u64 *sptep = NULL;
 	int direct;
 	gfn_t table_gfn;
-	int r;
 	int level;
 	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
 	unsigned direct_access;
-	pt_element_t curr_pte;
 	struct kvm_shadow_walk_iterator iterator;
 
 	if (!is_present_gpte(gw->ptes[gw->level - 1]))
@@ -364,17 +379,17 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		}
 		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
 					       direct, access, sptep);
-		if (!direct) {
-			r = kvm_read_guest_atomic(vcpu->kvm,
-						  gw->pte_gpa[level - 2],
-						  &curr_pte, sizeof(curr_pte));
-			if (r || curr_pte != gw->ptes[level - 2]) {
-				kvm_mmu_put_page(sp, sptep);
+		if (!direct)
+			/*
+			 * Verify that the gpte in the page we've just write
+			 * protected is still there.
+			 */
+			if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
+							   gw, level - 1)) {
 				kvm_release_pfn_clean(pfn);
 				sptep = NULL;
 				break;
 			}
-		}
 
 		link_shadow_page(sptep, sp);
 	}
-- 
1.7.1

[PATCH v3 6/8] KVM: MMU: Simplify spte fetch() function

[Avi Kivity]

Partition the function into three sections:

- fetching indirect shadow pages (host_level > guest_level)
- fetching direct shadow pages (page_level < host_level <= guest_level)
- the final spte (page_level == host_level)

Instead of the current spaghetti.

A slight change from the original code is that we call validate_direct_spte()
more often: previously we called it only for gw->level, now we also call it for
lower levels.  The change should have no effect.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/paging_tmpl.h |   88 +++++++++++++++++++++++---------------------
 1 files changed, 46 insertions(+), 42 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1ef4a6a..441f51c 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -327,9 +327,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	unsigned access = gw->pt_access;
 	struct kvm_mmu_page *sp;
 	u64 *sptep = NULL;
-	int direct;
-	gfn_t table_gfn;
-	int level;
+	int uninitialized_var(level);
 	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
 	unsigned direct_access;
 	struct kvm_shadow_walk_iterator iterator;
@@ -341,59 +339,65 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	if (!dirty)
 		direct_access &= ~ACC_WRITE_MASK;
 
-	for_each_shadow_entry(vcpu, addr, iterator) {
+	for (shadow_walk_init(&iterator, vcpu, addr);
+	     shadow_walk_okay(&iterator) && iterator.level > gw->level;
+	     shadow_walk_next(&iterator)) {
+		gfn_t table_gfn;
+
 		level = iterator.level;
 		sptep = iterator.sptep;
-		if (iterator.level == hlevel) {
-			mmu_set_spte(vcpu, sptep, access,
-				     gw->pte_access & access,
-				     user_fault, write_fault,
-				     dirty, ptwrite, level,
-				     gw->gfn, pfn, false, true);
-			break;
+
+		drop_large_spte(vcpu, sptep);
+
+		if (is_shadow_present_pte(*sptep))
+			continue;
+
+		table_gfn = gw->table_gfn[level - 2];
+		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
+				      false, access, sptep);
+
+		/*
+		 * Verify that the gpte in the page we've just write
+		 * protected is still there.
+		 */
+		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
+						   gw, level - 1)) {
+			kvm_release_pfn_clean(pfn);
+			return NULL;
 		}
 
-		if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep)
-		    && level == gw->level)
-			validate_direct_spte(vcpu, sptep, direct_access);
+		link_shadow_page(sptep, sp);
+	}
+
+	for (;
+	     shadow_walk_okay(&iterator) && iterator.level > hlevel;
+	     shadow_walk_next(&iterator)) {
+		gfn_t direct_gfn;
+
+		level = iterator.level;
+		sptep = iterator.sptep;
 
 		drop_large_spte(vcpu, sptep);
 
 		if (is_shadow_present_pte(*sptep))
 			continue;
 
-		if (level <= gw->level) {
-			direct = 1;
-			access = direct_access;
-
-			/*
-			 * It is a large guest pages backed by small host pages,
-			 * So we set @direct(@sp->role.direct)=1, and set
-			 * @table_gfn(@sp->gfn)=the base page frame for linear
-			 * translations.
-			 */
-			table_gfn = gw->gfn & ~(KVM_PAGES_PER_HPAGE(level) - 1);
-		} else {
-			direct = 0;
-			table_gfn = gw->table_gfn[level - 2];
-		}
-		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
-					       direct, access, sptep);
-		if (!direct)
-			/*
-			 * Verify that the gpte in the page we've just write
-			 * protected is still there.
-			 */
-			if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
-							   gw, level - 1)) {
-				kvm_release_pfn_clean(pfn);
-				sptep = NULL;
-				break;
-			}
+		validate_direct_spte(vcpu, sptep, direct_access);
 
+		direct_gfn = gw->gfn & ~(KVM_PAGES_PER_HPAGE(level) - 1);
+
+		sp = kvm_mmu_get_page(vcpu, direct_gfn, addr, level-1,
+				      true, direct_access, sptep);
 		link_shadow_page(sptep, sp);
 	}
 
+	sptep = iterator.sptep;
+	level = iterator.level;
+
+	mmu_set_spte(vcpu, sptep, access, gw->pte_access & access,
+		     user_fault, write_fault, dirty, ptwrite, level,
+		     gw->gfn, pfn, false, true);
+
 	return sptep;
 }
 
-- 
1.7.1

[PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Avi Kivity]

Currently, when we fetch an spte, we only verify that gptes match those that
the walker saw if we build new shadow pages for them.

However, this misses the following race:

  vcpu1            vcpu2

  walk
                  change gpte
                  walk
                  instantiate sp

  fetch existing sp

Fix by validating every gpte, regardless of whether it is used for building
a new sp or not.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/paging_tmpl.h |   44 +++++++++++++++++++++++++++++++-------------
 1 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 441f51c..89b2dab 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -310,7 +310,8 @@ static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu,
 				  gw->pte_gpa[level - 1],
 				  &curr_pte, sizeof(curr_pte));
 	if (r || curr_pte != gw->ptes[level - 1]) {
-		kvm_mmu_put_page(sp, sptep);
+		if (sp)
+			kvm_mmu_put_page(sp, sptep);
 		return false;
 	}
 	return true;
@@ -325,10 +326,11 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 			 int *ptwrite, pfn_t pfn)
 {
 	unsigned access = gw->pt_access;
-	struct kvm_mmu_page *sp;
+	struct kvm_mmu_page *uninitialized_var(sp);
 	u64 *sptep = NULL;
 	int uninitialized_var(level);
 	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
+	int top_level;
 	unsigned direct_access;
 	struct kvm_shadow_walk_iterator iterator;
 
@@ -339,34 +341,46 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	if (!dirty)
 		direct_access &= ~ACC_WRITE_MASK;
 
+	top_level = vcpu->arch.mmu.root_level;
+	if (top_level == PT32E_ROOT_LEVEL)
+		top_level = PT32_ROOT_LEVEL;
+	/*
+	 * Verify that the top-level gpte is still there.  Since the page
+	 * is a root page, it is either write protected (and cannot be
+	 * changed from now on) or it is invalid (in which case, we don't
+	 * really care if it changes underneath us after this point).
+	 */
+	if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level))
+		goto out_error;
+
 	for (shadow_walk_init(&iterator, vcpu, addr);
 	     shadow_walk_okay(&iterator) && iterator.level > gw->level;
 	     shadow_walk_next(&iterator)) {
 		gfn_t table_gfn;
+		bool new_page = false;
 
 		level = iterator.level;
 		sptep = iterator.sptep;
 
 		drop_large_spte(vcpu, sptep);
 
-		if (is_shadow_present_pte(*sptep))
-			continue;
-
-		table_gfn = gw->table_gfn[level - 2];
-		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
-				      false, access, sptep);
+		if (!is_shadow_present_pte(*sptep)) {
+			table_gfn = gw->table_gfn[level - 2];
+			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
+					      false, access, sptep);
+			new_page = true;
+		}
 
 		/*
 		 * Verify that the gpte in the page we've just write
 		 * protected is still there.
 		 */
 		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
-						   gw, level - 1)) {
-			kvm_release_pfn_clean(pfn);
-			return NULL;
-		}
+						   gw, level - 1))
+			goto out_error;
 
-		link_shadow_page(sptep, sp);
+		if (new_page)
+			link_shadow_page(sptep, sp);
 	}
 
 	for (;
@@ -399,6 +413,10 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		     gw->gfn, pfn, false, true);
 
 	return sptep;
+
+out_error:
+	kvm_release_pfn_clean(pfn);
+	return NULL;
 }
 
 /*
-- 
1.7.1

Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Marcelo Tosatti]

On Mon, Jul 12, 2010 at 07:15:50PM +0300, Avi Kivity wrote:
> Currently, when we fetch an spte, we only verify that gptes match those that
> the walker saw if we build new shadow pages for them.
> 
> However, this misses the following race:
> 
>   vcpu1            vcpu2
> 
>   walk
>                   change gpte
>                   walk
>                   instantiate sp
> 
>   fetch existing sp
> 
> Fix by validating every gpte, regardless of whether it is used for building
> a new sp or not.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/paging_tmpl.h |   44 +++++++++++++++++++++++++++++++-------------
>  1 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 441f51c..89b2dab 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -310,7 +310,8 @@ static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu,
>  				  gw->pte_gpa[level - 1],
>  				  &curr_pte, sizeof(curr_pte));
>  	if (r || curr_pte != gw->ptes[level - 1]) {
> -		kvm_mmu_put_page(sp, sptep);
> +		if (sp)
> +			kvm_mmu_put_page(sp, sptep);
>  		return false;
>  	}
>  	return true;
> @@ -325,10 +326,11 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>  			 int *ptwrite, pfn_t pfn)
>  {
>  	unsigned access = gw->pt_access;
> -	struct kvm_mmu_page *sp;
> +	struct kvm_mmu_page *uninitialized_var(sp);
>  	u64 *sptep = NULL;
>  	int uninitialized_var(level);
>  	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
> +	int top_level;
>  	unsigned direct_access;
>  	struct kvm_shadow_walk_iterator iterator;
>  
> @@ -339,34 +341,46 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>  	if (!dirty)
>  		direct_access &= ~ACC_WRITE_MASK;
>  
> +	top_level = vcpu->arch.mmu.root_level;
> +	if (top_level == PT32E_ROOT_LEVEL)
> +		top_level = PT32_ROOT_LEVEL;
> +	/*
> +	 * Verify that the top-level gpte is still there.  Since the page
> +	 * is a root page, it is either write protected (and cannot be
> +	 * changed from now on) or it is invalid (in which case, we don't
> +	 * really care if it changes underneath us after this point).
> +	 */
> +	if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level))
> +		goto out_error;
> +
>  	for (shadow_walk_init(&iterator, vcpu, addr);
>  	     shadow_walk_okay(&iterator) && iterator.level > gw->level;
>  	     shadow_walk_next(&iterator)) {
>  		gfn_t table_gfn;
> +		bool new_page = false;
>  
>  		level = iterator.level;
>  		sptep = iterator.sptep;
>  
>  		drop_large_spte(vcpu, sptep);
>  
> -		if (is_shadow_present_pte(*sptep))
> -			continue;
> -
> -		table_gfn = gw->table_gfn[level - 2];
> -		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
> -				      false, access, sptep);
> +		if (!is_shadow_present_pte(*sptep)) {
> +			table_gfn = gw->table_gfn[level - 2];
> +			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
> +					      false, access, sptep);
> +			new_page = true;
> +		}
>  
>  		/*
>  		 * Verify that the gpte in the page we've just write
>  		 * protected is still there.
>  		 */
>  		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
> -						   gw, level - 1)) {
> -			kvm_release_pfn_clean(pfn);
> -			return NULL;
> -		}
> +						   gw, level - 1))
> +			goto out_error;

If its not a new page, and validation fails, can't "sp" point to
a shadow page previously instantiated in the loop?

Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Avi Kivity]

On 07/12/2010 10:16 PM, Marcelo Tosatti wrote:
> On Mon, Jul 12, 2010 at 07:15:50PM +0300, Avi Kivity wrote:
>    
>> Currently, when we fetch an spte, we only verify that gptes match those that
>> the walker saw if we build new shadow pages for them.
>>
>> However, this misses the following race:
>>
>>    vcpu1            vcpu2
>>
>>    walk
>>                    change gpte
>>                    walk
>>                    instantiate sp
>>
>>    fetch existing sp
>>
>> Fix by validating every gpte, regardless of whether it is used for building
>> a new sp or not.
>>
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>> ---
>>   arch/x86/kvm/paging_tmpl.h |   44 +++++++++++++++++++++++++++++++-------------
>>   1 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>> index 441f51c..89b2dab 100644
>> --- a/arch/x86/kvm/paging_tmpl.h
>> +++ b/arch/x86/kvm/paging_tmpl.h
>> @@ -310,7 +310,8 @@ static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu,
>>   				  gw->pte_gpa[level - 1],
>>   				&curr_pte, sizeof(curr_pte));
>>   	if (r || curr_pte != gw->ptes[level - 1]) {
>> -		kvm_mmu_put_page(sp, sptep);
>> +		if (sp)
>> +			kvm_mmu_put_page(sp, sptep);
>>   		return false;
>>   	}
>>   	return true;
>> @@ -325,10 +326,11 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>>   			 int *ptwrite, pfn_t pfn)
>>   {
>>   	unsigned access = gw->pt_access;
>> -	struct kvm_mmu_page *sp;
>> +	struct kvm_mmu_page *uninitialized_var(sp);
>>   	u64 *sptep = NULL;
>>   	int uninitialized_var(level);
>>   	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
>> +	int top_level;
>>   	unsigned direct_access;
>>   	struct kvm_shadow_walk_iterator iterator;
>>
>> @@ -339,34 +341,46 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>>   	if (!dirty)
>>   		direct_access&= ~ACC_WRITE_MASK;
>>
>> +	top_level = vcpu->arch.mmu.root_level;
>> +	if (top_level == PT32E_ROOT_LEVEL)
>> +		top_level = PT32_ROOT_LEVEL;
>> +	/*
>> +	 * Verify that the top-level gpte is still there.  Since the page
>> +	 * is a root page, it is either write protected (and cannot be
>> +	 * changed from now on) or it is invalid (in which case, we don't
>> +	 * really care if it changes underneath us after this point).
>> +	 */
>> +	if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level))
>> +		goto out_error;
>> +
>>   	for (shadow_walk_init(&iterator, vcpu, addr);
>>   	     shadow_walk_okay(&iterator)&&  iterator.level>  gw->level;
>>   	     shadow_walk_next(&iterator)) {
>>   		gfn_t table_gfn;
>> +		bool new_page = false;
>>
>>   		level = iterator.level;
>>   		sptep = iterator.sptep;
>>
>>   		drop_large_spte(vcpu, sptep);
>>
>> -		if (is_shadow_present_pte(*sptep))
>> -			continue;
>> -
>> -		table_gfn = gw->table_gfn[level - 2];
>> -		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
>> -				      false, access, sptep);
>> +		if (!is_shadow_present_pte(*sptep)) {
>> +			table_gfn = gw->table_gfn[level - 2];
>> +			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
>> +					      false, access, sptep);
>> +			new_page = true;
>> +		}
>>
>>   		/*
>>   		 * Verify that the gpte in the page we've just write
>>   		 * protected is still there.
>>   		 */
>>   		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
>> -						   gw, level - 1)) {
>> -			kvm_release_pfn_clean(pfn);
>> -			return NULL;
>> -		}
>> +						   gw, level - 1))
>> +			goto out_error;
>>      
> If its not a new page, and validation fails, can't "sp" point to
> a shadow page previously instantiated in the loop?
>    

It can, and then sp and sptep are inconsistent.  Good catch.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Xiao Guangrong]

Avi Kivity wrote:
> Currently, when we fetch an spte, we only verify that gptes match those that
> the walker saw if we build new shadow pages for them.
> 
> However, this misses the following race:
> 
>   vcpu1            vcpu2
> 
>   walk
>                   change gpte
>                   walk
>                   instantiate sp
> 
>   fetch existing sp
> 
> Fix by validating every gpte, regardless of whether it is used for building
> a new sp or not.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/paging_tmpl.h |   44 +++++++++++++++++++++++++++++++-------------
>  1 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 441f51c..89b2dab 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -310,7 +310,8 @@ static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu,
>  				  gw->pte_gpa[level - 1],
>  				  &curr_pte, sizeof(curr_pte));
>  	if (r || curr_pte != gw->ptes[level - 1]) {
> -		kvm_mmu_put_page(sp, sptep);
> +		if (sp)
> +			kvm_mmu_put_page(sp, sptep);
>  		return false;
>  	}
>  	return true;
> @@ -325,10 +326,11 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>  			 int *ptwrite, pfn_t pfn)
>  {
>  	unsigned access = gw->pt_access;
> -	struct kvm_mmu_page *sp;
> +	struct kvm_mmu_page *uninitialized_var(sp);
>  	u64 *sptep = NULL;
>  	int uninitialized_var(level);
>  	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
> +	int top_level;
>  	unsigned direct_access;
>  	struct kvm_shadow_walk_iterator iterator;
>  
> @@ -339,34 +341,46 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>  	if (!dirty)
>  		direct_access &= ~ACC_WRITE_MASK;
>  
> +	top_level = vcpu->arch.mmu.root_level;
> +	if (top_level == PT32E_ROOT_LEVEL)
> +		top_level = PT32_ROOT_LEVEL;
> +	/*
> +	 * Verify that the top-level gpte is still there.  Since the page
> +	 * is a root page, it is either write protected (and cannot be
> +	 * changed from now on) or it is invalid (in which case, we don't
> +	 * really care if it changes underneath us after this point).
> +	 */
> +	if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level))
> +		goto out_error;
> +
>  	for (shadow_walk_init(&iterator, vcpu, addr);
>  	     shadow_walk_okay(&iterator) && iterator.level > gw->level;
>  	     shadow_walk_next(&iterator)) {
>  		gfn_t table_gfn;
> +		bool new_page = false;
>  
>  		level = iterator.level;
>  		sptep = iterator.sptep;
>  
>  		drop_large_spte(vcpu, sptep);
>  
> -		if (is_shadow_present_pte(*sptep))
> -			continue;
> -
> -		table_gfn = gw->table_gfn[level - 2];
> -		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
> -				      false, access, sptep);
> +		if (!is_shadow_present_pte(*sptep)) {
> +			table_gfn = gw->table_gfn[level - 2];
> +			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
> +					      false, access, sptep);
> +			new_page = true;
> +		}
>  
>  		/*
>  		 * Verify that the gpte in the page we've just write
>  		 * protected is still there.
>  		 */
>  		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
> -						   gw, level - 1)) {
> -			kvm_release_pfn_clean(pfn);
> -			return NULL;
> -		}
> +						   gw, level - 1))
> +			goto out_error;
>  

It missed the last mapping check? i only see validate_indirect_spte in
'level > gw->level' loop.

Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Avi Kivity]

On 07/13/2010 04:51 AM, Xiao Guangrong wrote:
>
> Avi Kivity wrote:
>    
>> Currently, when we fetch an spte, we only verify that gptes match those that
>> the walker saw if we build new shadow pages for them.
>>
>> However, this misses the following race:
>>
>>    vcpu1            vcpu2
>>
>>    walk
>>                    change gpte
>>                    walk
>>                    instantiate sp
>>
>>    fetch existing sp
>>
>> Fix by validating every gpte, regardless of whether it is used for building
>> a new sp or not.
>>
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>> ---
>>   arch/x86/kvm/paging_tmpl.h |   44 +++++++++++++++++++++++++++++++-------------
>>   1 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>> index 441f51c..89b2dab 100644
>> --- a/arch/x86/kvm/paging_tmpl.h
>> +++ b/arch/x86/kvm/paging_tmpl.h
>> @@ -310,7 +310,8 @@ static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu,
>>   				  gw->pte_gpa[level - 1],
>>   				&curr_pte, sizeof(curr_pte));
>>   	if (r || curr_pte != gw->ptes[level - 1]) {
>> -		kvm_mmu_put_page(sp, sptep);
>> +		if (sp)
>> +			kvm_mmu_put_page(sp, sptep);
>>   		return false;
>>   	}
>>   	return true;
>> @@ -325,10 +326,11 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>>   			 int *ptwrite, pfn_t pfn)
>>   {
>>   	unsigned access = gw->pt_access;
>> -	struct kvm_mmu_page *sp;
>> +	struct kvm_mmu_page *uninitialized_var(sp);
>>   	u64 *sptep = NULL;
>>   	int uninitialized_var(level);
>>   	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
>> +	int top_level;
>>   	unsigned direct_access;
>>   	struct kvm_shadow_walk_iterator iterator;
>>
>> @@ -339,34 +341,46 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>>   	if (!dirty)
>>   		direct_access&= ~ACC_WRITE_MASK;
>>
>> +	top_level = vcpu->arch.mmu.root_level;
>> +	if (top_level == PT32E_ROOT_LEVEL)
>> +		top_level = PT32_ROOT_LEVEL;
>> +	/*
>> +	 * Verify that the top-level gpte is still there.  Since the page
>> +	 * is a root page, it is either write protected (and cannot be
>> +	 * changed from now on) or it is invalid (in which case, we don't
>> +	 * really care if it changes underneath us after this point).
>> +	 */
>> +	if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level))
>> +		goto out_error;
>> +
>>   	for (shadow_walk_init(&iterator, vcpu, addr);
>>   	     shadow_walk_okay(&iterator)&&  iterator.level>  gw->level;
>>   	     shadow_walk_next(&iterator)) {
>>   		gfn_t table_gfn;
>> +		bool new_page = false;
>>
>>   		level = iterator.level;
>>   		sptep = iterator.sptep;
>>
>>   		drop_large_spte(vcpu, sptep);
>>
>> -		if (is_shadow_present_pte(*sptep))
>> -			continue;
>> -
>> -		table_gfn = gw->table_gfn[level - 2];
>> -		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
>> -				      false, access, sptep);
>> +		if (!is_shadow_present_pte(*sptep)) {
>> +			table_gfn = gw->table_gfn[level - 2];
>> +			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
>> +					      false, access, sptep);
>> +			new_page = true;
>> +		}
>>
>>   		/*
>>   		 * Verify that the gpte in the page we've just write
>>   		 * protected is still there.
>>   		 */
>>   		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
>> -						   gw, level - 1)) {
>> -			kvm_release_pfn_clean(pfn);
>> -			return NULL;
>> -		}
>> +						   gw, level - 1))
>> +			goto out_error;
>>
>>      
> It missed the last mapping check? i only see validate_indirect_spte in
> 'level>  gw->level' loop.
>    

But we check 'level - 1' here, so the final level is included.  It is 
the top level that is not checked in the loop (we check it separately, 
above).


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Xiao Guangrong]

Avi Kivity wrote:

>>>           /*
>>>            * Verify that the gpte in the page we've just write
>>>            * protected is still there.
>>>            */
>>>           if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
>>> -                           gw, level - 1)) {
>>> -            kvm_release_pfn_clean(pfn);
>>> -            return NULL;
>>> -        }
>>> +                           gw, level - 1))
>>> +            goto out_error;
>>>
>>>      
>> It missed the last mapping check? i only see validate_indirect_spte in
>> 'level>  gw->level' loop.
>>    
> 
> But we check 'level - 1' here, so the final level is included.  It is
> the top level that is not checked in the loop (we check it separately,
> above).
> 

Yeah, you are right!

[PATCH v3 8/8] KVM: MMU: Eliminate redundant temporaries in FNAME(fetch)

[Avi Kivity]

'level' and 'sptep' are aliases for 'interator.level' and 'iterator.sptep', no
need for them.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/paging_tmpl.h |   59 ++++++++++++++++++--------------------------
 1 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 89b2dab..66796f8 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -327,12 +327,10 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 {
 	unsigned access = gw->pt_access;
 	struct kvm_mmu_page *uninitialized_var(sp);
-	u64 *sptep = NULL;
-	int uninitialized_var(level);
 	bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]);
 	int top_level;
 	unsigned direct_access;
-	struct kvm_shadow_walk_iterator iterator;
+	struct kvm_shadow_walk_iterator it;
 
 	if (!is_present_gpte(gw->ptes[gw->level - 1]))
 		return NULL;
@@ -353,21 +351,18 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 	if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level))
 		goto out_error;
 
-	for (shadow_walk_init(&iterator, vcpu, addr);
-	     shadow_walk_okay(&iterator) && iterator.level > gw->level;
-	     shadow_walk_next(&iterator)) {
+	for (shadow_walk_init(&it, vcpu, addr);
+	     shadow_walk_okay(&it) && it.level > gw->level;
+	     shadow_walk_next(&it)) {
 		gfn_t table_gfn;
 		bool new_page = false;
 
-		level = iterator.level;
-		sptep = iterator.sptep;
-
-		drop_large_spte(vcpu, sptep);
+		drop_large_spte(vcpu, it.sptep);
 
-		if (!is_shadow_present_pte(*sptep)) {
-			table_gfn = gw->table_gfn[level - 2];
-			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
-					      false, access, sptep);
+		if (!is_shadow_present_pte(*it.sptep)) {
+			table_gfn = gw->table_gfn[it.level - 2];
+			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, it.level-1,
+					      false, access, it.sptep);
 			new_page = true;
 		}
 
@@ -375,44 +370,38 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		 * Verify that the gpte in the page we've just write
 		 * protected is still there.
 		 */
-		if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp,
-						   gw, level - 1))
+		if (!FNAME(validate_indirect_spte)(vcpu, it.sptep, sp,
+						   gw, it.level - 1))
 			goto out_error;
 
 		if (new_page)
-			link_shadow_page(sptep, sp);
+			link_shadow_page(it.sptep, sp);
 	}
 
 	for (;
-	     shadow_walk_okay(&iterator) && iterator.level > hlevel;
-	     shadow_walk_next(&iterator)) {
+	     shadow_walk_okay(&it) && it.level > hlevel;
+	     shadow_walk_next(&it)) {
 		gfn_t direct_gfn;
 
-		level = iterator.level;
-		sptep = iterator.sptep;
+		drop_large_spte(vcpu, it.sptep);
 
-		drop_large_spte(vcpu, sptep);
-
-		if (is_shadow_present_pte(*sptep))
+		if (is_shadow_present_pte(*it.sptep))
 			continue;
 
-		validate_direct_spte(vcpu, sptep, direct_access);
+		validate_direct_spte(vcpu, it.sptep, direct_access);
 
-		direct_gfn = gw->gfn & ~(KVM_PAGES_PER_HPAGE(level) - 1);
+		direct_gfn = gw->gfn & ~(KVM_PAGES_PER_HPAGE(it.level) - 1);
 
-		sp = kvm_mmu_get_page(vcpu, direct_gfn, addr, level-1,
-				      true, direct_access, sptep);
-		link_shadow_page(sptep, sp);
+		sp = kvm_mmu_get_page(vcpu, direct_gfn, addr, it.level-1,
+				      true, direct_access, it.sptep);
+		link_shadow_page(it.sptep, sp);
 	}
 
-	sptep = iterator.sptep;
-	level = iterator.level;
-
-	mmu_set_spte(vcpu, sptep, access, gw->pte_access & access,
-		     user_fault, write_fault, dirty, ptwrite, level,
+	mmu_set_spte(vcpu, it.sptep, access, gw->pte_access & access,
+		     user_fault, write_fault, dirty, ptwrite, it.level,
 		     gw->gfn, pfn, false, true);
 
-	return sptep;
+	return it.sptep;
 
 out_error:
 	kvm_release_pfn_clean(pfn);
-- 
1.7.1