Possible use_mm() mis-uses

[Paolo Bonzini]

On 23/08/2018 08:07, Zhenyu Wang wrote:
> 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>

Reviewed-by: Paolo Bonzini <pbonzini at redhat.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);
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180823/a6102d5b/attachment.sig>