Commit 1f33dc0c authored by Janusz Krzysztofik's avatar Janusz Krzysztofik Committed by Andi Shyti
Browse files

drm/i915: Remove extra multi-gt pm-references 



There was an attempt to fix an issue of illegal attempts to free a still
active i915 VMA object when parking a GT believed to be idle, reported by
CI on 2-GT Meteor Lake.  As a solution, an extra wakeref for a Primary GT
was acquired from i915_gem_do_execbuffer() -- see commit f56fe3e9
("drm/i915: Fix a VMA UAF for multi-gt platform").

However, that fix occurred insufficient -- the issue was still reported by
CI.  That wakeref was released on exit from i915_gem_do_execbuffer(), then
potentially before completion of the request and deactivation of its
associated VMAs.  Moreover, CI reports indicated that single-GT platforms
also suffered sporadically from the same race.

Since the issue has now been fixed by a preceding patch "drm/i915/vma: Fix
UAF on destroy against retire race", drop the no longer useful changes
introduced by that insufficient fix.

v3: Also drop the no longer used .wakeref_gt0 field from struct
    i915_execbuffer.
v2: Avoid the word "revert" in commit message (Rodrigo),
  - update commit description reusing relevant chunks dropped from the
    description of the proper fix (Rodrigo).

Signed-off-by: default avatarJanusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: default avatarNirmoy Das <nirmoy.das@intel.com>
Signed-off-by: default avatarAndi Shyti <andi.shyti@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240305143747.335367-7-janusz.krzysztofik@linux.intel.com
parent f3c71b2d

drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c

+0 −18
Original line number Diff line number Diff line
@@ -254,7 +254,6 @@ struct i915_execbuffer { 
	struct intel_context *context; /* logical state for the request */

	struct i915_gem_context *gem_context; /** caller's context */

	intel_wakeref_t wakeref;

	intel_wakeref_t wakeref_gt0;



	/** our requests to build */

	struct i915_request *requests[MAX_ENGINE_INSTANCE + 1];
@@ -2685,7 +2684,6 @@ static int 
eb_select_engine(struct i915_execbuffer *eb)

{

	struct intel_context *ce, *child;

	struct intel_gt *gt;

	unsigned int idx;

	int err;
@@ -2709,17 +2707,10 @@ eb_select_engine(struct i915_execbuffer *eb) 
		}

	}

	eb->num_batches = ce->parallel.number_children + 1;

	gt = ce->engine->gt;



	for_each_child(ce, child)

		intel_context_get(child);

	eb->wakeref = intel_gt_pm_get(ce->engine->gt);

	/*

	 * Keep GT0 active on MTL so that i915_vma_parked() doesn't

	 * free VMAs while execbuf ioctl is validating VMAs.

	 */

	if (gt->info.id)

		eb->wakeref_gt0 = intel_gt_pm_get(to_gt(gt->i915));



	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {

		err = intel_context_alloc_state(ce);
@@ -2758,9 +2749,6 @@ eb_select_engine(struct i915_execbuffer *eb) 
	return err;



err:

	if (gt->info.id)

		intel_gt_pm_put(to_gt(gt->i915), eb->wakeref_gt0);



	intel_gt_pm_put(ce->engine->gt, eb->wakeref);

	for_each_child(ce, child)

		intel_context_put(child);
@@ -2774,12 +2762,6 @@ eb_put_engine(struct i915_execbuffer *eb) 
	struct intel_context *child;



	i915_vm_put(eb->context->vm);

	/*

	 * This works in conjunction with eb_select_engine() to prevent

	 * i915_vma_parked() from interfering while execbuf validates vmas.

	 */

	if (eb->gt->info.id)

		intel_gt_pm_put(to_gt(eb->gt->i915), eb->wakeref_gt0);

	intel_gt_pm_put(eb->context->engine->gt, eb->wakeref);

	for_each_child(eb->context, child)

		intel_context_put(child);