Re: [PATCH] drm/i915/userptr: Probe vma range before gup

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/12/2017 09:27, Chris Wilson wrote:
> We want to exclude any GGTT objects from being present on our internal
> lists to avoid the deadlock we may run into with our requirement for
> struct_mutex during invalidate. However, if the gup_fast fails, we put
> the userptr onto the workqueue and mark it as active, so that we
> remember to serialise the worker upon mmu_invalidate.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 40 +++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 382a77a1097e..562b869dc750 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -598,6 +598,39 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
>   	return ERR_PTR(-EAGAIN);
>   }
>   
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	down_read(&mm->mmap_sem);
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		/*
> +		 * Exclude any VMA that is backed only by struct_page, i.e.
> +		 * IO regions that include our own GGTT mmaps. We cannot handle
> +		 * such ranges, as we may encounter deadlocks around our
> +		 * struct_mutex on mmu_invalidate_range.
> +		 */
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +	up_read(&mm->mmap_sem);
> +
> +	return ret;
> +}
> +
>   static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   {
>   	const int num_pages = obj->base.size >> PAGE_SHIFT;
> @@ -632,9 +665,12 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
>   			return -EAGAIN;
>   	}
>   
> -	pvec = NULL;
> -	pinned = 0;
> +	/* Quickly exclude any invalid VMA */
> +	pinned = probe_range(mm, obj->userptr.ptr, obj->base.size);
> +	if (pinned)
> +		return pinned;
>   
> +	pvec = NULL;
>   	if (mm == current->mm) {
>   		pvec = kvmalloc_array(num_pages, sizeof(struct page *),
>   				      GFP_KERNEL |
> 

Okay as a band-aid, but open to exploitation, which I think was my issue 
last time you posted something similar? Anyways.. it's not worse so 
lesson learnt, of some sort.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx