Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	 Zhenyu Wang <zhenyuw@linux.intel.com>,
	Zhi Wang <zhi.a.wang@intel.com>
Cc: Yan Zhao <yan.y.zhao@intel.com>,
	kvm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, Ben Gardon <bgardon@google.com>,
	intel-gvt-dev@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH 09/27] drm/i915/gvt: Protect gfn hash table with dedicated mutex
Date: Fri, 23 Dec 2022 00:57:21 +0000	[thread overview]
Message-ID: <20221223005739.1295925-10-seanjc@google.com> (raw)
In-Reply-To: <20221223005739.1295925-1-seanjc@google.com>

Add and use a new mutex, gfn_lock, to protect accesses to the hash table
used to track which gfns are write-protected when shadowing the guest's
GTT.  This fixes a bug where kvmgt_page_track_write(), which doesn't hold
kvm->mmu_lock, could race with intel_gvt_page_track_remove() and trigger
a use-after-free.

Fixing kvmgt_page_track_write() by taking kvm->mmu_lock is not an option
as mmu_lock is a r/w spinlock, and intel_vgpu_page_track_handler() might
sleep when acquiring vgpu->cache_lock deep down the callstack:

  intel_vgpu_page_track_handler()
  |
  |->  page_track->handler / ppgtt_write_protection_handler()
       |
       |-> ppgtt_handle_guest_write_page_table_bytes()
           |
           |->  ppgtt_handle_guest_write_page_table()
                |
                |-> ppgtt_handle_guest_entry_removal()
                    |
                    |-> ppgtt_invalidate_pte()
                        |
                        |-> intel_gvt_dma_unmap_guest_page()
                            |
                            |-> mutex_lock(&vgpu->cache_lock);

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/gpu/drm/i915/gvt/gvt.h   |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c | 65 ++++++++++++++++++++------------
 drivers/gpu/drm/i915/gvt/vgpu.c  |  1 +
 3 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index dbf8d7470b2c..fbfd7eafec14 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -176,6 +176,7 @@ struct intel_vgpu {
 	struct vfio_device vfio_device;
 	struct intel_gvt *gvt;
 	struct mutex vgpu_lock;
+	struct mutex gfn_lock;
 	int id;
 	bool active;
 	bool attached;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index ca9926061cd8..a4747e153dad 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -366,6 +366,8 @@ __kvmgt_protect_table_find(struct intel_vgpu *info, gfn_t gfn)
 {
 	struct kvmgt_pgfn *p, *res = NULL;
 
+	lockdep_assert_held(&info->gfn_lock);
+
 	hash_for_each_possible(info->ptable, p, hnode, gfn) {
 		if (gfn == p->gfn) {
 			res = p;
@@ -388,6 +390,8 @@ static void kvmgt_protect_table_add(struct intel_vgpu *info, gfn_t gfn)
 {
 	struct kvmgt_pgfn *p;
 
+	lockdep_assert_held(&info->gfn_lock);
+
 	if (kvmgt_gfn_is_write_protected(info, gfn))
 		return;
 
@@ -1563,60 +1567,68 @@ int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn)
 {
 	struct kvm *kvm = info->vfio_device.kvm;
 	struct kvm_memory_slot *slot;
-	int idx;
+	int idx, ret = 0;
 
 	if (!info->attached)
 		return -ESRCH;
 
+	mutex_lock(&info->gfn_lock);
+
+	if (kvmgt_gfn_is_write_protected(info, gfn))
+		goto out;
+
 	idx = srcu_read_lock(&kvm->srcu);
 	slot = gfn_to_memslot(kvm, gfn);
 	if (!slot) {
 		srcu_read_unlock(&kvm->srcu, idx);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	write_lock(&kvm->mmu_lock);
-
-	if (kvmgt_gfn_is_write_protected(info, gfn))
-		goto out;
-
 	kvm_slot_page_track_add_page(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE);
+	write_unlock(&kvm->mmu_lock);
+
+	srcu_read_unlock(&kvm->srcu, idx);
+
 	kvmgt_protect_table_add(info, gfn);
-
 out:
-	write_unlock(&kvm->mmu_lock);
-	srcu_read_unlock(&kvm->srcu, idx);
-	return 0;
+	mutex_unlock(&info->gfn_lock);
+	return ret;
 }
 
 int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn)
 {
 	struct kvm *kvm = info->vfio_device.kvm;
 	struct kvm_memory_slot *slot;
-	int idx;
+	int idx, ret = 0;
 
 	if (!info->attached)
 		return 0;
 
-	idx = srcu_read_lock(&kvm->srcu);
-	slot = gfn_to_memslot(kvm, gfn);
-	if (!slot) {
-		srcu_read_unlock(&kvm->srcu, idx);
-		return -EINVAL;
-	}
-
-	write_lock(&kvm->mmu_lock);
+	mutex_lock(&info->gfn_lock);
 
 	if (!kvmgt_gfn_is_write_protected(info, gfn))
 		goto out;
 
+	idx = srcu_read_lock(&kvm->srcu);
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		srcu_read_unlock(&kvm->srcu, idx);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	write_lock(&kvm->mmu_lock);
 	kvm_slot_page_track_remove_page(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE);
+	write_unlock(&kvm->mmu_lock);
+	srcu_read_unlock(&kvm->srcu, idx);
+
 	kvmgt_protect_table_del(info, gfn);
 
 out:
-	write_unlock(&kvm->mmu_lock);
-	srcu_read_unlock(&kvm->srcu, idx);
-	return 0;
+	mutex_unlock(&info->gfn_lock);
+	return ret;
 }
 
 static void kvmgt_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa,
@@ -1627,11 +1639,13 @@ static void kvmgt_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		container_of(node, struct intel_vgpu, track_node);
 
 	mutex_lock(&info->vgpu_lock);
+	mutex_lock(&info->gfn_lock);
 
 	if (kvmgt_gfn_is_write_protected(info, gpa_to_gfn(gpa)))
 		intel_vgpu_page_track_handler(info, gpa,
 						     (void *)val, len);
 
+	mutex_unlock(&info->gfn_lock);
 	mutex_unlock(&info->vgpu_lock);
 }
 
@@ -1644,16 +1658,19 @@ static void kvmgt_page_track_flush_slot(struct kvm *kvm,
 	struct intel_vgpu *info =
 		container_of(node, struct intel_vgpu, track_node);
 
-	write_lock(&kvm->mmu_lock);
+	mutex_lock(&info->gfn_lock);
 	for (i = 0; i < slot->npages; i++) {
 		gfn = slot->base_gfn + i;
 		if (kvmgt_gfn_is_write_protected(info, gfn)) {
+			write_lock(&kvm->mmu_lock);
 			kvm_slot_page_track_remove_page(kvm, slot, gfn,
 						KVM_PAGE_TRACK_WRITE);
+			write_unlock(&kvm->mmu_lock);
+
 			kvmgt_protect_table_del(info, gfn);
 		}
 	}
-	write_unlock(&kvm->mmu_lock);
+	mutex_unlock(&info->gfn_lock);
 }
 
 void intel_vgpu_detach_regions(struct intel_vgpu *vgpu)
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index 56c71474008a..f2479781b770 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -277,6 +277,7 @@ struct intel_vgpu *intel_gvt_create_idle_vgpu(struct intel_gvt *gvt)
 	vgpu->id = IDLE_VGPU_IDR;
 	vgpu->gvt = gvt;
 	mutex_init(&vgpu->vgpu_lock);
+	mutex_init(&vgpu->gfn_lock);
 
 	for (i = 0; i < I915_NUM_ENGINES; i++)
 		INIT_LIST_HEAD(&vgpu->submission.workload_q_head[i]);
-- 
2.39.0.314.g84b9a713c41-goog


  parent reply	other threads:[~2022-12-23  0:58 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-23  0:57 [Intel-gfx] [PATCH 00/27] drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 01/27] drm/i915/gvt: Verify pfn is "valid" before dereferencing "struct page" Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 02/27] KVM: x86/mmu: Factor out helper to get max mapping size of a memslot Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 03/27] drm/i915/gvt: Incorporate KVM memslot info into check for 2MiB GTT entry Sean Christopherson
2022-12-28  5:42   ` Yan Zhao
2023-01-03 21:13     ` Sean Christopherson
2023-01-05  3:07       ` Yan Zhao
2023-01-05 17:40         ` Sean Christopherson
2023-01-06  5:56           ` Yan Zhao
2023-01-06 23:01             ` Sean Christopherson
2023-01-09  9:58               ` Yan Zhao
2023-01-11 17:55                 ` Sean Christopherson
2023-01-19  2:58                   ` Zhenyu Wang
2023-01-19  5:26                     ` Yan Zhao
2023-02-23 20:41                       ` Sean Christopherson
2023-02-24  5:09                         ` Yan Zhao
2023-01-12  8:31         ` Yan Zhao
2022-12-23  0:57 ` [Intel-gfx] [PATCH 04/27] drm/i915/gvt: Verify VFIO-pinned page is THP when shadowing 2M gtt entry Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 05/27] drm/i915/gvt: Put the page reference obtained by KVM's gfn_to_pfn() Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 06/27] drm/i915/gvt: Don't rely on KVM's gfn_to_pfn() to query possible 2M GTT Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 07/27] drm/i915/gvt: Use an "unsigned long" to iterate over memslot gfns Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 08/27] drm/i915/gvt: Hoist acquisition of vgpu_lock out to kvmgt_page_track_write() Sean Christopherson
2022-12-23  0:57 ` Sean Christopherson [this message]
2022-12-28  5:03   ` [Intel-gfx] [PATCH 09/27] drm/i915/gvt: Protect gfn hash table with dedicated mutex Yan Zhao
2023-01-03 20:43     ` Sean Christopherson
2023-01-05  0:51       ` Yan Zhao
2022-12-23  0:57 ` [Intel-gfx] [PATCH 10/27] KVM: x86/mmu: Don't rely on page-track mechanism to flush on memslot change Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 11/27] KVM: x86/mmu: Don't bounce through page-track mechanism for guest PTEs Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 12/27] KVM: drm/i915/gvt: Drop @vcpu from KVM's ->track_write() hook Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 13/27] KVM: x86: Reject memslot MOVE operations if KVMGT is attached Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 14/27] drm/i915/gvt: Don't bother removing write-protection on to-be-deleted slot Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 15/27] KVM: x86: Add a new page-track hook to handle memslot deletion Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 16/27] drm/i915/gvt: switch from ->track_flush_slot() to ->track_remove_region() Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 17/27] KVM: x86: Remove the unused page-track hook track_flush_slot() Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 18/27] KVM: x86/mmu: Move KVM-only page-track declarations to internal header Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 19/27] KVM: x86/mmu: Use page-track notifiers iff there are external users Sean Christopherson
2022-12-28  6:56   ` Yan Zhao
2023-01-04  0:50     ` Sean Christopherson
2023-08-07 12:01   ` Like Xu
2023-08-07 17:19     ` Sean Christopherson
2023-08-09  1:02       ` Yan Zhao
2023-08-09 14:33         ` Sean Christopherson
2023-08-09 23:21           ` Yan Zhao
2023-08-10  3:02             ` Yan Zhao
2023-08-10 15:41               ` Sean Christopherson
2023-08-11  5:57                 ` Yan Zhao
2022-12-23  0:57 ` [Intel-gfx] [PATCH 20/27] KVM: x86/mmu: Drop infrastructure for multiple page-track modes Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 21/27] KVM: x86/mmu: Rename page-track APIs to reflect the new reality Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 22/27] KVM: x86/mmu: Assert that correct locks are held for page write-tracking Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 23/27] KVM: x86/mmu: Bug the VM if write-tracking is used but not enabled Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 24/27] KVM: x86/mmu: Drop @slot param from exported/external page-track APIs Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 25/27] KVM: x86/mmu: Handle KVM bookkeeping in page-track APIs, not callers Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 26/27] KVM: x86/mmu: Add page-track API to query if a gfn is valid Sean Christopherson
2022-12-28  7:57   ` Yan Zhao
2023-01-03 21:19     ` Sean Christopherson
2023-01-05  3:12       ` Yan Zhao
2023-01-05 17:53         ` Sean Christopherson
2022-12-23  0:57 ` [Intel-gfx] [PATCH 27/27] drm/i915/gvt: Drop final dependencies on KVM internal details Sean Christopherson
2022-12-23  1:28 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups Patchwork
2022-12-23  9:05 ` [Intel-gfx] [PATCH 00/27] " Yan Zhao
2023-01-04  1:01   ` Sean Christopherson
2023-01-05  3:13     ` Yan Zhao
2022-12-28  5:28 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups (rev2) Patchwork
2023-01-06  6:25 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups (rev3) Patchwork
2023-01-19  9:01 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups (rev4) Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221223005739.1295925-10-seanjc@google.com \
    --to=seanjc@google.com \
    --cc=bgardon@google.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=intel-gvt-dev@lists.freedesktop.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=yan.y.zhao@intel.com \
    --cc=zhenyuw@linux.intel.com \
    --cc=zhi.a.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox