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. -- --- 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
