[Intel-gfx] [PATCH] drm/i915/gem: Safely acquire the ctx->vm when copying

[Niranjan Vishwanathapura]

On Wed, Nov 06, 2019 at 09:13:12AM +0000, Chris Wilson wrote:
>As we read the ctx->vm unlocked before cloning/exporting, we should
>validate our reference is correct before returning it. We already do for
>clone_vm() but were not so strict around get_ppgtt().
>
>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 80 +++++++++++----------
> 1 file changed, 43 insertions(+), 37 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>index de6e55af82cf..a06cc8e63281 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>@@ -995,6 +995,38 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> 	return err;
> }
>
>+static struct i915_address_space *
>+context_get_vm_rcu(struct i915_gem_context *ctx)
>+{
>+	do {
>+		struct i915_address_space *vm;
>+
>+		vm = rcu_dereference(ctx->vm);
>+		if (!kref_get_unless_zero(&vm->ref))
>+			continue;

But should we check for NULL vm?
I know the callers are ensuring ctx->vm will not be NULL, but just wondering.

>+
>+		/*
>+		 * This ppgtt may have be reallocated between
>+		 * the read and the kref, and reassigned to a third
>+		 * context. In order to avoid inadvertent sharing
>+		 * of this ppgtt with that third context (and not
>+		 * src), we have to confirm that we have the same
>+		 * ppgtt after passing through the strong memory
>+		 * barrier implied by a successful
>+		 * kref_get_unless_zero().
>+		 *
>+		 * Once we have acquired the current ppgtt of src,
>+		 * we no longer care if it is released from src, as
>+		 * it cannot be reallocated elsewhere.
>+		 */

Comment should be made generic? It is too specific to cloning case.

Other than that, patch looks good to me.
Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>

>+
>+		if (vm == rcu_access_pointer(ctx->vm))
>+			return rcu_pointer_handoff(vm);
>+
>+		i915_vm_put(vm);
>+	} while (1);
>+}
>+
> static int get_ppgtt(struct drm_i915_file_private *file_priv,
> 		     struct i915_gem_context *ctx,
> 		     struct drm_i915_gem_context_param *args)
>@@ -1006,7 +1038,7 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
> 		return -ENODEV;
>
> 	rcu_read_lock();
>-	vm = i915_vm_get(ctx->vm);
>+	vm = context_get_vm_rcu(ctx);
> 	rcu_read_unlock();
>
> 	ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
>@@ -2035,47 +2067,21 @@ static int clone_vm(struct i915_gem_context *dst,
> 	struct i915_address_space *vm;
> 	int err = 0;
>
>-	rcu_read_lock();
>-	do {
>-		vm = rcu_dereference(src->vm);
>-		if (!vm)
>-			break;
>-
>-		if (!kref_get_unless_zero(&vm->ref))
>-			continue;
>-
>-		/*
>-		 * This ppgtt may have be reallocated between
>-		 * the read and the kref, and reassigned to a third
>-		 * context. In order to avoid inadvertent sharing
>-		 * of this ppgtt with that third context (and not
>-		 * src), we have to confirm that we have the same
>-		 * ppgtt after passing through the strong memory
>-		 * barrier implied by a successful
>-		 * kref_get_unless_zero().
>-		 *
>-		 * Once we have acquired the current ppgtt of src,
>-		 * we no longer care if it is released from src, as
>-		 * it cannot be reallocated elsewhere.
>-		 */
>-
>-		if (vm == rcu_access_pointer(src->vm))
>-			break;
>+	if (!rcu_access_pointer(src->vm))
>+		return 0;
>
>-		i915_vm_put(vm);
>-	} while (1);
>+	rcu_read_lock();
>+	vm = context_get_vm_rcu(src);
> 	rcu_read_unlock();
>
>-	if (vm) {
>-		if (!mutex_lock_interruptible(&dst->mutex)) {
>-			__assign_ppgtt(dst, vm);
>-			mutex_unlock(&dst->mutex);
>-		} else {
>-			err = -EINTR;
>-		}
>-		i915_vm_put(vm);
>+	if (!mutex_lock_interruptible(&dst->mutex)) {
>+		__assign_ppgtt(dst, vm);
>+		mutex_unlock(&dst->mutex);
>+	} else {
>+		err = -EINTR;
> 	}
>
>+	i915_vm_put(vm);
> 	return err;
> }
>
>-- 
>2.24.0
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx