Possible use_mm() mis-uses

[Zhenyu Wang]

On 2018.08.22 20:20:46 +0200, Paolo Bonzini wrote:
> On 22/08/2018 18:44, Linus Torvalds wrote:
> > An example of something that *isn't* right, is the i915 kvm interface,
> > which does
> > 
> >         use_mm(kvm->mm);
> > 
> > on an mm that was initialized in virt/kvm/kvm_main.c using
> > 
> >         mmgrab(current->mm);
> >         kvm->mm = current->mm;
> > 
> > which is *not* right. Using "mmgrab()" does indeed guarantee the
> > lifetime of the 'struct mm_struct' itself, but it does *not* guarantee
> > the lifetime of the page tables. You need to use "mmget()" and
> > "mmput()", which get the reference to the actual process address
> > space!
> > 
> > Now, it is *possible* that the kvm use is correct too, because kvm
> > does register a mmu_notifier chain, and in theory you can avoid the
> > proper refcounting by just making sure the mmu "release" notifier
> > kills any existing uses, but I don't really see kvm doing that. Kvm
> > does register a release notifier, but that just flushes the shadow
> > page tables, it doesn't kill any use_mm() use by some i915 use case.
> 
> Yes, KVM is correct but the i915 bits are at least fishy.  It's probably
> as simple as adding a mmget/mmput pair respectively in kvmgt_guest_init
> and kvmgt_guest_exit, or maybe mmget_not_zero.
> 

yeah, that's the clear way to fix this imo. We only depend on guest
life cycle to access guest memory properly. Here's proposed fix, will
verify and integrate it later.

Thanks!

From 5e5a8d0409aa150884adf5a4d0b956fd0b9906b3 Mon Sep 17 00:00:00 2001
From: Zhenyu Wang <zhenyuw at linux.intel.com>
Date: Thu, 23 Aug 2018 14:08:06 +0800
Subject: [PATCH] drm/i915/gvt: Fix life cycle reference on KVM mm

Handle guest mm access life cycle properly with mmget()/mmput()
through guest init()/exit(). As noted by Linus, use_mm() depends
on valid live page table but KVM's mmgrab() doesn't guarantee that.
As vGPU usage depends on guest VM life cycle, need to make sure to
use mmget()/mmput() to guarantee VM address access.

Cc: Linus Torvalds <torvalds at linux-foundation.org>
Cc: Paolo Bonzini <pbonzini at redhat.com>
Cc: Zhi Wang <zhi.a.wang at intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw at linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 71751be329e3..4a0988747d08 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -32,6 +32,7 @@
 #include <linux/device.h>
 #include <linux/mm.h>
 #include <linux/mmu_context.h>
+#include <linux/sched/mm.h>
 #include <linux/types.h>
 #include <linux/list.h>
 #include <linux/rbtree.h>
@@ -1614,9 +1615,16 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
 	if (__kvmgt_vgpu_exist(vgpu, kvm))
 		return -EEXIST;
 
+	if (!mmget_not_zero(kvm->mm)) {
+		gvt_vgpu_err("Can't get KVM mm reference\n");
+		return -EINVAL;
+	}
+
 	info = vzalloc(sizeof(struct kvmgt_guest_info));
-	if (!info)
+	if (!info) {
+		mmput(kvm->mm);
 		return -ENOMEM;
+	}
 
 	vgpu->handle = (unsigned long)info;
 	info->vgpu = vgpu;
@@ -1647,6 +1655,8 @@ static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
 	debugfs_remove(info->debugfs_cache_entries);
 
 	kvm_page_track_unregister_notifier(info->kvm, &info->track_node);
+	if (info->kvm->mm)
+		mmput(info->kvm->mm);
 	kvm_put_kvm(info->kvm);
 	kvmgt_protect_table_destroy(info);
 	gvt_cache_destroy(info->vgpu);
-- 
2.18.0


-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180823/5c0a7a24/attachment.sig>