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@xxxxxxxxxxxxxxxxxx>
> >---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
