Message ID | 20220805230513.148869-9-seanjc@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: x86: Apply NX mitigation more precisely | expand |
On 8/6/22 01:05, Sean Christopherson wrote: > !is_large_pte(spte)) { > + u64 page_mask; > + > + /* > + * Ensure nx_huge_page_disallowed is read after checking for a > + * present shadow page. A different vCPU may be concurrently > + * installing the shadow page if mmu_lock is held for read. > + * Pairs with the smp_wmb() in kvm_tdp_mmu_map(). > + */ > + smp_rmb(); > + > + if (!spte_to_child_sp(spte)->nx_huge_page_disallowed) > + return; > + I wonder if the barrier shouldn't be simply in to_shadow_page(), i.e. always assume in the TDP MMU code that sp->xyz is read after the SPTE that points to that struct kvm_mmu_page. Paolo
On Tue, Aug 09, 2022, Paolo Bonzini wrote: > On 8/6/22 01:05, Sean Christopherson wrote: > > !is_large_pte(spte)) { > > + u64 page_mask; > > + > > + /* > > + * Ensure nx_huge_page_disallowed is read after checking for a > > + * present shadow page. A different vCPU may be concurrently > > + * installing the shadow page if mmu_lock is held for read. > > + * Pairs with the smp_wmb() in kvm_tdp_mmu_map(). > > + */ > > + smp_rmb(); > > + > > + if (!spte_to_child_sp(spte)->nx_huge_page_disallowed) > > + return; > > + > > I wonder if the barrier shouldn't be simply in to_shadow_page(), i.e. always > assume in the TDP MMU code that sp->xyz is read after the SPTE that points > to that struct kvm_mmu_page. If we can get away with it, I'd prefer to rely on the READ_ONCE() in kvm_tdp_mmu_read_spte() and required ordering of: READ_ONCE() => PRESENT => spte_to_child_sp()
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 1442129c85e0..3ddfc82868fd 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3090,6 +3090,19 @@ void disallowed_hugepage_adjust(struct kvm_page_fault *fault, u64 spte, int cur_ cur_level == fault->goal_level && is_shadow_present_pte(spte) && !is_large_pte(spte)) { + u64 page_mask; + + /* + * Ensure nx_huge_page_disallowed is read after checking for a + * present shadow page. A different vCPU may be concurrently + * installing the shadow page if mmu_lock is held for read. + * Pairs with the smp_wmb() in kvm_tdp_mmu_map(). + */ + smp_rmb(); + + if (!spte_to_child_sp(spte)->nx_huge_page_disallowed) + return; + /* * A small SPTE exists for this pfn, but FNAME(fetch) * and __direct_map would like to create a large PTE @@ -3097,8 +3110,8 @@ void disallowed_hugepage_adjust(struct kvm_page_fault *fault, u64 spte, int cur_ * patching back for them into pfn the next 9 bits of * the address. */ - u64 page_mask = KVM_PAGES_PER_HPAGE(cur_level) - - KVM_PAGES_PER_HPAGE(cur_level - 1); + page_mask = KVM_PAGES_PER_HPAGE(cur_level) - + KVM_PAGES_PER_HPAGE(cur_level - 1); fault->pfn |= fault->gfn & page_mask; fault->goal_level--; } diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 526d38704e5c..c5314ca95e08 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -1202,7 +1202,8 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) /* * Ensure nx_huge_page_disallowed is visible before the * SP is marked present, as mmu_lock is held for read. - * Pairs with the smp_rmb() in tdp_mmu_unlink_sp(). + * Pairs with the smp_rmb() in tdp_mmu_unlink_sp() and + * in disallowed_hugepage_adjust(). */ smp_wmb();