[Intel-gfx] [PATCH 1/3] drm/i915: Fix phys pwrite for struct_mutex-less operation

[Tvrtko Ursulin]

On 06/01/2017 15:22, Chris Wilson wrote:
> Since commit fe115628d567 ("drm/i915: Implement pwrite without
> struct-mutex") the lowlevel pwrite calls are now called without the
> protection of struct_mutex, but pwrite_phys was still asserting that it
> held the struct_mutex and later tried to drop and relock it.
>
> Fixes: fe115628d567 ("drm/i915: Implement pwrite without struct-mutex")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: <drm-intel-fixes at lists.freedesktop.org>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 34 ++++------------------------------
>  1 file changed, 4 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 91072ebdbdcb..94958437252a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -597,47 +597,21 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>  		     struct drm_i915_gem_pwrite *args,
>  		     struct drm_file *file)
>  {
> -	struct drm_device *dev = obj->base.dev;
>  	void *vaddr = obj->phys_handle->vaddr + args->offset;
>  	char __user *user_data = u64_to_user_ptr(args->data_ptr);
> -	int ret;
>
>  	/* We manually control the domain here and pretend that it
>  	 * remains coherent i.e. in the GTT domain, like shmem_pwrite.
>  	 */
> -	lockdep_assert_held(&obj->base.dev->struct_mutex);
> -	ret = i915_gem_object_wait(obj,
> -				   I915_WAIT_INTERRUPTIBLE |
> -				   I915_WAIT_LOCKED |
> -				   I915_WAIT_ALL,
> -				   MAX_SCHEDULE_TIMEOUT,
> -				   to_rps_client(file));
> -	if (ret)
> -		return ret;
> -
>  	intel_fb_obj_invalidate(obj, ORIGIN_CPU);
> -	if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
> -		unsigned long unwritten;
> -
> -		/* The physical object once assigned is fixed for the lifetime
> -		 * of the obj, so we can safely drop the lock and continue
> -		 * to access vaddr.
> -		 */
> -		mutex_unlock(&dev->struct_mutex);
> -		unwritten = copy_from_user(vaddr, user_data, args->size);
> -		mutex_lock(&dev->struct_mutex);
> -		if (unwritten) {
> -			ret = -EFAULT;
> -			goto out;
> -		}
> -	}
> +	if (copy_from_user(vaddr, user_data, args->size))
> +		return -EFAULT;
>
>  	drm_clflush_virt_range(vaddr, args->size);
> -	i915_gem_chipset_flush(to_i915(dev));
> +	i915_gem_chipset_flush(to_i915(obj->base.dev));
>
> -out:
>  	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> -	return ret;
> +	return 0;
>  }
>
>  void *i915_gem_object_alloc(struct drm_i915_private *dev_priv)
>

First time round I missed how obviously broken the copy was and that 
wait is already done one level up. What I missed from the original work 
is, now it is quite possible for userspace to see/create corruption if 
it doesn't serialize rendering and pread/pwrite itself. Is that OK?

Regards.

Tvrtko