linux-kernel - Re: [PATCH] KVM: Use precise range-based flush in mmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240823121538.GA32110@willie-the-truck>
Date: Fri, 23 Aug 2024 13:15:38 +0100
From: Will Deacon <will@...nel.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, Marc Zyngier <maz@...nel.org>
Subject: Re: [PATCH] KVM: Use precise range-based flush in mmu_notifier hooks
 when possible

On Tue, Aug 20, 2024 at 10:06:00AM -0700, Sean Christopherson wrote:
> On Tue, Aug 20, 2024, Will Deacon wrote:
> > On Tue, Aug 20, 2024 at 09:07:22AM -0700, Sean Christopherson wrote:
> > > On Tue, Aug 20, 2024, Will Deacon wrote:
> > > > handler could do the invalidation as part of its page-table walk (for
> > > > example, it could use information about the page-table structure such
> > > > as the level of the leaves to optimise the invalidation further), but
> > > > this does at least avoid zapping the whole VMID on CPUs with range
> > > > support.
> > > > 
> > > > My only slight concern is that, should clear_flush_young() be extended
> > > > to operate on more than a single page-at-a-time in future, this will
> > > > silently end up invalidating the entire VMID for each memslot unless we
> > > > teach kvm_arch_flush_remote_tlbs_range() to return !0 in that case.
> > > 
> > > I'm not sure I follow the "entire VMID for each memslot" concern.  Are you
> > > worried about kvm_arch_flush_remote_tlbs_range() failing and triggering a VM-wide
> > > flush?
> > 
> > The arm64 implementation of kvm_arch_flush_remote_tlbs_range()
> > unconditionally returns 0, so we could end up over-invalidating pretty
> > badly if that doesn't change. It should be straightforward to fix, but
> > I just wanted to point it out because it would be easy to miss too!
> 
> Sorry, I'm still not following.  0==success, and gfn_range.{start,end} is scoped
> precisely to the overlap between the memslot and hva range.  Regardless of the
> number of pages that are passed into clear_flush_young(), KVM should naturally
> flush only the exact range being aged.  The only hiccup would be if the hva range
> straddles multiple memslots, but if userspace creates multiple memslots for a
> single vma, then that's a userspace problem.

Fair enough, but it's not a lot of effort to fix this (untested diff
below) and if the code were to change in future so that
__kvm_handle_hva_range() was more commonly used to span multiple
memslots we probably wouldn't otherwise notice the silent
over-invalidation for a while.

Will

--->8

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 6981b1bc0946..1e34127f79b0 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -175,6 +175,9 @@ int kvm_arch_flush_remote_tlbs(struct kvm *kvm)
 int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm,
                                      gfn_t gfn, u64 nr_pages)
 {
+       if (!system_supports_tlb_range())
+               return -EOPNOTSUPP;
+
        kvm_tlb_flush_vmid_range(&kvm->arch.mmu,
                                gfn << PAGE_SHIFT, nr_pages << PAGE_SHIFT);
        return 0;