[PATCH v7 2/3] drm/i915: Remove extra multi-gt pm-references

[Nirmoy Das]

On 3/5/2024 3:35 PM, Janusz Krzysztofik wrote:
> 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 f56fe3e91787
> ("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: Janusz Krzysztofik<janusz.krzysztofik at linux.intel.com>
> Cc: Nirmoy Das<nirmoy.das at intel.com>
> Cc: Rodrigo Vivi<rodrigo.vivi at intel.com>

Reviewed-by: Nirmoy Das <nirmoy.das at intel.com>

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 18 ------------------
>   1 file changed, 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index d3a771afb083e..3f20fe3811999 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -255,7 +255,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];
> @@ -2686,7 +2685,6 @@ static int
>   eb_select_engine(struct i915_execbuffer *eb)
>   {
>   	struct intel_context *ce, *child;
> -	struct intel_gt *gt;
>   	unsigned int idx;
>   	int err;
>   
> @@ -2710,17 +2708,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);
> @@ -2759,9 +2750,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);
> @@ -2775,12 +2763,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);
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20240305/15d07f40/attachment.htm>