On 23/08/2018 08:07, Zhenyu Wang wrote:

On 2018.08.22 20:20:46 +0200, Paolo Bonzini wrote:

quoted

On 22/08/2018 18:44, Linus Torvalds wrote:

quoted

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 <redacted>
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@linux-foundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zhi Wang <redacted>
Signed-off-by: Zhenyu Wang <redacted>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

quoted hunk

---
 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);