Bug 219588
| Summary: | [6.13.0-rc2+]WARNING: CPU: 52 PID: 12253 at arch/x86/kvm/mmu/tdp_mmu.c:1001 tdp_mmu_map_handle_target_level+0x1f0/0x310 [kvm] | ||
|---|---|---|---|
| Product: | Virtualization | Reporter: | leiyang |
| Component: | kvm | Assignee: | virtualization_kvm |
| Status: | NEW --- | ||
| Severity: | normal | ||
| Priority: | P3 | ||
| Hardware: | Intel | ||
| OS: | Linux | ||
| Kernel Version: | Subsystem: | ||
| Regression: | No | Bisected commit-id: | |
|
Description
leiyang
2024-12-11 05:41:06 UTC
On Wed, Dec 11, 2024, bugzilla-daemon@kernel.org wrote: > I hit a bug on the intel host, this problem occurs randomly: > [ 406.127925] ------------[ cut here ]------------ > [ 406.132572] WARNING: CPU: 52 PID: 12253 at arch/x86/kvm/mmu/tdp_mmu.c:1001 > tdp_mmu_map_handle_target_level+0x1f0/0x310 [kvm] Can you describe the host activity at the time of the WARN? E.g. is it under memory pressure and potentially swapping, is KSM or NUMA balancing active? I have a sound theory for how the scenario occurs on KVM's end, but I still think it's wrong for KVM to overwrite a writable SPTE with a read-only SPTE in this situation. And does the VM have device memory or any other type of VM_PFNMAP or VM_IO memory exposed to it? E.g. an assigned device? If so, can you provide the register state from the other WARNs? If the PFNs are all in the same range, then maybe this is something funky with the VM_PFNMAP | VM_IO path. The WARN is a sanity check I added because it should be impossible for KVM to install a non-writable SPTE overtop an existing writable SPTE. Or so I thought. The WARN is benign in the sense that nothing bad will happen _in KVM_; KVM correctly handles the unexpected change, the WARN is there purely to flag that something unexpected happen. if (new_spte == iter->old_spte) ret = RET_PF_SPURIOUS; else if (tdp_mmu_set_spte_atomic(vcpu->kvm, iter, new_spte)) return RET_PF_RETRY; else if (is_shadow_present_pte(iter->old_spte) && (!is_last_spte(iter->old_spte, iter->level) || WARN_ON_ONCE(leaf_spte_change_needs_tlb_flush(iter->old_spte, new_spte)))) <==== kvm_flush_remote_tlbs_gfn(vcpu->kvm, iter->gfn, iter->level); Cross referencing the register state RAX: 860000025e000bf7 RBX: ff4af92c619cf920 RCX: 0400000000000000 RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000015 RBP: ff4af92c619cf9e8 R08: 800000025e0009f5 R09: 0000000000000002 R10: 000000005e000901 R11: 0000000000000001 R12: ff1e70694fc68000 R13: 0000000000000005 R14: 0000000000000000 R15: ff4af92c619a1000 with the disassembly 4885C8 TEST RAX,RCX 0F84EEFEFFFF JE 0000000000000-F1 4985C8 TEST R8,RCX 0F85E5FEFFFF JNE 0000000000000-F1 0F0B UD2 RAX is the old SPTE and RCX is the new SPTE, i.e. the SPTE change is: 860000025e000bf7 800000025e0009f5 On Intel, bits 57 and 58 are the host-writable and MMU-writable flags #define EPT_SPTE_HOST_WRITABLE BIT_ULL(57) #define EPT_SPTE_MMU_WRITABLE BIT_ULL(58) which means KVM is overwriting a writable SPTE with a non-writable SPTE because the current vCPU (a) hit a READ or EXEC fault on a non-present SPTE and (b) retrieved a non-writable PFN from the primary MMU, and that fault raced with a WRITE fault on a different vCPU that retrieved and installed a writable PFN. On a READ or EXEC fault, this code in hva_to_pfn_slow() should get a writable PFN. Given that KVM has an valid writable SPTE, the corresponding PTE in the primary MMU *must* be writable, otherwise there's a missing mmu_notifier invalidation. /* map read fault as writable if possible */ if (!(flags & FOLL_WRITE) && kfp->map_writable && get_user_page_fast_only(kfp->hva, FOLL_WRITE, &wpage)) { put_page(page); page = wpage; flags |= FOLL_WRITE; } out: *pfn = kvm_resolve_pfn(kfp, page, NULL, flags & FOLL_WRITE); return npages; Hmm, gup_fast_folio_allowed() has a few conditions where it will reject fast GUP, but they should be mutually exclusive with KVM having a writable SPTE. If the mapping is truncated or the folio is swapped out, secondary MMUs need to be invalidated before folio->mapping is nullified. /* * The mapping may have been truncated, in any case we cannot determine * if this mapping is safe - fall back to slow path to determine how to * proceed. */ if (!mapping) return false; And secretmem can't be GUP'd, and it's not a long-term pin, so these checks don't apply either: if (check_secretmem && secretmem_mapping(mapping)) return false; /* The only remaining allowed file system is shmem. */ return !reject_file_backed || shmem_mapping(mapping); Similarly, hva_to_pfn_remapped() should get a writable PFN if said PFN is writable in the primary MMU, regardless of the fault type. If this turns out to get a legitimate scenario, then I think it makes sense to add an is_access_allowed() check and treat the fault as spurious. But I would like to try to bottom out on what exactly is happening, because I'm mildly concerned something is buggy in the primary MMU. (In reply to Sean Christopherson from comment #1) > On Wed, Dec 11, 2024, bugzilla-daemon@kernel.org wrote: > > I hit a bug on the intel host, this problem occurs randomly: > > [ 406.127925] ------------[ cut here ]------------ > > [ 406.132572] WARNING: CPU: 52 PID: 12253 at > arch/x86/kvm/mmu/tdp_mmu.c:1001 > > tdp_mmu_map_handle_target_level+0x1f0/0x310 [kvm] > > Can you describe the host activity at the time of the WARN? E.g. is it under > memory pressure and potentially swapping, is KSM or NUMA balancing active? I > have a sound theory for how the scenario occurs on KVM's end, but I still > think > it's wrong for KVM to overwrite a writable SPTE with a read-only SPTE in this > situation. I spent some time for this problem so late reply. When host dmesg print this error messages which running install a new guest via automation. And I found this bug's reproducer is run this install case after the mchine first time running(Let me introduce more to avoid ambiguity： 1. Must to test it when the machine first time running this kernel,that mean's if I hit this problem then reboot host, it can not reproduced again even if I run the same tests. 2. Based on 1, I also must test this installation guest case, it can not reporduced on other cases.). But through compare, this installation cases only used pxe install based on a internal KS cfg is different other cases. Sure, I think it's running under memory pressure and swapping. Based on automation log, KSM is disable and I don't add NUMA in qemu command line. If you have a machine can clone avocado and run tp-qemu tests, you can prepare env then run this case: unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads > > And does the VM have device memory or any other type of VM_PFNMAP or VM_IO > memory exposed to it? E.g. an assigned device? If so, can you provide the > register > state from the other WARNs? If the PFNs are all in the same range, then > maybe > this is something funky with the VM_PFNMAP | VM_IO path. I can confirm it has VM_IO because it runing installation case, VM is constantly performing I/O operations This my tests used memory and CPU configured, hope it help you debug this problem: -m 29G \ -smp 48,maxcpus=48,cores=24,threads=1,dies=1,sockets=2 \ And looks like there are no other device and no using VM_PFNMAP. Please correct me if I'm wrong. > > The WARN is a sanity check I added because it should be impossible for KVM to > install a non-writable SPTE overtop an existing writable SPTE. Or so I > thought. > The WARN is benign in the sense that nothing bad will happen _in KVM_; KVM > correctly handles the unexpected change, the WARN is there purely to flag > that > something unexpected happen. > > if (new_spte == iter->old_spte) > ret = RET_PF_SPURIOUS; > else if (tdp_mmu_set_spte_atomic(vcpu->kvm, iter, new_spte)) > return RET_PF_RETRY; > else if (is_shadow_present_pte(iter->old_spte) && > (!is_last_spte(iter->old_spte, iter->level) || > WARN_ON_ONCE(leaf_spte_change_needs_tlb_flush(iter->old_spte, > new_spte)))) <==== > kvm_flush_remote_tlbs_gfn(vcpu->kvm, iter->gfn, iter->level); > > Cross referencing the register state > > RAX: 860000025e000bf7 RBX: ff4af92c619cf920 RCX: 0400000000000000 > RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000015 > RBP: ff4af92c619cf9e8 R08: 800000025e0009f5 R09: 0000000000000002 > R10: 000000005e000901 R11: 0000000000000001 R12: ff1e70694fc68000 > R13: 0000000000000005 R14: 0000000000000000 R15: ff4af92c619a1000 > > with the disassembly > > 4885C8 TEST RAX,RCX > 0F84EEFEFFFF JE 0000000000000-F1 > 4985C8 TEST R8,RCX > 0F85E5FEFFFF JNE 0000000000000-F1 > 0F0B UD2 > > RAX is the old SPTE and RCX is the new SPTE, i.e. the SPTE change is: > > 860000025e000bf7 > 800000025e0009f5 > > On Intel, bits 57 and 58 are the host-writable and MMU-writable flags > > #define EPT_SPTE_HOST_WRITABLE BIT_ULL(57) > #define EPT_SPTE_MMU_WRITABLE BIT_ULL(58) > > which means KVM is overwriting a writable SPTE with a non-writable SPTE > because > the current vCPU (a) hit a READ or EXEC fault on a non-present SPTE and (b) > retrieved > a non-writable PFN from the primary MMU, and that fault raced with a WRITE > fault > on a different vCPU that retrieved and installed a writable PFN. > > On a READ or EXEC fault, this code in hva_to_pfn_slow() should get a > writable PFN. > Given that KVM has an valid writable SPTE, the corresponding PTE in the > primary MMU > *must* be writable, otherwise there's a missing mmu_notifier invalidation. > > /* map read fault as writable if possible */ > if (!(flags & FOLL_WRITE) && kfp->map_writable && > get_user_page_fast_only(kfp->hva, FOLL_WRITE, &wpage)) { > put_page(page); > page = wpage; > flags |= FOLL_WRITE; > } > > out: > *pfn = kvm_resolve_pfn(kfp, page, NULL, flags & FOLL_WRITE); > return npages; > > Hmm, gup_fast_folio_allowed() has a few conditions where it will reject fast > GUP, > but they should be mutually exclusive with KVM having a writable SPTE. If > the > mapping is truncated or the folio is swapped out, secondary MMUs need to be > invalidated before folio->mapping is nullified. > > /* > * The mapping may have been truncated, in any case we cannot determine > * if this mapping is safe - fall back to slow path to determine how to > * proceed. > */ > if (!mapping) > return false; > > And secretmem can't be GUP'd, and it's not a long-term pin, so these checks > don't > apply either: > > if (check_secretmem && secretmem_mapping(mapping)) > return false; > /* The only remaining allowed file system is shmem. */ > return !reject_file_backed || shmem_mapping(mapping); > > Similarly, hva_to_pfn_remapped() should get a writable PFN if said PFN is > writable > in the primary MMU, regardless of the fault type. > > If this turns out to get a legitimate scenario, then I think it makes sense > to > add an is_access_allowed() check and treat the fault as spurious. But I > would > like to try to bottom out on what exactly is happening, because I'm mildly > concerned something is buggy in the primary MMU. If you need me to provide more info, please feel free to let me know. And if you sent a patch to fix this problem I can help to verified it, since I think I found the stable reproducer. On Mon, Dec 16, 2024, bugzilla-daemon@kernel.org wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=219588 > > --- Comment #2 from leiyang@redhat.com --- > (In reply to Sean Christopherson from comment #1) > > On Wed, Dec 11, 2024, bugzilla-daemon@kernel.org wrote: > > > I hit a bug on the intel host, this problem occurs randomly: > > > [ 406.127925] ------------[ cut here ]------------ > > > [ 406.132572] WARNING: CPU: 52 PID: 12253 at > > arch/x86/kvm/mmu/tdp_mmu.c:1001 > > > tdp_mmu_map_handle_target_level+0x1f0/0x310 [kvm] > > > > Can you describe the host activity at the time of the WARN? E.g. is it > under > > memory pressure and potentially swapping, is KSM or NUMA balancing active? > I > > have a sound theory for how the scenario occurs on KVM's end, but I still > > think > > it's wrong for KVM to overwrite a writable SPTE with a read-only SPTE in > this > > situation. > > I spent some time for this problem so late reply. When host dmesg print this > error messages which running install a new guest via automation. And I found > this bug's reproducer is run this install case after the mchine first time > running(Let me introduce more to avoid ambiguity： 1. Must to test it when the > machine first time running this kernel,that mean's if I hit this problem then > reboot host, it can not reproduced again even if I run the same tests. Yeah, WARN_ON_ONCE() suppresses the splat after a single occurrence. If KVM is built as a module, you can can unload and reload kvm.ko (and whatever vendor you're using) instead of rebooting. The flag that controls the "once" behavior is tied to the lifetime of kvm.ko. > 2. Based on 1, I also must test this installation guest case, it can not > reporduced on other cases.). But through compare, this installation cases > only used pxe install based on a internal KS cfg is different other cases. Mostly out of curiosity, what's "KS cfg"? > Sure, I think it's running under memory pressure and swapping. Based on > automation log, KSM is disable and I don't add NUMA in qemu command line. > > If you have a machine can clone avocado and run tp-qemu tests, you can > prepare > env then run this case: > unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads > > > > > And does the VM have device memory or any other type of VM_PFNMAP or VM_IO > > memory exposed to it? E.g. an assigned device? If so, can you provide the > > register > > state from the other WARNs? If the PFNs are all in the same range, then > > maybe > > this is something funky with the VM_PFNMAP | VM_IO path. > > I can confirm it has VM_IO because it runing installation case, VM is > constantly performing I/O operations Heh, VM_IO and emulated I/O for the guest are two very different things. VM_IO would typically only be present if you're doing some form of device passthrough. Regardless, I don't know that it's worth trying to figure out exactly what's happening in the primary MMU. What's happening is perfectly legal, and the odds of there being an underlying bug *and* it not having any other symptoms is very low. And it definitely doesn't make sense to withold a fix just because I'm paranoid :-) So, assuming you can test custom kernels, can you test this and confirm it makes the WARN go away? If it does, or if you can't test it, I'll post a proper patch. If it doesn't fix the issue, then something completely unexpected is happening and we'll have to dig deeper. --- From: Sean Christopherson <seanjc@google.com> Date: Mon, 16 Dec 2024 13:18:01 -0800 Subject: [PATCH] KVM: x86/mmu: Treat TDP MMU faults as spurious if access is already allowed Treat slow-path TDP MMU faults as spurious if the access is allowed given the existing SPTE to fix a benign warning (other than the WARN itself) due to replacing a writable SPTE with a read-only SPTE, and to avoid the unnecessary LOCK CMPXCHG and subsequent TLB flush. If a read fault races with a write fault, fast GUP fails for any reason when trying to "promote" the read fault to a writable mapping, and KVM resolves the write fault first, then KVM will end up trying to install a read-only SPTE (for a !map_writable fault) overtop a writable SPTE. Note, it's not entirely clear why fast GUP fails, or if that's even how KVM ends up with a !map_writable fault with a writable SPTE. If something else is going awry, e.g. due to a bug in mmu_notifiers, then treating read faults as spurious in this scenario could effectively mask the underlying problem. However, retrying the faulting access instead of overwriting an existing SPTE is functionally correct and desirable irrespective of the WARN, and fast GUP _can_ legitimately fail with a writable VMA, e.g. if the Accessed bit in primary MMU's PTE is toggled and causes a PTE value mismatch. The WARN was also recently added, specifically to track down scenarios where KVM is unnecessarily overwrites SPTEs, i.e. treating the fault as spurious doesn't regress KVM's bug-finding capabilities in any way. In short, letting the WARN linger because there's a tiny chance it's due to a bug elsewhere would be excessively paranoid. Fixes: 1a175082b190 ("KVM: x86/mmu: WARN and flush if resolving a TDP MMU fault clears MMU-writable") Reported-by: leiyang@redhat.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219588 Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/mmu/mmu.c | 12 ------------ arch/x86/kvm/mmu/spte.h | 17 +++++++++++++++++ arch/x86/kvm/mmu/tdp_mmu.c | 5 +++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 22e7ad235123..2401606db260 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3364,18 +3364,6 @@ static bool fast_pf_fix_direct_spte(struct kvm_vcpu *vcpu, return true; } -static bool is_access_allowed(struct kvm_page_fault *fault, u64 spte) -{ - if (fault->exec) - return is_executable_pte(spte); - - if (fault->write) - return is_writable_pte(spte); - - /* Fault was on Read access */ - return spte & PT_PRESENT_MASK; -} - /* * Returns the last level spte pointer of the shadow page walk for the given * gpa, and sets *spte to the spte value. This spte may be non-preset. If no diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h index f332b33bc817..6285c45fa56d 100644 --- a/arch/x86/kvm/mmu/spte.h +++ b/arch/x86/kvm/mmu/spte.h @@ -461,6 +461,23 @@ static inline bool is_mmu_writable_spte(u64 spte) return spte & shadow_mmu_writable_mask; } +/* + * Returns true if the access indiciated by @fault is allowed by the existing + * SPTE protections. Note, the caller is responsible for checking that the + * SPTE is a shadow-present, leaf SPTE (either before or after). + */ +static inline bool is_access_allowed(struct kvm_page_fault *fault, u64 spte) +{ + if (fault->exec) + return is_executable_pte(spte); + + if (fault->write) + return is_writable_pte(spte); + + /* Fault was on Read access */ + return spte & PT_PRESENT_MASK; +} + /* * If the MMU-writable flag is cleared, i.e. the SPTE is write-protected for * write-tracking, remote TLBs must be flushed, even if the SPTE was read-only, diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 4508d868f1cd..2f15e0e33903 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -985,6 +985,11 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, if (fault->prefetch && is_shadow_present_pte(iter->old_spte)) return RET_PF_SPURIOUS; + if (is_shadow_present_pte(iter->old_spte) && + is_access_allowed(fault, iter->old_spte) && + is_last_spte(iter->old_spte, iter->level)) + return RET_PF_SPURIOUS; + if (unlikely(!fault->slot)) new_spte = make_mmio_spte(vcpu, iter->gfn, ACC_ALL); else base-commit: 3522c419758ee8dca5a0e8753ee0070a22157bc1 Hi (In reply to Sean Christopherson from comment #3) > On Mon, Dec 16, 2024, bugzilla-daemon@kernel.org wrote > However, retrying the faulting access instead of overwriting an existing > SPTE is functionally correct and desirable irrespective of the WARN, and > fast GUP _can_ legitimately fail with a writable VMA, e.g. if the Accessed > bit in primary MMU's PTE is toggled and causes a PTE value mismatch. The > WARN was also recently added, specifically to track down scenarios where > KVM is unnecessarily overwrites SPTEs, i.e. treating the fault as spurious > doesn't regress KVM's bug-finding capabilities in any way. In short, > letting the WARN linger because there's a tiny chance it's due to a bug > elsewhere would be excessively paranoid. > > Fixes: 1a175082b190 ("KVM: x86/mmu: WARN and flush if resolving a TDP MMU > fault clears MMU-writable") > Reported-by: leiyang@redhat.com > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219588 > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 12 ------------ > arch/x86/kvm/mmu/spte.h | 17 +++++++++++++++++ > arch/x86/kvm/mmu/tdp_mmu.c | 5 +++++ > 3 files changed, 22 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 22e7ad235123..2401606db260 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -3364,18 +3364,6 @@ static bool fast_pf_fix_direct_spte(struct kvm_vcpu > *vcpu, > return true; > } > > -static bool is_access_allowed(struct kvm_page_fault *fault, u64 spte) > -{ > - if (fault->exec) > - return is_executable_pte(spte); > - > - if (fault->write) > - return is_writable_pte(spte); > - > - /* Fault was on Read access */ > - return spte & PT_PRESENT_MASK; > -} > - > /* > * Returns the last level spte pointer of the shadow page walk for the given > * gpa, and sets *spte to the spte value. This spte may be non-preset. If no > diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h > index f332b33bc817..6285c45fa56d 100644 > --- a/arch/x86/kvm/mmu/spte.h > +++ b/arch/x86/kvm/mmu/spte.h > @@ -461,6 +461,23 @@ static inline bool is_mmu_writable_spte(u64 spte) > return spte & shadow_mmu_writable_mask; > } > > +/* > + * Returns true if the access indiciated by @fault is allowed by the > existing > + * SPTE protections. Note, the caller is responsible for checking that the > + * SPTE is a shadow-present, leaf SPTE (either before or after). > + */ > +static inline bool is_access_allowed(struct kvm_page_fault *fault, u64 spte) > +{ > + if (fault->exec) > + return is_executable_pte(spte); > + > + if (fault->write) > + return is_writable_pte(spte); > + > + /* Fault was on Read access */ > + return spte & PT_PRESENT_MASK; > +} > + > /* > * If the MMU-writable flag is cleared, i.e. the SPTE is write-protected for > * write-tracking, remote TLBs must be flushed, even if the SPTE was > read-only, > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 4508d868f1cd..2f15e0e33903 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -985,6 +985,11 @@ static int tdp_mmu_map_handle_target_level(struct > kvm_vcpu *vcpu, > if (fault->prefetch && is_shadow_present_pte(iter->old_spte)) > return RET_PF_SPURIOUS; > > + if (is_shadow_present_pte(iter->old_spte) && > + is_access_allowed(fault, iter->old_spte) && > + is_last_spte(iter->old_spte, iter->level)) > + return RET_PF_SPURIOUS; > + > if (unlikely(!fault->slot)) > new_spte = make_mmio_spte(vcpu, iter->gfn, ACC_ALL); > else > > base-commit: 3522c419758ee8dca5a0e8753ee0070a22157bc1 Hi Sean This problem has gone after applied your provide patch. Thanks Lei |