Re: [PATCH 06/12] drm/panic: Pass global module parameters to drm_panic_dispatch()

[sashiko-bot@kernel.org]

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 <tzimmermann@suse.de>

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/20260818125012.468092-1-tzimmermann@suse.de?part=6