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

[Niranjan Vishwanathapura]

On Thu, Nov 07, 2019 at 05:01:14PM +0000, Chris Wilson wrote:
>Quoting Niranjan Vishwanathapura (2019-11-07 16:09:31)
>> 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.
>
>We don't need to as the rule is that ctx->vm once set can never be
>unset; and it can only be set during construction based on the HW
>properties. The idea is that !!ctx->vm is an invariant indicating
>whether or not we have full-ppgtt enabled. From a security perspective,
>allowing a downgrade from full-ppgtt to a shared global gtt is a big no,
>so I don't anticipating us allowing setting ctx->vm to NULL anytime in
>the near future :)

OK, sounds right.

>
>> >+
>> >+              /*
>> >+               * 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.
>
>s/src/ctx/
>-Chris