From patchwork Tue Feb 27 11:49:18 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: David Woodhouse X-Patchwork-Id: 13573723 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 818F2145B07 for ; Tue, 27 Feb 2024 12:19:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709036358; cv=none; b=AnKMtpDdqP/d44+7e8aTDn3aNlpEl0Nyo1ZMP37ZmtYhHRdV0u/+28l8854HBXtuM52CZaG23mhSvCMdG5zee3yolXXctvM7Af+VevyjTeYXAht9hUQ/NN7UdfTw8+vN9WK3ZfAtNUBK2DKJRaYr4qwyrBsV/pvaRE+qfoRAKmw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709036358; c=relaxed/simple; bh=zUP4W9ocOf2QlPtwjYEiJOKLkNkU9oBTZFc27GWQhAQ=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=e1gC0Jv1QjPg8aPXAYFLCMvUYsN2e1zmFzPY7XVFQdkv0ywOQzHX2DFP0BZ0VV0q/eh7Cd2GGfLm28ORyzPuREQwAf15hPnhmFrMqD/KrbI+ItcIHLT+lbXQki86g4O0qMLqjOENFwGvPixJfdPmcqYjVOXV+kyGmuackYisBL8= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org; spf=none smtp.mailfrom=casper.srs.infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=R4erJRk7; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=casper.srs.infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="R4erJRk7" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:Cc: To:From:Reply-To:Content-ID:Content-Description; bh=vFgdx1YhFNLyaO06G93phjZWUsQaEpwpJ51a+zXlhfs=; b=R4erJRk7sHah8+pqXn+YrNJ9/p 0TFXmU+/V6f6a6CHq38u/q7DJ6VsHfetogQoCuE+cXRgFouxZnouzm56cN2cIYAP9lvxu5p6BauGk fVzpZd0QJIwh/vuptIoclXzfQZwabWbf3XqhRZcnmq+7gDSC1xcXU6yG/2TYmWjxzUTkCNuCE28cp BKEGA0ja53pjTIwquIorx3udMESdGKdetLQVwI78V+/4O13iGcsUwdvU8gErHImi2mIRQQN17ehGx EqtiPha5T/+qHabFtIiDRFqF/xucaO3Jsz6xenjyNBT0Q6mcpBAkZgl5mXwKJUxEdbe9kgPIUGhOQ 0JJmmAwg==; Received: from [2001:8b0:10b:1::ebe] (helo=i7.infradead.org) by casper.infradead.org with esmtpsa (Exim 4.97.1 #2 (Red Hat Linux)) id 1rew4p-00000002JfM-3LC1; Tue, 27 Feb 2024 11:56:51 +0000 Received: from dwoodhou by i7.infradead.org with local (Exim 4.97.1 #2 (Red Hat Linux)) id 1rew4n-000000000wX-4409; Tue, 27 Feb 2024 11:56:49 +0000 From: David Woodhouse To: kvm@vger.kernel.org Cc: Sean Christopherson , Paul Durrant , Paolo Bonzini , Michal Luczaj , David Woodhouse Subject: [PATCH v2 4/8] KVM: pfncache: simplify locking and make more self-contained Date: Tue, 27 Feb 2024 11:49:18 +0000 Message-ID: <20240227115648.3104-5-dwmw2@infradead.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240227115648.3104-1-dwmw2@infradead.org> References: <20240227115648.3104-1-dwmw2@infradead.org> Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Sender: David Woodhouse X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org. See http://www.infradead.org/rpr.html From: David Woodhouse The locking on the gfn_to_pfn_cache is... interesting. And awful. There is a rwlock in ->lock which readers take to ensure protection against concurrent changes. But __kvm_gpc_refresh() makes assumptions that certain fields will not change even while it drops the write lock and performs MM operations to revalidate the target PFN and kernel mapping. Commit 93984f19e7bc ("KVM: Fully serialize gfn=>pfn cache refresh via mutex") partly addressed that — not by fixing it, but by adding a new mutex, ->refresh_lock. This prevented concurrent __kvm_gpc_refresh() calls on a given gfn_to_pfn_cache, but is still only a partial solution. There is still a theoretical race where __kvm_gpc_refresh() runs in parallel with kvm_gpc_deactivate(). While __kvm_gpc_refresh() has dropped the write lock, kvm_gpc_deactivate() clears the ->active flag and unmaps ->khva. Then __kvm_gpc_refresh() determines that the previous ->pfn and ->khva are still valid, and reinstalls those values into the structure. This leaves the gfn_to_pfn_cache with the ->valid bit set, but ->active clear. And a ->khva which looks like a reasonable kernel address but is actually unmapped. All it takes is a subsequent reactivation to cause that ->khva to be dereferenced. This would theoretically cause an oops which would look something like this: [1724749.564994] BUG: unable to handle page fault for address: ffffaa3540ace0e0 [1724749.565039] RIP: 0010:__kvm_xen_has_interrupt+0x8b/0xb0 I say "theoretically" because theoretically, that oops that was seen in production cannot happen. The code which uses the gfn_to_pfn_cache is supposed to have its *own* locking, to further paper over the fact that the gfn_to_pfn_cache's own papering-over (->refresh_lock) of its own rwlock abuse is not sufficient. For the Xen vcpu_info that external lock is the vcpu->mutex, and for the shared info it's kvm->arch.xen.xen_lock. Those locks ought to protect the gfn_to_pfn_cache against concurrent deactivation vs. refresh in all but the cases where the vcpu or kvm object is being *destroyed*, in which case the subsequent reactivation should never happen. Theoretically. Nevertheless, this locking abuse is awful and should be fixed, even if no clear explanation can be found for how the oops happened. So expand the use of the ->refresh_lock mutex to ensure serialization of activate/deactivate vs. refresh and make the pfncache locking entirely self-sufficient. This means that a future commit can simplify the locking in the callers, such as the Xen emulation code which has an outstanding problem with recursive locking of kvm->arch.xen.xen_lock, which will no longer be necessary. The rwlock abuse described above is still not best practice, although it's harmless now that the ->refresh_lock is held for the entire duration while the offending code drops the write lock, does some other stuff, then takes the write lock again and assumes nothing changed. That can also be fixed^W cleaned up in a subsequent commit, but this commit is a simpler basis for the Xen deadlock fix mentioned above. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Signed-off-by: Sean Christopherson --- virt/kvm/pfncache.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 9ac8c9da4eda..43d67f8f064e 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -256,12 +256,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l if (page_offset + len > PAGE_SIZE) return -EINVAL; - /* - * If another task is refreshing the cache, wait for it to complete. - * There is no guarantee that concurrent refreshes will see the same - * gpa, memslots generation, etc..., so they must be fully serialized. - */ - mutex_lock(&gpc->refresh_lock); + lockdep_assert_held(&gpc->refresh_lock); write_lock_irq(&gpc->lock); @@ -347,8 +342,6 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l out_unlock: write_unlock_irq(&gpc->lock); - mutex_unlock(&gpc->refresh_lock); - if (unmap_old) gpc_unmap(old_pfn, old_khva); @@ -357,15 +350,23 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len) { + unsigned long uhva; + int ret; + + mutex_lock(&gpc->refresh_lock); + /* * If the GPA is valid then ignore the HVA, as a cache can be GPA-based * or HVA-based, not both. For GPA-based caches, the HVA will be * recomputed during refresh if necessary. */ - unsigned long uhva = kvm_is_error_gpa(gpc->gpa) ? gpc->uhva : - KVM_HVA_ERR_BAD; + uhva = kvm_is_error_gpa(gpc->gpa) ? gpc->uhva : KVM_HVA_ERR_BAD; + + ret = __kvm_gpc_refresh(gpc, gpc->gpa, uhva, len); - return __kvm_gpc_refresh(gpc, gpc->gpa, uhva, len); + mutex_unlock(&gpc->refresh_lock); + + return ret; } void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm) @@ -377,12 +378,16 @@ 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; } static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva, unsigned long len) { struct kvm *kvm = gpc->kvm; + int ret; + + mutex_lock(&gpc->refresh_lock); if (!gpc->active) { if (KVM_BUG_ON(gpc->valid, kvm)) @@ -401,7 +406,10 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned gpc->active = true; write_unlock_irq(&gpc->lock); } - return __kvm_gpc_refresh(gpc, gpa, uhva, len); + ret = __kvm_gpc_refresh(gpc, gpa, uhva, len); + mutex_unlock(&gpc->refresh_lock); + + return ret; } int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len) @@ -420,6 +428,7 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc) kvm_pfn_t old_pfn; void *old_khva; + mutex_lock(&gpc->refresh_lock); if (gpc->active) { /* * Deactivate the cache before removing it from the list, KVM @@ -449,4 +458,5 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc) gpc_unmap(old_pfn, old_khva); } + mutex_unlock(&gpc->refresh_lock); }