Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> On Mon, Feb 06, 2017 at 03:57:47PM +0200, Mika Kuoppala wrote:
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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.
> + */
> +
Agreed that it is better in this spot and that any pointer chasing
will lead to trouble.
-Mika
> if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
> continue;
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
