Am 18.08.26 um 15:09 schrieb [email protected]:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
to consider:
Pre-existing issues:
- [High] Concurrent debugfs writes or KUnit tests across multiple DRM devices
race on global static buffers without adequate synchronization.
A fix would be nice, but is beyond this series. As it affects debugfs
and kunit test there's no urgency.
--
--- Patch [6]: [PATCH 06/12] drm/panic: Pass global module parameters to
drm_panic_dispatch() ---
commit 9f2a1b5158feac614c09a17c83f387591642e034
Author: Thomas Zimmermann <[email protected]>
drm/panic: Pass global module parameters to drm_panic_dispatch()
This patch sets the panic type and QR version in draw_panic_plane() as a single
point of truth, passing these values into the individual drawing functions.
In the test case, it always creates a QR code with version 40.
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index ede620b27515a..12c39217cbcb5 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
[ ... ]
@@ -951,6 +955,11 @@ static void draw_panic_plane(struct drm_plane *plane,
const char *description)
#else
u32 bg_color = 0x00000000;
#endif
+#if IS_ENABLED(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
+ unsigned int qr_version = panic_qr_version;
+#else
+ unsigned int qr_version = 0;
+#endif
if (!drm_panic_trylock(plane->dev, flags))
return;
@@ -966,7 +975,7 @@ static void draw_panic_plane(struct drm_plane *plane, const
char *description)
drm_panic_set_description(description);
- ret = draw_panic_dispatch(&sb, fg_color, bg_color);
+ ret = draw_panic_dispatch(&sb, drm_panic_type, fg_color, bg_color,
qr_version);
[Severity: High]
This is a pre-existing issue, but does the per-device locking in
drm_panic_trylock() adequately protect the global buffers?
Since the lock is per-device (plane->dev->mode_config.panic_lock), concurrent
debugfs writes across multiple DRM devices can still race and mutate global
static variables, such as the zlib stream workspace (struct z_stream_s stream)
or qrbuf1 and qrbuf2.
If a real kernel panic occurs while a debugfs write is concurrently using these
buffers, the true panic handler could inherit a corrupted zlib workspace state,
which might lead to a recursive fault and failure to display the panic screen.
Could this be protected by a global lock for non-panic paths?
[ ... ]
diff --git a/drivers/gpu/drm/tests/drm_panic_test.c
b/drivers/gpu/drm/tests/drm_panic_test.c
index 969a096ec840f..196496238d231 100644
--- a/drivers/gpu/drm/tests/drm_panic_test.c
+++ b/drivers/gpu/drm/tests/drm_panic_test.c
@@ -58,7 +58,7 @@ static int draw_panic_screen_kmsg_default(struct
drm_scanout_buffer *sb)
#if IS_ENABLED(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
static int draw_panic_screen_qr_code_default(struct drm_scanout_buffer *sb)
{
- return draw_panic_screen_qr_code(sb, 0x00ffffff, 0x00000000);
+ return draw_panic_screen_qr_code(sb, 0x00ffffff, 0x00000000, 40);
}
#endif
[Severity: High]
This isn't a bug introduced by this patch, but do the KUnit tests safely access
the global drawing state?
It appears the KUnit tests bypass draw_panic_plane() entirely and directly call
drawing functions like draw_panic_screen_qr_code() without acquiring any locks.
Could running the tests race on the global buffers and corrupt the zlib
workspace?
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
