[PATCH] drm/i915/vma: Fix potential UAF on multi-tile platforms
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Wed Nov 15 13:21:21 UTC 2023
Object debugging tools were sporadically reporting illegal attempts to
free a still active i915 VMA object from when parking a GPU tile believed
to be idle.
[161.359441] ODEBUG: free active (active state 0) object: ffff88811643b958 object type: i915_active hint: __i915_vma_active+0x0/0x50 [i915]
[161.360082] WARNING: CPU: 5 PID: 276 at lib/debugobjects.c:514 debug_print_object+0x80/0xb0
...
[161.360304] CPU: 5 PID: 276 Comm: kworker/5:2 Not tainted 6.5.0-rc1-CI_DRM_13375-g003f860e5577+ #1
[161.360314] Hardware name: Intel Corporation Rocket Lake Client Platform/RocketLake S UDIMM 6L RVP, BIOS RKLSFWI1.R00.3173.A03.2204210138 04/21/2022
[161.360322] Workqueue: i915-unordered __intel_wakeref_put_work [i915]
[161.360592] RIP: 0010:debug_print_object+0x80/0xb0
...
[161.361347] debug_object_free+0xeb/0x110
[161.361362] i915_active_fini+0x14/0x130 [i915]
[161.361866] release_references+0xfe/0x1f0 [i915]
[161.362543] i915_vma_parked+0x1db/0x380 [i915]
[161.363129] __gt_park+0x121/0x230 [i915]
[161.363515] ____intel_wakeref_put_last+0x1f/0x70 [i915]
That has been tracked down to be happening when another thread was
deactivating the VMA inside __active_retire() helper, after the VMA's
active counter was already decremented to 0, but before deactivation of
the VMA's object was reported to the object debugging tools. Furthermore,
premature release of last wakeref for the GPU tile to which the active VMA
belonged has been identified as a root cause.
A VMA associated with a request doesn't acquire a GT wakeref by itself,
though it may be destroyed from __gt_park() called on last release of the
GT wakeref. Instead, it depends on a wakeref held directly by the
request's active intel_context for a GT associated with its VM, and
indirectly on that intel_context engine's wakeref if from the same GT. In
case of single-tile platforms, at least one of those wakerefs is usually
held long enough for the request's VMA to be deactivated on time, before
it is destroyed on last put of its VM GT's wakeref. However, on multi-
tile platforms, a request may use a VMA from a tile other than the one
that hosts the request's engine, then it is protected only with the
intel_context VM GT's wakeref.
There was an attempt to fix this issue on 2-tile Meteor Lake by acquiring
an extra wakeref for a Primary GT from i915_gem_do_execbuffer() -- see
commit f56fe3e91787 ("drm/i915: Fix a VMA UAF for multi-gt platform").
However, it occurred not sufficient -- the issue was still reported by CI,
most probably because that wakeref was then released on exit from that
function, potentially before completion of the request, then before
deactivation of its associated VMAs.
Moreover, CI reports indicate that single-tile platforms also suffer
sporadically from the same race. Based on analysis of the driver code and
history of its changes, an attempt to address a not clearly disclosed but
potentially the same issue was identified under commit 2bf541ff6d06
("drm/i915: Pin engine before pinning all objects, v5."). That commit
added an extra call to intel_gt_pm_get(ce->engine->gt) from inside
i915_gem_do_execbuffer(), then it was somehow similar to the fix attempt
described above, only for a GT that hosted the engine. However, all
my attempts to reproduce the race on single-tile have failed, then exact
scenario of the race and a commit to be blame for it haven't been
discovered yet. I'm submitting this patch as a locally tested fix for
multi-tile, in hope it also addresses single-tile cases.
Fix the issue by getting a wakeref for the VMA's tile when activating it,
and putting that wakeref only after the VMA is deactivated. However,
exclude global GTT from that processing path, otherwise the GPU never goes
idle. Since __i915_vma_retire() may be called from atomic contexts, use
async variant of wakeref put.
Having that fixed, stop explicitly acquiring the extra GT0 wakeref from
inside i915_gem_do_execbuffer(), and also drop an extra call to
i915_active_wait() introduced by commit 7a2280e8dcd2 ("drm/i915: Wait for
active retire before i915_active_fini()") as another insufficient fix for
this UAF race issue.
v3: Identify and drop former workarounds, update commit description.
v2: Get the wakeref before VM mutex to avoid circular locking dependency,
- drop questionable Fixes: tag.
Closes: https://gitlab.freedesktop.org/drm/intel/issues/8875
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 21 ++------------
drivers/gpu/drm/i915/i915_vma.c | 28 +++++++++++++------
2 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 45b9d9e34b8b8..e0c3eaf316e9e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2691,7 +2691,6 @@ static int
eb_select_engine(struct i915_execbuffer *eb)
{
struct intel_context *ce, *child;
- struct intel_gt *gt;
unsigned int idx;
int err;
@@ -2715,17 +2714,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);
- intel_gt_pm_get(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)
- intel_gt_pm_get(to_gt(gt->i915));
+ intel_gt_pm_get(ce->engine->gt);
if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
err = intel_context_alloc_state(ce);
@@ -2764,10 +2756,7 @@ eb_select_engine(struct i915_execbuffer *eb)
return err;
err:
- if (gt->info.id)
- intel_gt_pm_put(to_gt(gt->i915));
-
- intel_gt_pm_put(gt);
+ intel_gt_pm_put(ce->engine->gt);
for_each_child(ce, child)
intel_context_put(child);
intel_context_put(ce);
@@ -2780,12 +2769,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));
intel_gt_pm_put(eb->gt);
for_each_child(eb->context, child)
intel_context_put(child);
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index d09aad34ba37f..727123ebfc06e 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -34,6 +34,7 @@
#include "gt/intel_engine.h"
#include "gt/intel_engine_heartbeat.h"
#include "gt/intel_gt.h"
+#include "gt/intel_gt_pm.h"
#include "gt/intel_gt_requests.h"
#include "gt/intel_tlb.h"
@@ -103,12 +104,25 @@ static inline struct i915_vma *active_to_vma(struct i915_active *ref)
static int __i915_vma_active(struct i915_active *ref)
{
- return i915_vma_tryget(active_to_vma(ref)) ? 0 : -ENOENT;
+ struct i915_vma *vma = active_to_vma(ref);
+
+ if (!i915_vma_tryget(vma))
+ return -ENOENT;
+
+ if (!i915_vma_is_ggtt(vma))
+ intel_gt_pm_get(vma->vm->gt);
+
+ return 0;
}
static void __i915_vma_retire(struct i915_active *ref)
{
- i915_vma_put(active_to_vma(ref));
+ struct i915_vma *vma = active_to_vma(ref);
+
+ if (!i915_vma_is_ggtt(vma))
+ intel_gt_pm_put_async(vma->vm->gt);
+
+ i915_vma_put(vma);
}
static struct i915_vma *
@@ -1404,7 +1418,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
struct i915_vma_work *work = NULL;
struct dma_fence *moving = NULL;
struct i915_vma_resource *vma_res = NULL;
- intel_wakeref_t wakeref = 0;
+ intel_wakeref_t wakeref;
unsigned int bound;
int err;
@@ -1424,8 +1438,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
if (err)
return err;
- if (flags & PIN_GLOBAL)
- wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm);
+ wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm);
if (flags & vma->vm->bind_async_flags) {
/* lock VM */
@@ -1561,8 +1574,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
if (work)
dma_fence_work_commit_imm(&work->base);
err_rpm:
- if (wakeref)
- intel_runtime_pm_put(&vma->vm->i915->runtime_pm, wakeref);
+ intel_runtime_pm_put(&vma->vm->i915->runtime_pm, wakeref);
if (moving)
dma_fence_put(moving);
@@ -1740,8 +1752,6 @@ static void release_references(struct i915_vma *vma, struct intel_gt *gt,
if (vm_ddestroy)
i915_vm_resv_put(vma->vm);
- /* Wait for async active retire */
- i915_active_wait(&vma->active);
i915_active_fini(&vma->active);
GEM_WARN_ON(vma->resource);
i915_vma_free(vma);
--
2.42.1
More information about the Intel-gfx-trybot
mailing list