linux-kernel - Re: [PATCH] KVM: Move gfn_to_pfn_cache invalidation to invalidate_range

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: <dd6ca54cfd23dba0d3cba7c1ceefea1fdfcdecbe.camel@infradead.org>
Date: Tue, 06 Aug 2024 10:06:07 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Mushahid Hussain
 <hmushi@...zon.co.uk>,  Vitaly Kuznetsov <vkuznets@...hat.com>, Wanpeng Li
 <wanpengli@...cent.com>, Jim Mattson <jmattson@...gle.com>,  Joerg Roedel
 <joro@...tes.org>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Mingwei Zhang <mizhang@...gle.com>, Maxim Levitsky <mlevitsk@...hat.com>
Subject: Re: [PATCH] KVM: Move gfn_to_pfn_cache invalidation to
 invalidate_range_end hook

On Mon, 2024-08-05 at 17:45 -0700, Sean Christopherson wrote:
> On Mon, Aug 05, 2024, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@...zon.co.uk>
> > 
> > The existing retry loop in hva_to_pfn_retry() is extremely pessimistic.
> > If there is an invalidation running concurrently, it is effectively just
> > a complex busy wait loop because its local mmu_notifier_retry_cache()
> > function will always return true.
> > 
> > It ends up functioning as a very unfair read/write lock. If userspace is
> > acting as a 'writer', performing many unrelated MM changes, then the
> > hva_to_pfn_retry() function acting as the 'reader' just backs off and
> > keep retrying for ever, not making any progress.
> > 
> > Solve this by introducing a separate 'validating' flag to the GPC, so
> > that it can be marked invalid before it's even mapped. This allows the
> > invalidation to be moved to the range_end hook, and the retry loop in
> > hva_to_pfn_retry() can be changed to loop only if its particular uHVA
> > has been affected.
> 
> I think I'm missing something.  How does allowing hva_to_pfn_retry() allow KVM
> as a whole to make forward progress?  Doesn't getting past hva_to_pfn_retry()
> just move the problem to kvm_gpc_check()?
> 
> kvm_gpc_refresh() can't be called with gpc->lock held, and nor does it return
> with gpc->lock held, so a racing mmu_notifier invalidation can/will acquire
> gpc->lock and clear gpc->active, no?
> 
> Oh, by "unrelated", you mean _completely_ unrelated?  As in, KVM happens to do a
> refresh when userspace is blasting MADV_DONTNEED, and gets stuck retrying for
> no good reason?

Right. And again, I have no idea why userspace is doing that, and we
need to make it stop. But that's not really the point; the kernel
should be able to make progress anyway.

> Servicing guest pages faults has the same problem, which is why
> mmu_invalidate_retry_gfn() was added.  Supporting hva-only GPCs made our lives a
> little harder, but not horrifically so (there are ordering differences regardless).
> 
> Woefully incomplete, but I think this is the gist of what you want:

Hm, maybe. It does mean that migration occurring all through memory
(indeed, just one at top and bottom of guest memory space) would
perturb GPCs which remain present.

> > @@ -849,6 +837,8 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
> >         wake = !kvm->mn_active_invalidate_count;
> >         spin_unlock(&kvm->mn_invalidate_lock);
> >  
> > +       gfn_to_pfn_cache_invalidate(kvm, range->start, range->end);
> 
> We can't do this.  The contract with mmu_notifiers is that secondary MMUs must
> unmap the hva before returning from invalidate_range_start(), and must not create
> new mappings until invalidate_range_end().

But in the context of the GPC, it is only "mapped" when the ->valid bit is set. 

Even the invalidation callback just clears the valid bit, and that
means nobody is allowed to dereference the ->khva any more. It doesn't
matter that the underlying (stale) PFN is still kmapped.

Can we not apply the same logic to the hva_to_pfn_retry() loop? Yes, it
might kmap a page that gets removed, but it's not actually created a
new mapping if it hasn't set the ->valid bit.

I don't think this version quite meets the constraints, and I might
need to hook *both* the start and end notifiers, and might not like it
once I get there. But I'll have a go...

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)