| Message ID | 20240305143747.335367-7-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix VMA UAF on destroy against deactivate race | expand |
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@linux.intel.com> > Cc: Nirmoy Das<nirmoy.das@intel.com> > Cc: Rodrigo Vivi<rodrigo.vivi@intel.com> Reviewed-by: Nirmoy Das <nirmoy.das@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);
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);
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@linux.intel.com> Cc: Nirmoy Das <nirmoy.das@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 18 ------------------ 1 file changed, 18 deletions(-)