<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">On 11.07.2023 12:18, Andi Shyti wrote:<br>
</div>
<blockquote type="cite"
cite="mid:ZK0sbz+h0r%2FPwYn2@ashyti-mobl2.lan">
<pre class="moz-quote-pre" wrap="">Hi Andrzej,
On Thu, Jul 06, 2023 at 05:16:11PM +0200, Andrzej Hajda wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">GuC virtual engines can be removed before request removal. On the other
side driver expects rq->engine to be a valid pointer for a whole life of
request. Setting rq->engine to an always valid engine should solve
the issue.
The patch fixes bug detected by KASAN with following signature:
i915 0000:00:02.0: [drm:i915_drop_caches_set [i915]] Dropping caches: 0x0000005c [0x0000005c]
BUG: KASAN: slab-use-after-free in i915_fence_release+0x2a2/0x2f0 [i915]
Read of size 4 at addr ffff88813ffda6e8 by task kworker/u32:10/157
...
Allocated by task 1184:
...
guc_create_virtual+0x4d/0x1160 [i915]
i915_gem_create_context+0x11ee/0x18c0 [i915]
...
Freed by task 151:
...
intel_guc_deregister_done_process_msg+0x324/0x6d0 [i915]
...
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
so the only difference between v1 and v2 is this part of the log?</pre>
</blockquote>
<br>
yes, I forgot to add changelog.<br>
<br>
<blockquote type="cite"
cite="mid:ZK0sbz+h0r%2FPwYn2@ashyti-mobl2.lan">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Closes: <a class="moz-txt-link-freetext" href="https://gitlab.freedesktop.org/drm/intel/-/issues/7926">https://gitlab.freedesktop.org/drm/intel/-/issues/7926</a>
Signed-off-by: Andrzej Hajda <a class="moz-txt-link-rfc2396E" href="mailto:andrzej.hajda@intel.com"><andrzej.hajda@intel.com></a>
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 +++++++++++
1 file changed, 11 insertions(+)
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 a0e3ef1c65d246..2c877ea5eda6f0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -3461,6 +3461,8 @@ static void guc_prio_fini(struct i915_request *rq, struct intel_context *ce)
static void remove_from_context(struct i915_request *rq)
{
struct intel_context *ce = request_to_scheduling_context(rq);
+ struct intel_engine_cs *engine;
+ intel_engine_mask_t tmp;
GEM_BUG_ON(intel_context_is_child(ce));
@@ -3478,6 +3480,15 @@ static void remove_from_context(struct i915_request *rq)
atomic_dec(&ce->guc_id.ref);
i915_request_notify_execute_cb_imm(rq);
+
+ /*
+ * GuC virtual engine can disappear after this call, so let's assign
+ * something valid, as driver expects this to be always valid pointer.
+ */
+ for_each_engine_masked(engine, rq->engine->gt, rq->execution_mask, tmp) {
+ rq->engine = engine;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
yes... here the context might lose the virtual engine... I wonder
whether this is the rigth solution, though. Maybe we should set
rq->engine = NULL; and check for NULL? Don't know.</pre>
</blockquote>
<br>
Setting NULL causes occasional null page de-reference in <br>
<div style="background-color:#ffffff;padding:0px 0px 0px 0px;">
<div style="color:#000000;background-color:#ffffff;font-family:"Consolas";font-size:10pt;white-space:pre;"><p style="margin:0;">i915_request_wait_timeout:</p><p style="margin:0;"> mutex_release(&rq->engine->gt->reset.mutex.dep_map, _THIS_IP_)</p><p style="margin:0;">
</p><p style="margin:0;">rq->engine after removing rq from context is (IMHO) used as a set of aliases for gt and i915 (despite rq itself contains the alias to i915).</p><p style="margin:0;">
</p><p style="margin:0;">Regards</p><p style="margin:0;">Andrzej
</p><p style="margin:0;">
</p><p style="margin:0;">
</p></div>
</div>
<blockquote type="cite"
cite="mid:ZK0sbz+h0r%2FPwYn2@ashyti-mobl2.lan">
<pre class="moz-quote-pre" wrap="">
Andi
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ break;
+ }
}
static const struct intel_context_ops guc_context_ops = {
--
2.34.1
</pre>
</blockquote>
</blockquote>
<br>
</body>
</html>