Re: [PATCH 1/2] drm/i915/guc: Complete all requests of a non-schedulable context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/i915/guc: use-after-free in __guc_add_request() on
  completed rq
- [High] drm/i915/guc: out-of-order completion of rq in
  __guc_add_request()
- [Medium] drm/i915/guc: uncompleted requests in __guc_add_request() on
  pending enable
--

commit 75c751fdc5e9421329da29b1d39d5306a72785b3
Author: Javier MV <javiermv@duck.com>

drm/i915/guc: Complete all requests of a non-schedulable context

This commit completes all requests on ce->guc_state.requests with -EIO in
__guc_add_request() when a context is not schedulable and is off the
hardware.

Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15301

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 788e59cdfac9b..555ca8fdf64af 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -709,6 +709,20 @@ static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>  	 * request resubmitted after the context was banned.
>  	 */
>  	if (unlikely(!intel_context_is_schedulable(ce))) {
> +		struct i915_request *pos;
> +
> +		/*
> +		 * The dequeue only passes the last of the merged requests, and
> +		 * the context won't run again to write the others' breadcrumbs:
> +		 * complete them all, once the context is off the HW.
> +		 */
> +		spin_lock(&ce->guc_state.lock);
> +		if (!context_enabled(ce) && !context_pending_enable(ce) &&
> +		    !context_pending_disable(ce))
> +			list_for_each_entry(pos, &ce->guc_state.requests, sched.link)
> +				i915_request_put(i915_request_mark_eio(pos));

[Severity: Medium]
Can checking !context_pending_enable(ce) leave requests on
ce->guc_state.requests incomplete forever?

When GuC resets a context before an in-flight schedule-enable G2H is
processed, __guc_reset_context() calls clr_context_enabled(ce) while
SCHED_STATE_PENDING_ENABLE remains set and SCHED_STATE_PENDING_DISABLE is
clear.  In that state guc_context_revoke() treats the context as already off
the hardware and does not send a disable H2G:

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:guc_context_revoke() {
	...
	if (submission_disabled(guc) ||
	    (!context_enabled(ce) && !context_pending_disable(ce))) {
		spin_unlock_irqrestore(&ce->guc_state.lock, flags);

		guc_cancel_context_requests(ce);
		intel_engine_signal_breadcrumbs(ce->engine);
	...
}

When the pending enable G2H later arrives in
intel_guc_sched_done_process_msg(), it only calls clr_context_pending_enable()
and never calls guc_cancel_context_requests(ce), so any requests skipped here
because of context_pending_enable(ce) will never be completed.

> +		spin_unlock(&ce->guc_state.lock);
> +
> +		i915_request_put(i915_request_mark_eio(rq));
> +		intel_engine_signal_breadcrumbs(ce->engine);

[Severity: High]
Can this trigger a use-after-free on rq (and ce->engine) after dropping
ce->guc_state.lock?

Both callers of guc_add_request() first run __i915_request_submit(rq) ->
add_to_context(rq), which moves rq onto ce->guc_state.requests without taking
an extra reference beyond the timeline reference on tl->requests.  When the
loop above runs, it visits pos == rq and marks rq complete under
ce->guc_state.lock.  In GuC mode, request retirement does not take
sched_engine->lock; it only serializes on tl->mutex and ce->guc_state.lock in
remove_from_context():

CPU0                                    CPU1
__guc_add_request(guc, rq)
  spin_lock(&ce->guc_state.lock)
  list_for_each_entry(pos, ...)
    i915_request_mark_eio(rq)
    i915_request_put(rq)
  spin_unlock(&ce->guc_state.lock)
                                        i915_request_retire(rq)
                                          remove_from_context(rq)
                                          intel_context_unpin(ce)
                                          i915_request_put(rq)  <-- frees rq
  i915_request_mark_eio(rq)  <-- use-after-free
  intel_engine_signal_breadcrumbs(ce->engine)

If rq is recycled from the SLAB_TYPESAFE_BY_RCU pool by another context on
CPU1 before CPU0 reaches the second i915_request_mark_eio(rq), CPU0 will mark
that unrelated request complete with -EIO, or access freed memory if the slab
or virtual engine context was freed.

[Severity: High]
When the context is still enabled or has a schedule transition in flight,
does marking rq complete outside the if condition complete rq out of order
ahead of earlier requests on the same timeline?

guc_dequeue_one_context() merges all queued requests of a context onto
ce->guc_state.requests and passes only the last request rq to
__guc_add_request().  If context_enabled(ce), context_pending_enable(ce), or
context_pending_disable(ce) is true, the loop is skipped and earlier requests
on the timeline remain incomplete, but rq at the tail of the timeline is
still marked complete here.

Because __i915_active_fence_set() and dma_resv replace earlier timeline
fences with the newest request's fence, completing rq while earlier requests
are still running on the GPU allows VMAs to be unbound and backing pages
freed during active GPU DMA, and triggers
GEM_BUG_ON(!i915_request_completed(tmp)) in i915_request_retire_upto().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110122534.12989-1-javiermv@gmail.com?part=1