Commit 4c71fd09 authored by Sebastian Brzezinka's avatar Sebastian Brzezinka Committed by Joonas Lahtinen
Browse files

drm/i915/gt: fix refcount underflow in intel_engine_park_heartbeat 

A use-after-free / refcount underflow is possible when the heartbeat
worker and intel_engine_park_heartbeat() race to release the same
engine->heartbeat.systole request.

The heartbeat worker reads engine->heartbeat.systole and calls
i915_request_put() on it when the request is complete, but clears
the pointer in a separate, non-atomic step. Concurrently, a request
retirement on another CPU can drop the engine wakeref to zero, triggering
__engine_park() -> intel_engine_park_heartbeat(). If the heartbeat
timer is pending at that point, cancel_delayed_work() returns true and
intel_engine_park_heartbeat() reads the stale non-NULL systole pointer
and calls i915_request_put() on it again, causing a refcount underflow:

```
<4> [487.221889] Workqueue: i915-unordered engine_retire [i915]
<4> [487.222640] RIP: 0010:refcount_warn_saturate+0x68/0xb0
...
<4> [487.222707] Call Trace:
<4> [487.222711]  <TASK>
<4> [487.222716]  intel_engine_park_heartbeat.part.0+0x6f/0x80 [i915]
<4> [487.223115]  intel_engine_park_heartbeat+0x25/0x40 [i915]
<4> [487.223566]  __engine_park+0xb9/0x650 [i915]
<4> [487.223973]  ____intel_wakeref_put_last+0x2e/0xb0 [i915]
<4> [487.224408]  __intel_wakeref_put_last+0x72/0x90 [i915]
<4> [487.224797]  intel_context_exit_engine+0x7c/0x80 [i915]
<4> [487.225238]  intel_context_exit+0xf1/0x1b0 [i915]
<4> [487.225695]  i915_request_retire.part.0+0x1b9/0x530 [i915]
<4> [487.226178]  i915_request_retire+0x1c/0x40 [i915]
<4> [487.226625]  engine_retire+0x122/0x180 [i915]
<4> [487.227037]  process_one_work+0x239/0x760
<4> [487.227060]  worker_thread+0x200/0x3f0
<4> [487.227068]  ? __pfx_worker_thread+0x10/0x10
<4> [487.227075]  kthread+0x10d/0x150
<4> [487.227083]  ? __pfx_kthread+0x10/0x10
<4> [487.227092]  ret_from_fork+0x3d4/0x480
<4> [487.227099]  ? __pfx_kthread+0x10/0x10
<4> [487.227107]  ret_from_fork_asm+0x1a/0x30
<4> [487.227141]  </TASK>
```

Fix this by replacing the non-atomic pointer read + separate clear with
xchg() in both racing paths. xchg() is a single indivisible hardware
instruction that atomically reads the old pointer and writes NULL. This
guarantees only one of the two concurrent callers obtains the non-NULL
pointer and performs the put, the other gets NULL and skips it.

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15880


Fixes: 058179e7 ("drm/i915/gt: Replace hangcheck by heartbeats")
Cc: <stable@vger.kernel.org> # v5.5+
Signed-off-by: default avatarSebastian Brzezinka <sebastian.brzezinka@intel.com>
Reviewed-by: default avatarKrzysztof Karas <krzysztof.karas@intel.com>
Reviewed-by: default avatarAndi Shyti <andi.shyti@linux.intel.com>
Signed-off-by: default avatarAndi Shyti <andi.shyti@linux.intel.com>
Link: https://lore.kernel.org/r/d4c1c14255688dd07cc8044973c4f032a8d1559e.1775038106.git.sebastian.brzezinka@intel.com


(cherry picked from commit 13238dc0ee4f9ab8dafa2cca7295736191ae2f42)
Signed-off-by: default avatarJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
parent 75519f5d

drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c

+18 −8
Original line number Diff line number Diff line
@@ -148,10 +148,12 @@ static void heartbeat(struct work_struct *wrk) 
	/* Just in case everything has gone horribly wrong, give it a kick */

	intel_engine_flush_submission(engine);



	rq = engine->heartbeat.systole;

	if (rq && i915_request_completed(rq)) {

	rq = xchg(&engine->heartbeat.systole, NULL);

	if (rq) {

		if (i915_request_completed(rq))

			i915_request_put(rq);

		engine->heartbeat.systole = NULL;

		else

			engine->heartbeat.systole = rq;

	}



	if (!intel_engine_pm_get_if_awake(engine))
@@ -232,8 +234,11 @@ static void heartbeat(struct work_struct *wrk) 
unlock:

	mutex_unlock(&ce->timeline->mutex);

out:

	if (!engine->i915->params.enable_hangcheck || !next_heartbeat(engine))

		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));

	if (!engine->i915->params.enable_hangcheck || !next_heartbeat(engine)) {

		rq = xchg(&engine->heartbeat.systole, NULL);

		if (rq)

			i915_request_put(rq);

	}

	intel_engine_pm_put(engine);

}
@@ -247,8 +252,13 @@ void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine) 


void intel_engine_park_heartbeat(struct intel_engine_cs *engine)

{

	if (cancel_delayed_work(&engine->heartbeat.work))

		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));

	if (cancel_delayed_work(&engine->heartbeat.work)) {

		struct i915_request *rq;



		rq = xchg(&engine->heartbeat.systole, NULL);

		if (rq)

			i915_request_put(rq);

	}

}



void intel_gt_unpark_heartbeats(struct intel_gt *gt)