Call the set_external_spte() hook only when the present bit is changed from
non-present to present.
Observation of the issue
------------------------
An issue was reported on an out-of-tree kernel with an older version of TDX
support, but that uses similar code to the current existing mirror/external
MMU support. It can't be triggered without future patches. In the out of
tree kernel it appears as a KVM_BUG_ON() getting hit in TDX code when
set_external_spte() was called on a PTE that was already present.
Investigation turned up that:
1. It was due to the AD state machine trying to update the dirty bit
2. A problem in the currently queued mirror/external PTE, but that can't be
hit until more TDX support is added.
In more detail, the spotted issue was caused by two vcpus simultaneously
faulting for same GFN:
vcpu0 vcpu1
----- -----
ept violation for GFN ept violation for GFN
encounter !present mirror PTE
set_external_spte()
re-enter TD, and accept GFN
encounter present mirror PTE
trigger AD state machine transition
see old/new PTEs are different
call set_external_spte() again
The TDX external TDP backend couldn't handle the second call to
set_external_spte(). In recent TDX development branches, the
TDH.MEM.PAGE.AUG() SEAMCALL() hook would fail unexpectedly with
TDX_EPT_ENTRY_STATE_INCORRECT + SPTE PENDING or PRESENT. However, the
callback assumes that it's called only when non-present => present leaf
SPTE change. It shows as a triggering a KVM_BUG_ON() with a stacktrace
like:
WARNING: CPU: 4 PID: 3700 at tdx_mem_page_aug+0x16d/0x560 [kvm_intel]
IP: 0010:tdx_mem_page_aug+0x16d/0x560 [kvm_intel]
Call Trace:
set_external_spte_present+0x244/0x7e0
tdp_mmu_map_handle_target_level+0x460/0xd60
kvm_tdp_mmu_map+0x9c6/0xdc0
kvm_tdp_mmu_page_fault+0x32b/0x3e0
kvm_mmu_do_page_fault+0x4e5/0x6a0
kvm_mmu_page_fault+0x1a0/0x5e0
vcpu_enter_guest+0x1f5f/0x3900
vcpu_run+0x133/0xb60
kvm_arch_vcpu_ioctl_run+0x8ef/0x1650
kvm_vcpu_ioctl+0x4f9/0xb70
__x64_sys_ioctl+0x132/0x1a0
do_syscall_64+0xc1/0x1d0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Design of MMU mirror/external
-----------------------------
In current mirror/external MMU support, the set_external_spte() allows
external PTEs to be set as present, but doesn't allow configuration of any
other PTE bits. remove_external_spte() allows external PTEs to be zapped.
Based on the fact that external PTE callbacks are exposed to essentially
configure two PTE states (present or not present), and that mirror PTEs
record whether the external PTE is present or not, future patches were
planned to introduce a TDX backend for mirror/external page tables to not
expect set_external_spte() calls on already present external PTEs. To
accommodate this future code, steps were taken in the mirror/external
support in the core MMU to prevent permission bits changing on such PTEs.
However, the AD state machine scenario was missed. When the multiple vCPUs
fault in the same GFN simultaneously, the hook is called multiple times
with some bits changed while both old/new SPTEs have the present bits. The
leaf SPTE is a complex state machine in general, and the value can change
with software available bits and hardware bits.
Solution
--------
There are several options to address this:
- Tame the SPTE state machine so that SPTE change won't happen for mirrored
page table.
PRO: It's conceptually natural to disable D bit support because mirrored
page table doesn't support AD bits.
CON: Not only D bit but also other bits can be modified.
setting strict kvm_page_table.role.ad_disabled = true doesn't work.
It requires to change the SPTE more deeply.
- Add a check to the TDP MMU so that it won't call the hook unnecessarily
PRO: Direct fix that shares set_external_spte() expectations in core
MMU code.
CON: Hard code in the TDP MMU when the hook is needed.
- Pass old and new SPTE values to the hooks, add a check to the backend
PRO: The backend can determine whether the callback is needed.
CON: The KVM MMU implementation details leak to the backend because
The SPTE encoding is specific to the KVM MMU implementation.
And it's subject to change in the future.
For example, is_shadow_present_pte() needs to be exported from the
KVM MMU to the backend.
The first choice is out because it doesn't easily handle the problem.
Choose the second option over the third choice because the current
interesting change is only non-present => present because we don't support
dirty page tracking by write protect and large page yet by TDX KVM for now.
(TDX supports them.) They're future work for TDX KVM.
Care only about the leaf SPTE because non-leaf doesn't have a state
machine.
Reported-by: Sagi Shahar <sagis@xxxxxxxxxx>
Fixes: b6bcd88ad43a ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
[log heavily edited by Rick]
Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
---
v2:
- Removed KVM_BUG_ON() into another patch (Sean)
- Removed pr_debug() (Sean)
v1:
https://lore.kernel.org/kvm/6eecc450d0326c9bedfbb34096a0279410923c8d.1726182754.git.isaku.yamahata@xxxxxxxxx/
---
arch/x86/kvm/mmu/tdp_mmu.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 1da3df517522..c7f251549973 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -503,8 +503,6 @@ static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
kvm_pfn_t new_pfn = spte_to_pfn(new_spte);
int ret = 0;
- KVM_BUG_ON(was_present, kvm);
-
lockdep_assert_held(&kvm->mmu_lock);
/*
* We need to lock out other updates to the SPTE until the external
@@ -519,10 +517,16 @@ static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
* external page table, or leaf.
*/
if (is_leaf) {
- ret = static_call(kvm_x86_set_external_spte)(kvm, gfn, level, new_pfn);
+ /*
+ * SPTE is state machine with software available bits used.
+ * Check if the change is interesting to the backend.
+ */
+ if (!was_present)
+ ret = static_call(kvm_x86_set_external_spte)(kvm, gfn, level, new_pfn);
} else {
void *external_spt = get_external_spt(gfn, new_spte, level);
+ KVM_BUG_ON(was_present, kvm);
KVM_BUG_ON(!external_spt, kvm);
ret = static_call(kvm_x86_link_external_spt)(kvm, gfn, level, external_spt);
}
--
2.45.2
