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

[Chris Wilson]

On Fri, Jan 06, 2017 at 04:04:44PM +0000, Tvrtko Ursulin wrote:
> 
> 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?

Yes. We take the general view that we cannot prevent userspace from
racing with itself. Consider not just concurrent pwrites, but vs GTT,
CPU, wC mmaps and execbuf.

Starting from scratch, we would fence everything (including CPU access)
and have no implicit syncs and rely on userspace using the fences
correctly (if they wish to avoid seeing corruption).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre