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