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

[Chris Wilson]

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 :)

> >+
> >+              /*
> >+               * 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