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

[Tvrtko Ursulin]

On 06/01/2017 16:24, Chris Wilson wrote:
> 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).

Good, just double checking I got everything right.

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

Regards,

Tvrtko