[PATCH] drm/i915/guc: Sort multi-lrc engines by logical instance

[PATCH] drm/i915/guc: Sort multi-lrc engines by logical instance

[Linmao Li]

logical_sort() looks up the engine for each logical instance i by
scanning all MAX_ENGINE_INSTANCE + 1 slots of the engines array, even
though its only caller initializes just num_engines entries, so the
scan can dereference uninitialized stack pointers.  In addition, the
final memcpy() passes *engines and *sorted instead of the arrays
themselves, so it copies engine structure data rather than the pointer
arrays.

Replace the implementation with a plain sort() of the initialized part
of the array, keyed by the logical mask.  This touches only initialized
entries, does not copy anything by hand, and does not rely on the
logical numbering being contiguous.

Fixes: f9d72092cb49 ("drm/i915/guc: Add basic GuC multi-lrc selftest")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 .../drm/i915/gt/uc/selftest_guc_multi_lrc.c   | 28 ++++++++++---------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
index 28e8a092f4e7..b56768d202b0 100644
--- a/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
+++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
@@ -3,6 +3,8 @@
  * Copyright © 2019 Intel Corporation
  */
 
+#include <linux/sort.h>
+
 #include "gt/intel_gt_print.h"
 #include "selftests/igt_spinner.h"
 #include "selftests/igt_reset.h"
@@ -10,21 +12,21 @@
 #include "gt/intel_engine_heartbeat.h"
 #include "gem/selftests/mock_context.h"
 
+static int cmp_logical_instance(const void *a, const void *b)
+{
+	const struct intel_engine_cs *ea = *(const struct intel_engine_cs **)a;
+	const struct intel_engine_cs *eb = *(const struct intel_engine_cs **)b;
+
+	if (ea->logical_mask < eb->logical_mask)
+		return -1;
+	if (ea->logical_mask > eb->logical_mask)
+		return 1;
+	return 0;
+}
+
 static void logical_sort(struct intel_engine_cs **engines, int num_engines)
 {
-	struct intel_engine_cs *sorted[MAX_ENGINE_INSTANCE + 1];
-	int i, j;
-
-	for (i = 0; i < num_engines; ++i)
-		for (j = 0; j < MAX_ENGINE_INSTANCE + 1; ++j) {
-			if (engines[j]->logical_mask & BIT(i)) {
-				sorted[i] = engines[j];
-				break;
-			}
-		}
-
-	memcpy(*engines, *sorted,
-	       sizeof(struct intel_engine_cs *) * num_engines);
+	sort(engines, num_engines, sizeof(*engines), cmp_logical_instance, NULL);
 }
 
 static struct intel_context *
-- 
2.25.1

Re: [PATCH] drm/i915/guc: Sort multi-lrc engines by logical instance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] On failure to allocate a child request in multi_lrc_nop_request(), the already-submitted parent request (and possibly some child requests) are abandoned without waiting for them to complete. This leads to a Use-After-Free when the parent context is unpinned and freed by the caller.
--

commit 5b7b11cc22424a0209d44bac64a471f684e9959f
Author: Linmao Li <lilinmao@kylinos.cn>

drm/i915/guc: Sort multi-lrc engines by logical instance
    
This commit replaces a custom logical sorting loop with the standard sort()
function for multi-lrc engines. This fixes potential out-of-bounds stack
accesses and corrects the sorting logic to copy pointers rather than the
engine structure data.

> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
> index 28e8a092f4e7d..b56768d202b06 100644
> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c

[ ... ]

While reviewing this code, I noticed a pre-existing issue in the same file:

drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c:multi_lrc_nop_request()
> 		child_rq = intel_context_create_request(child);
> 		if (IS_ERR(child_rq))
> 			goto child_error;

[Severity: Critical]
This is a pre-existing issue, but does this error path leave earlier parent
or child requests in flight without waiting for them to complete?

If so, when the caller __intel_guc_multi_lrc_basic() receives this error:

drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c:__intel_guc_multi_lrc_basic()
> out:
> 	multi_lrc_context_unpin(parent);
> 	multi_lrc_context_put(parent);

Could this lead to a use-after-free? The caller forces the unpin and drops
the final creation reference to the context while the stranded in-flight
requests still hold dangling pointers to it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706111328.618675-1-lilinmao@kylinos.cn?part=1

✗ i915.CI.BAT: failure for drm/i915/guc: Sort multi-lrc engines by logical instance

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

== Series Details ==

Series: drm/i915/guc: Sort multi-lrc engines by logical instance
URL   : https://patchwork.freedesktop.org/series/169853/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_18765 -> Patchwork_169853v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_169853v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_169853v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169853v1/index.html

Participating hosts (42 -> 40)
------------------------------

  Missing    (2): bat-dg2-13 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_169853v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - bat-atsm-1:         [PASS][1] -> [INCOMPLETE][2] +1 other test incomplete
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18765/bat-atsm-1/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169853v1/bat-atsm-1/igt@i915_selftest@live@hangcheck.html

  


Build changes
-------------

  * Linux: CI_DRM_18765 -> Patchwork_169853v1

  CI-20190529: 20190529
  CI_DRM_18765: 9179ea89567d036ecc9013cc27af4a367aeb14b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8990: 8990
  Patchwork_169853v1: 9179ea89567d036ecc9013cc27af4a367aeb14b9 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_169853v1/index.html

[-- Attachment #2: Type: text/html, Size: 2454 bytes --]