[Intel-gfx] [PATCH 2/2] drm/i915: Avoid unguarded reads from the request pointer
Chris Wilson
chris at chris-wilson.co.uk
Mon Feb 6 14:16:09 UTC 2017
On Mon, Feb 06, 2017 at 03:57:47PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
>
> > In commit 86aa7e760a67 ("drm/i915: Assert that the context-switch
> > completion matches our context") I added a read to the irq tasklet
> > handler that compared the on-chip status with that of our sw tracking,
> > using an unguarded read of the request pointer to get the context and
> > beyond. Whilst we hold a reference to the request, we do not hold
> > anything on the context and if we are unlucky it may be reaped from a
> > second thread retiring the request (since it may retire the request as
> > soon as the breadcrumb is complete, even before we finish processing the
> > context switch) as we try to read from the context pointer.
> >
>
> Please add warning of the possibility of context vanishing beneath
> our feet. Perhaps a good spot is when we store a bug on variable
> context_id.
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c6c5050c79c0..937e2af2da64 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -564,6 +564,22 @@ static void intel_lrc_irq_handler(unsigned long data)
unsigned int idx = ++head % GEN8_CSB_ENTRIES;
unsigned int status = readl(buf + 2 * idx);
+ /* We are flying near dragons again.
+ *
+ * We hold a reference to the request in execlist_port[]
+ * but no more than that. We are operating in softirq
+ * context and so cannot hold any mutex or sleep. That
+ * prevents us stopping the requests we are processing
+ * in port[] from being retired simultaneously (the
+ * breadcrumb will be complete before we see the
+ * context-switch). As we only hold the reference to
+ * request, any pointer chasing underneath the request
+ * is subject to a potential use-after-free. Thus we
+ * store all of the bookkeeping within port[], if
+ * required, and avoid using request itself. The
+ * same applies to the atomic status notifier.
+ */
+
if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
continue;
--
Chris Wilson, Intel Open Source Technology Centre
More information about the Intel-gfx
mailing list