[PATCH 0/4] KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled

[PATCH 0/4] KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled

[David Matlack]

Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
that causes writes made by L2 to no be reflected in the dirty log when
L1 has disabled EPT.

Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
bug.

David Matlack (4):
  KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing
    TDP MMU dirty bits
  KVM: x86/mmu: Remove function comments above
    clear_dirty_{gfn_range,pt_masked}()
  KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs.
    write-protecting
  KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test

 arch/x86/kvm/mmu/tdp_mmu.c                    | 51 +++++++---------
 .../selftests/kvm/x86_64/vmx_dirty_log_test.c | 60 ++++++++++++++-----
 2 files changed, 68 insertions(+), 43 deletions(-)


base-commit: 687d8f4c3dea0758afd748968d91288220bbe7e3
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits

[David Matlack]

Check kvm_mmu_page_ad_need_write_protect() when deciding whether to
write-protect or clear D-bits on TDP MMU SPTEs.

TDP MMU SPTEs must be write-protected when the TDP MMU is being used to
run an L2 (i.e. L1 has disabled EPT) and PML is enabled. KVM always
disables the PML hardware when running L2, so failing to write-protect
TDP MMU SPTEs will cause writes made by L2 to not be reflected in the
dirty log.

Reported-by: syzbot+900d58a45dcaab9e4821@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=900d58a45dcaab9e4821
Fixes: 5982a5392663 ("KVM: x86/mmu: Use kvm_ad_enabled() to determine if TDP MMU SPTEs need wrprot")
Cc: stable@vger.kernel.org
Cc: Vipin Sharma <vipinsh@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 6ae19b4ee5b1..c3c1a8f430ef 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1498,6 +1498,16 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm,
 	}
 }
 
+static bool tdp_mmu_need_write_protect(struct kvm_mmu_page *sp)
+{
+	/*
+	 * All TDP MMU shadow pages share the same role as their root, aside
+	 * from level, so it is valid to key off any shadow page to determine if
+	 * write protection is needed for an entire tree.
+	 */
+	return kvm_mmu_page_ad_need_write_protect(sp) || !kvm_ad_enabled();
+}
+
 /*
  * Clear the dirty status of all the SPTEs mapping GFNs in the memslot. If
  * AD bits are enabled, this will involve clearing the dirty bit on each SPTE.
@@ -1508,7 +1518,8 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm,
 static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
 			   gfn_t start, gfn_t end)
 {
-	u64 dbit = kvm_ad_enabled() ? shadow_dirty_mask : PT_WRITABLE_MASK;
+	const u64 dbit = tdp_mmu_need_write_protect(root)
+		? PT_WRITABLE_MASK : shadow_dirty_mask;
 	struct tdp_iter iter;
 	bool spte_set = false;
 
@@ -1523,7 +1534,7 @@ static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
 		if (tdp_mmu_iter_cond_resched(kvm, &iter, false, true))
 			continue;
 
-		KVM_MMU_WARN_ON(kvm_ad_enabled() &&
+		KVM_MMU_WARN_ON(dbit == shadow_dirty_mask &&
 				spte_ad_need_write_protect(iter.old_spte));
 
 		if (!(iter.old_spte & dbit))
@@ -1570,8 +1581,8 @@ bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm,
 static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
 				  gfn_t gfn, unsigned long mask, bool wrprot)
 {
-	u64 dbit = (wrprot || !kvm_ad_enabled()) ? PT_WRITABLE_MASK :
-						   shadow_dirty_mask;
+	const u64 dbit = (wrprot || tdp_mmu_need_write_protect(root))
+		? PT_WRITABLE_MASK : shadow_dirty_mask;
 	struct tdp_iter iter;
 
 	lockdep_assert_held_write(&kvm->mmu_lock);
@@ -1583,7 +1594,7 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
 		if (!mask)
 			break;
 
-		KVM_MMU_WARN_ON(kvm_ad_enabled() &&
+		KVM_MMU_WARN_ON(dbit == shadow_dirty_mask &&
 				spte_ad_need_write_protect(iter.old_spte));
 
 		if (iter.level > PG_LEVEL_4K ||
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()

[David Matlack]

Drop the comments above clear_dirty_gfn_range() and
clear_dirty_pt_masked(), since each is word-for-word identical to the
comment above their parent function.

Leave the comment on the parent functions since they are APIs called by
the KVM/x86 MMU.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index c3c1a8f430ef..01192ac760f1 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1508,13 +1508,6 @@ static bool tdp_mmu_need_write_protect(struct kvm_mmu_page *sp)
 	return kvm_mmu_page_ad_need_write_protect(sp) || !kvm_ad_enabled();
 }
 
-/*
- * Clear the dirty status of all the SPTEs mapping GFNs in the memslot. If
- * AD bits are enabled, this will involve clearing the dirty bit on each SPTE.
- * If AD bits are not enabled, this will require clearing the writable bit on
- * each SPTE. Returns true if an SPTE has been changed and the TLBs need to
- * be flushed.
- */
 static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
 			   gfn_t start, gfn_t end)
 {
@@ -1571,13 +1564,6 @@ bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm,
 	return spte_set;
 }
 
-/*
- * Clears the dirty status of all the 4k SPTEs mapping GFNs for which a bit is
- * set in mask, starting at gfn. The given memslot is expected to contain all
- * the GFNs represented by set bits in the mask. If AD bits are enabled,
- * clearing the dirty status will involve clearing the dirty bit on each SPTE
- * or, if AD bits are not enabled, clearing the writable bit on each SPTE.
- */
 static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
 				  gfn_t gfn, unsigned long mask, bool wrprot)
 {
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting

[David Matlack]

Drop the "If AD bits are enabled/disabled" verbiage from the comments
above kvm_tdp_mmu_clear_dirty_{slot,pt_masked}() since TDP MMU SPTEs may
need to be write-protected even when A/D bits are enabled. i.e. These
comments aren't technically correct.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 01192ac760f1..1e9b48b5f6e1 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1544,11 +1544,9 @@ static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
 }
 
 /*
- * Clear the dirty status of all the SPTEs mapping GFNs in the memslot. If
- * AD bits are enabled, this will involve clearing the dirty bit on each SPTE.
- * If AD bits are not enabled, this will require clearing the writable bit on
- * each SPTE. Returns true if an SPTE has been changed and the TLBs need to
- * be flushed.
+ * Clear the dirty status (D-bit or W-bit) of all the SPTEs mapping GFNs in the
+ * memslot. Returns true if an SPTE has been changed and the TLBs need to be
+ * flushed.
  */
 bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm,
 				  const struct kvm_memory_slot *slot)
@@ -1606,11 +1604,9 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
 }
 
 /*
- * Clears the dirty status of all the 4k SPTEs mapping GFNs for which a bit is
- * set in mask, starting at gfn. The given memslot is expected to contain all
- * the GFNs represented by set bits in the mask. If AD bits are enabled,
- * clearing the dirty status will involve clearing the dirty bit on each SPTE
- * or, if AD bits are not enabled, clearing the writable bit on each SPTE.
+ * Clears the dirty status (D-bit or W-bit) of all the 4k SPTEs mapping GFNs for
+ * which a bit is set in mask, starting at gfn. The given memslot is expected to
+ * contain all the GFNs represented by set bits in the mask.
  */
 void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm,
 				       struct kvm_memory_slot *slot,
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test

[David Matlack]

Extend vmx_dirty_log_test to include accesses made by L2 when EPT is
disabled.

This commit adds explicit coverage of a bug caught by syzkaller, where
the TDP MMU would clear D-bits instead of write-protecting SPTEs being
used to map an L2, which only happens when L1 does not enable EPT,
causing writes made by L2 to not be reflected in the dirty log when PML
is enabled:

  $ ./vmx_dirty_log_test
  Nested EPT: disabled
  ==== Test Assertion Failure ====
    x86_64/vmx_dirty_log_test.c:151: test_bit(0, bmap)
    pid=72052 tid=72052 errno=4 - Interrupted system call
    (stack trace empty)
    Page 0 incorrectly reported clean

Opportunistically replace the volatile casts with {READ,WRITE}_ONCE().

Link: https://lore.kernel.org/kvm/000000000000c6526f06137f18cc@google.com/
Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/x86_64/vmx_dirty_log_test.c | 60 ++++++++++++++-----
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c b/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
index e4ad5fef52ff..609a767c4655 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
@@ -28,16 +28,16 @@
 #define NESTED_TEST_MEM1		0xc0001000
 #define NESTED_TEST_MEM2		0xc0002000
 
-static void l2_guest_code(void)
+static void l2_guest_code(u64 *a, u64 *b)
 {
-	*(volatile uint64_t *)NESTED_TEST_MEM1;
-	*(volatile uint64_t *)NESTED_TEST_MEM1 = 1;
+	READ_ONCE(*a);
+	WRITE_ONCE(*a, 1);
 	GUEST_SYNC(true);
 	GUEST_SYNC(false);
 
-	*(volatile uint64_t *)NESTED_TEST_MEM2 = 1;
+	READ_ONCE(*b);
 	GUEST_SYNC(true);
-	*(volatile uint64_t *)NESTED_TEST_MEM2 = 1;
+	WRITE_ONCE(*b, 1);
 	GUEST_SYNC(true);
 	GUEST_SYNC(false);
 
@@ -45,17 +45,33 @@ static void l2_guest_code(void)
 	vmcall();
 }
 
+static void l2_guest_code_ept_enabled(void)
+{
+	l2_guest_code((u64 *)NESTED_TEST_MEM1, (u64 *)NESTED_TEST_MEM2);
+}
+
+static void l2_guest_code_ept_disabled(void)
+{
+	/* Access the same L1 GPAs as l2_guest_code_ept_enabled() */
+	l2_guest_code((u64 *)GUEST_TEST_MEM, (u64 *)GUEST_TEST_MEM);
+}
+
 void l1_guest_code(struct vmx_pages *vmx)
 {
 #define L2_GUEST_STACK_SIZE 64
 	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
+	void *l2_rip;
 
 	GUEST_ASSERT(vmx->vmcs_gpa);
 	GUEST_ASSERT(prepare_for_vmx_operation(vmx));
 	GUEST_ASSERT(load_vmcs(vmx));
 
-	prepare_vmcs(vmx, l2_guest_code,
-		     &l2_guest_stack[L2_GUEST_STACK_SIZE]);
+	if (vmx->eptp_gpa)
+		l2_rip = l2_guest_code_ept_enabled;
+	else
+		l2_rip = l2_guest_code_ept_disabled;
+
+	prepare_vmcs(vmx, l2_rip, &l2_guest_stack[L2_GUEST_STACK_SIZE]);
 
 	GUEST_SYNC(false);
 	GUEST_ASSERT(!vmlaunch());
@@ -64,7 +80,7 @@ void l1_guest_code(struct vmx_pages *vmx)
 	GUEST_DONE();
 }
 
-int main(int argc, char *argv[])
+static void test_vmx_dirty_log(bool enable_ept)
 {
 	vm_vaddr_t vmx_pages_gva = 0;
 	struct vmx_pages *vmx;
@@ -76,8 +92,7 @@ int main(int argc, char *argv[])
 	struct ucall uc;
 	bool done = false;
 
-	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
-	TEST_REQUIRE(kvm_cpu_has_ept());
+	pr_info("Nested EPT: %s\n", enable_ept ? "enabled" : "disabled");
 
 	/* Create VM */
 	vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
@@ -103,11 +118,16 @@ int main(int argc, char *argv[])
 	 *
 	 * Note that prepare_eptp should be called only L1's GPA map is done,
 	 * meaning after the last call to virt_map.
+	 *
+	 * When EPT is disabled, the L2 guest code will still access the same L1
+	 * GPAs as the EPT enabled case.
 	 */
-	prepare_eptp(vmx, vm, 0);
-	nested_map_memslot(vmx, vm, 0);
-	nested_map(vmx, vm, NESTED_TEST_MEM1, GUEST_TEST_MEM, 4096);
-	nested_map(vmx, vm, NESTED_TEST_MEM2, GUEST_TEST_MEM, 4096);
+	if (enable_ept) {
+		prepare_eptp(vmx, vm, 0);
+		nested_map_memslot(vmx, vm, 0);
+		nested_map(vmx, vm, NESTED_TEST_MEM1, GUEST_TEST_MEM, 4096);
+		nested_map(vmx, vm, NESTED_TEST_MEM2, GUEST_TEST_MEM, 4096);
+	}
 
 	bmap = bitmap_zalloc(TEST_MEM_PAGES);
 	host_test_mem = addr_gpa2hva(vm, GUEST_TEST_MEM);
@@ -148,3 +168,15 @@ int main(int argc, char *argv[])
 		}
 	}
 }
+
+int main(int argc, char *argv[])
+{
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
+
+	test_vmx_dirty_log(/*enable_ept=*/false);
+
+	if (kvm_cpu_has_ept())
+		test_vmx_dirty_log(/*enable_ept=*/true);
+
+	return 0;
+}
-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test

[David Matlack]

On Fri, Mar 15, 2024 at 4:05 PM David Matlack <dmatlack@google.com> wrote:
>
> Extend vmx_dirty_log_test to include accesses made by L2 when EPT is
> disabled.
>
> This commit adds explicit coverage of a bug caught by syzkaller, where
> the TDP MMU would clear D-bits instead of write-protecting SPTEs being
> used to map an L2, which only happens when L1 does not enable EPT,
> causing writes made by L2 to not be reflected in the dirty log when PML
> is enabled:
>
>   $ ./vmx_dirty_log_test
>   Nested EPT: disabled
>   ==== Test Assertion Failure ====
>     x86_64/vmx_dirty_log_test.c:151: test_bit(0, bmap)
>     pid=72052 tid=72052 errno=4 - Interrupted system call
>     (stack trace empty)
>     Page 0 incorrectly reported clean
>
> Opportunistically replace the volatile casts with {READ,WRITE}_ONCE().
>
> Link: https://lore.kernel.org/kvm/000000000000c6526f06137f18cc@google.com/
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  .../selftests/kvm/x86_64/vmx_dirty_log_test.c | 60 ++++++++++++++-----
>  1 file changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c b/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
> index e4ad5fef52ff..609a767c4655 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
>
> -       *(volatile uint64_t *)NESTED_TEST_MEM2 = 1;
> +       READ_ONCE(*b);

This should be WRITE_ONCE(*b, 1). I forgot to reformat the patch after
I fixed this bug locally.

Re: [PATCH 1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits

[Sean Christopherson]

The shortlog is a bit too literal, e.g. without intimate knowledge of what
kvm_mmu_page_ad_need_write_protect() does, it's impossible to know what this
patch actually fixes.

In this case, since it's a bug fix, I think it makes sense to explicitly call
out the L2 SPTEs angled, at the cost of not capturing the more general gist of
the patch.

On Fri, Mar 15, 2024, David Matlack wrote:
> Check kvm_mmu_page_ad_need_write_protect() when deciding whether to
> write-protect or clear D-bits on TDP MMU SPTEs.
> 
> TDP MMU SPTEs must be write-protected when the TDP MMU is being used to
> run an L2 (i.e. L1 has disabled EPT) and PML is enabled. KVM always
> disables the PML hardware when running L2, so failing to write-protect
> TDP MMU SPTEs will cause writes made by L2 to not be reflected in the
> dirty log.

I massaged this slightly to explain what kvm_mmu_page_ad_need_write_protect()
does at a high level, at least as far as this patch is concerned.

    KVM: x86/mmu: Write-protect L2 SPTEs in TDP MMU when clearing dirty status
    
    Check kvm_mmu_page_ad_need_write_protect() when deciding whether to
    write-protect or clear D-bits on TDP MMU SPTEs, so that the TDP MMU
    accounts for any role-specific reasons for disabling D-bit dirty logging.
    
    Specifically, TDP MMU SPTEs must be write-protected when the TDP MMU is
    being used to run an L2 (i.e. L1 has disabled EPT) and PML is enabled.
    KVM always disables PML when running L2, even when L1 and L2 GPAs are in
    the some domain, so failing to write-protect TDP MMU SPTEs will cause
    writes made by L2 to not be reflected in the dirty log.
 
> Reported-by: syzbot+900d58a45dcaab9e4821@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=900d58a45dcaab9e4821
> Fixes: 5982a5392663 ("KVM: x86/mmu: Use kvm_ad_enabled() to determine if TDP MMU SPTEs need wrprot")
> Cc: stable@vger.kernel.org
> Cc: Vipin Sharma <vipinsh@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 6ae19b4ee5b1..c3c1a8f430ef 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -1498,6 +1498,16 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm,
>  	}
>  }
>  
> +static bool tdp_mmu_need_write_protect(struct kvm_mmu_page *sp)
> +{
> +	/*
> +	 * All TDP MMU shadow pages share the same role as their root, aside
> +	 * from level, so it is valid to key off any shadow page to determine if
> +	 * write protection is needed for an entire tree.
> +	 */
> +	return kvm_mmu_page_ad_need_write_protect(sp) || !kvm_ad_enabled();
> +}
> +
>  /*
>   * Clear the dirty status of all the SPTEs mapping GFNs in the memslot. If
>   * AD bits are enabled, this will involve clearing the dirty bit on each SPTE.
> @@ -1508,7 +1518,8 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm,
>  static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
>  			   gfn_t start, gfn_t end)
>  {
> -	u64 dbit = kvm_ad_enabled() ? shadow_dirty_mask : PT_WRITABLE_MASK;
> +	const u64 dbit = tdp_mmu_need_write_protect(root)
> +		? PT_WRITABLE_MASK : shadow_dirty_mask;

I would much prefer to keep the '?' and the first clause on the previous line.
Putting operators on a newline is frowned upon in general, and having the
PT_WRITABLE_MASK on the same line as tdp_mmu_need_write_protect() makes it quite
easy to understand the logic.

>  	struct tdp_iter iter;
>  	bool spte_set = false;
>  
> @@ -1523,7 +1534,7 @@ static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
>  		if (tdp_mmu_iter_cond_resched(kvm, &iter, false, true))
>  			continue;
>  
> -		KVM_MMU_WARN_ON(kvm_ad_enabled() &&
> +		KVM_MMU_WARN_ON(dbit == shadow_dirty_mask &&
>  				spte_ad_need_write_protect(iter.old_spte));
>  
>  		if (!(iter.old_spte & dbit))
> @@ -1570,8 +1581,8 @@ bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm,
>  static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
>  				  gfn_t gfn, unsigned long mask, bool wrprot)
>  {
> -	u64 dbit = (wrprot || !kvm_ad_enabled()) ? PT_WRITABLE_MASK :
> -						   shadow_dirty_mask;
> +	const u64 dbit = (wrprot || tdp_mmu_need_write_protect(root))
> +		? PT_WRITABLE_MASK : shadow_dirty_mask;

Same here.

>  	struct tdp_iter iter;
>  
>  	lockdep_assert_held_write(&kvm->mmu_lock);
> @@ -1583,7 +1594,7 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
>  		if (!mask)
>  			break;
>  
> -		KVM_MMU_WARN_ON(kvm_ad_enabled() &&
> +		KVM_MMU_WARN_ON(dbit == shadow_dirty_mask &&
>  				spte_ad_need_write_protect(iter.old_spte));
>  
>  		if (iter.level > PG_LEVEL_4K ||
> -- 
> 2.44.0.291.gc1ea87d7ee-goog
> 

Re: [PATCH 3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting

[Sean Christopherson]

On Fri, Mar 15, 2024, David Matlack wrote:
> Drop the "If AD bits are enabled/disabled" verbiage from the comments
> above kvm_tdp_mmu_clear_dirty_{slot,pt_masked}() since TDP MMU SPTEs may
> need to be write-protected even when A/D bits are enabled. i.e. These
> comments aren't technically correct.
> 
> No functional change intended.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 01192ac760f1..1e9b48b5f6e1 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -1544,11 +1544,9 @@ static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
>  }
>  
>  /*
> - * Clear the dirty status of all the SPTEs mapping GFNs in the memslot. If
> - * AD bits are enabled, this will involve clearing the dirty bit on each SPTE.
> - * If AD bits are not enabled, this will require clearing the writable bit on
> - * each SPTE. Returns true if an SPTE has been changed and the TLBs need to
> - * be flushed.
> + * Clear the dirty status (D-bit or W-bit) of all the SPTEs mapping GFNs in the
> + * memslot. Returns true if an SPTE has been changed and the TLBs need to be
> + * flushed.
>   */
>  bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm,
>  				  const struct kvm_memory_slot *slot)
> @@ -1606,11 +1604,9 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
>  }
>  
>  /*
> - * Clears the dirty status of all the 4k SPTEs mapping GFNs for which a bit is
> - * set in mask, starting at gfn. The given memslot is expected to contain all
> - * the GFNs represented by set bits in the mask. If AD bits are enabled,
> - * clearing the dirty status will involve clearing the dirty bit on each SPTE
> - * or, if AD bits are not enabled, clearing the writable bit on each SPTE.
> + * Clears the dirty status (D-bit or W-bit) of all the 4k SPTEs mapping GFNs for

Heh, purely because it stood out when reading the two comments back-to-back, I
I opportunistically used "Clear" instead of "Clears" so that the comments use
similar tone.

> + * which a bit is set in mask, starting at gfn. The given memslot is expected to
> + * contain all the GFNs represented by set bits in the mask.
>   */
>  void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm,
>  				       struct kvm_memory_slot *slot,
> -- 
> 2.44.0.291.gc1ea87d7ee-goog
> 

Re: [PATCH 0/4] KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled

[Sean Christopherson]

On Fri, 15 Mar 2024 16:05:37 -0700, David Matlack wrote:
> Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
> that causes writes made by L2 to no be reflected in the dirty log when
> L1 has disabled EPT.
> 
> Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
> dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
> logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
> bug.
> 
> [...]

Applied to kvm-x86 fixes, with the various tweaks mentioned in reply, and the
s/READ_ONCE/WRITE_ONCE fixup.  A sanity check would be nice though, I botched
the first attempt at the fixup (the one time I _should_ have copy+pasted code...).

Thanks!

[1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits
      https://github.com/kvm-x86/linux/commit/b44914b27e6b
[2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
      https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
[3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
      https://github.com/kvm-x86/linux/commit/5709b14d1cea
[4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
      https://github.com/kvm-x86/linux/commit/1d24b536d85b

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH 0/4] KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled

[David Matlack]

On Tue, Apr 9, 2024 at 5:20 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, 15 Mar 2024 16:05:37 -0700, David Matlack wrote:
> > Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
> > that causes writes made by L2 to no be reflected in the dirty log when
> > L1 has disabled EPT.
> >
> > Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
> > dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
> > logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
> > bug.
> >
> > [...]
>
> Applied to kvm-x86 fixes, with the various tweaks mentioned in reply, and the
> s/READ_ONCE/WRITE_ONCE fixup.  A sanity check would be nice though, I botched
> the first attempt at the fixup (the one time I _should_ have copy+pasted code...).
>
> Thanks!
>
> [1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits
>       https://github.com/kvm-x86/linux/commit/b44914b27e6b
> [2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
>       https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
> [3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
>       https://github.com/kvm-x86/linux/commit/5709b14d1cea
> [4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
>       https://github.com/kvm-x86/linux/commit/1d24b536d85b

This commit does not have the WRITE_ONCE() fixup, but when I look at
the commits in the fixes branch itself I see [1] which is correct.

The other commits look good as well, thanks for the various fixups.

[1] https://github.com/kvm-x86/linux/commit/f1ef5c343399ec5e21eb1e1e0093e731dce2fa1e

>
> --
> https://github.com/kvm-x86/linux/tree/next

Re: [PATCH 0/4] KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled

[Sean Christopherson]

On Wed, Apr 10, 2024, David Matlack wrote:
> On Tue, Apr 9, 2024 at 5:20 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, 15 Mar 2024 16:05:37 -0700, David Matlack wrote:
> > > Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
> > > that causes writes made by L2 to no be reflected in the dirty log when
> > > L1 has disabled EPT.
> > >
> > > Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
> > > dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
> > > logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
> > > bug.
> > >
> > > [...]
> >
> > Applied to kvm-x86 fixes, with the various tweaks mentioned in reply, and the
> > s/READ_ONCE/WRITE_ONCE fixup.  A sanity check would be nice though, I botched
> > the first attempt at the fixup (the one time I _should_ have copy+pasted code...).
> >
> > Thanks!
> >
> > [1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits
> >       https://github.com/kvm-x86/linux/commit/b44914b27e6b
> > [2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
> >       https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
> > [3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
> >       https://github.com/kvm-x86/linux/commit/5709b14d1cea
> > [4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
> >       https://github.com/kvm-x86/linux/commit/1d24b536d85b
> 
> This commit does not have the WRITE_ONCE() fixup, but when I look at
> the commits in the fixes branch itself I see [1] which is correct.

Argh, I must have forgot to copy+paste in the correct hashes (like I said above,
it took me a few tries to get things right).

For posterity...

[1/4] KVM: x86/mmu: Write-protect L2 SPTEs in TDP MMU when clearing dirty status
      https://github.com/kvm-x86/linux/commit/b44914b27e6b
[2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
      https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
[3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
      https://github.com/kvm-x86/linux/commit/5709b14d1cea
[4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
      https://github.com/kvm-x86/linux/commit/f1ef5c343399