linux-kernel - Re: [PATCH v3 1/5] KVM: pfncache: Add needs_invalidation flag to gfn_to_pfn

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] [day] [month] [year] [list]

Message-ID: <30CC6DCB-DF32-4FE5-A600-14CE242CBA61@infradead.org>
Date: Thu, 19 Sep 2024 13:13:25 +0200
From: David Woodhouse <dwmw2@...radead.org>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>,
 "Hussain, Mushahid" <hmushi@...zon.co.uk>
CC: 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 v3 1/5] KVM: pfncache: Add needs_invalidation flag to gfn_to_pfn_cache

On 21 August 2024 22:28:09 CEST, David Woodhouse <dwmw2@...radead.org> wrote:
>From: David Woodhouse <dwmw@...zon.co.uk>
>
>This will be used to allow hva_to_pfn_retry() to be more selective about
>its retry loop, which is currently extremely pessimistic.
>
>It allows for invalidations to occur even while the PFN is being mapped
>(which happens with the lock dropped), before the GPC is fully valid.
>
>No functional change yet, as the existing mmu_notifier_retry_cache()
>function will still return true in all cases where the invalidation
>may have triggered.
>
>Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
>---
> include/linux/kvm_types.h |  1 +
> virt/kvm/pfncache.c       | 29 ++++++++++++++++++++++++-----
> 2 files changed, 25 insertions(+), 5 deletions(-)
>
>diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>index 827ecc0b7e10..4d8fbd87c320 100644
>--- a/include/linux/kvm_types.h
>+++ b/include/linux/kvm_types.h
>@@ -69,6 +69,7 @@ struct gfn_to_pfn_cache {
> 	void *khva;
> 	kvm_pfn_t pfn;
> 	bool active;
>+	bool needs_invalidation;
> 	bool valid;
> };
> 
>diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
>index f0039efb9e1e..7007d32d197a 100644
>--- a/virt/kvm/pfncache.c
>+++ b/virt/kvm/pfncache.c
>@@ -32,7 +32,7 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
> 		read_lock_irq(&gpc->lock);
> 
> 		/* Only a single page so no need to care about length */
>-		if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
>+		if (gpc->needs_invalidation && !is_error_noslot_pfn(gpc->pfn) &&
> 		    gpc->uhva >= start && gpc->uhva < end) {
> 			read_unlock_irq(&gpc->lock);
> 
>@@ -45,9 +45,11 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
> 			 */
> 
> 			write_lock_irq(&gpc->lock);
>-			if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
>-			    gpc->uhva >= start && gpc->uhva < end)
>+			if (gpc->needs_invalidation && !is_error_noslot_pfn(gpc->pfn) &&
>+			    gpc->uhva >= start && gpc->uhva < end) {
>+				gpc->needs_invalidation = false;
> 				gpc->valid = false;
>+			}
> 			write_unlock_irq(&gpc->lock);
> 			continue;
> 		}
>@@ -93,6 +95,9 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
> 	if (!gpc->valid)
> 		return false;
> 
>+	/* If it's valid, it needs invalidation! */
>+	WARN_ON_ONCE(!gpc->needs_invalidation);
>+
> 	return true;
> }
> 
>@@ -175,6 +180,17 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
> 		mmu_seq = gpc->kvm->mmu_invalidate_seq;
> 		smp_rmb();
> 
>+		/*
>+		 * The translation made by hva_to_pfn() below could be made
>+		 * invalid as soon as it's mapped. But the uhva is already
>+		 * known and that's all that gfn_to_pfn_cache_invalidate()
>+		 * looks at. So set the 'needs_invalidation' flag to allow
>+		 * the GPC to be marked invalid from the moment the lock is
>+		 * dropped, before the corresponding PFN is even found (and,
>+		 * more to the point, immediately afterwards).
>+		 */
>+		gpc->needs_invalidation = true;
>+
> 		write_unlock_irq(&gpc->lock);
> 
> 		/*
>@@ -224,7 +240,8 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
> 		 * attempting to refresh.
> 		 */
> 		WARN_ON_ONCE(gpc->valid);
>-	} while (mmu_notifier_retry_cache(gpc->kvm, mmu_seq));
>+	} while (!gpc->needs_invalidation ||
>+		 mmu_notifier_retry_cache(gpc->kvm, mmu_seq));
> 
> 	gpc->valid = true;
> 	gpc->pfn = new_pfn;
>@@ -339,6 +356,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
> 	 */
> 	if (ret) {
> 		gpc->valid = false;
>+		gpc->needs_invalidation = false;
> 		gpc->pfn = KVM_PFN_ERR_FAULT;
> 		gpc->khva = NULL;
> 	}
>@@ -383,7 +401,7 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)
> 	gpc->pfn = KVM_PFN_ERR_FAULT;
> 	gpc->gpa = INVALID_GPA;
> 	gpc->uhva = KVM_HVA_ERR_BAD;
>-	gpc->active = gpc->valid = false;
>+	gpc->active = gpc->valid = gpc->needs_invalidation = false;
> }
> 
> static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva,
>@@ -453,6 +471,7 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)
> 		write_lock_irq(&gpc->lock);
> 		gpc->active = false;
> 		gpc->valid = false;
>+		gpc->needs_invalidation = false;
> 
> 		/*
> 		 * Leave the GPA => uHVA cache intact, it's protected by the

Ping?