Re: [PATCH v2 04/13] drm/panic: Return errno codes if panic output fails

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Using KUNIT_ASSERT_EQ causes kernel test memory to leak if the drawing function returns an error, bypassing explicit resource cleanup.

Pre-existing issues:
- [High] Concurrent invocation of the debugfs trigger for planes on different devices causes data races on global panic buffers, leading to memory corruption.
--

--- Patch [4]: [PATCH v2 04/13] drm/panic: Return errno codes if panic output fails ---
commit 6a8545466378db489975c829328cc895f57550b4
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/panic: Return errno codes if panic output fails

Return errno codes from the panic output helpers to detect invalid panic
handling and avoid flushing the display if an error occurred. The panic
test cases are updated to assert these new return values.

> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 62490d32019cc..cc33a2b4bb52a 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c

[ ... ]

> @@ -945,9 +954,15 @@ static void draw_panic_plane(struct drm_plane *plane, const char *description)
>  
>  	drm_panic_set_description(description);
>  
> -	draw_panic_dispatch(&sb);
> -	if (plane->helper_private->panic_flush)
> -		plane->helper_private->panic_flush(plane);
> +	ret = draw_panic_dispatch(&sb);

[Severity: High]
This is a pre-existing issue, but can concurrent writes to the debugfs trigger
corrupt global buffers?

If a user concurrently writes to the debugfs trigger files (drm_panic_plane_%d)
for planes on different DRM devices, draw_panic_plane() executes concurrently
because dev->mode_config.panic_lock is a per-device lock.

When DRM_PANIC_TYPE_QR is active, multiple threads could enter
drm_panic_get_qr_code_url() and modify the global static variables qrbuf1,
qrbuf2, and stream.workspace without cross-device synchronization:

drivers/gpu/drm/drm_panic.c:drm_panic_get_qr_code_url() {
    ...
    stream.next_in = kmsg;
    stream.avail_in = kmsg_len;
    stream.total_in = 0;
    stream.next_out = qrbuf2;
    stream.avail_out = QR_BUFFER2_SIZE;
    stream.total_out = 0;
    ...
}

Could this cause memory corruption during simultaneous debugfs writes?

> +	if (!ret) {
> +		/*
> +		 * Only flush if we have a panic screen to display. Otherwise
> +		 * it's probably better to leave the display output as-is.
> +		 */
> +		if (plane->helper_private->panic_flush)
> +			plane->helper_private->panic_flush(plane);
> +	}
>  
>  	drm_panic_clear_description();

[ ... ]

> diff --git a/drivers/gpu/drm/tests/drm_panic_test.c b/drivers/gpu/drm/tests/drm_panic_test.c
> index ad2f3a2f93b6a..fdd77b0cc54c9 100644
> --- a/drivers/gpu/drm/tests/drm_panic_test.c
> +++ b/drivers/gpu/drm/tests/drm_panic_test.c

[ ... ]

> @@ -102,7 +102,8 @@ static void drm_test_panic_screen_user_map(struct kunit *test)
>  	sb->height = params->height;
>  	sb->pitch[0] = params->width * sb->format->cpp[0];
>  
> -	params->draw_screen(sb);
> +	ret = params->draw_screen(sb);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
Will this KUNIT_ASSERT_EQ() cause memory leaks if the test fails?

If params->draw_screen() returns an error, KUNIT_ASSERT_EQ() will abort the
test thread immediately. This means the test will skip explicit resource
cleanup, such as the vfree(fb) call at the end of
drm_test_panic_screen_user_map().

Should this use KUNIT_EXPECT_EQ() instead to allow the cleanup code to run?

>  
>  	for (i = 0; i < fb_size; i++)
>  		drm_panic_check_color_byte(test, fb[i]);

[ ... ]

> @@ -146,7 +147,8 @@ static void drm_test_panic_screen_user_page(struct kunit *test)
>  	sb->height = params->height;
>  	sb->pitch[0] = params->width * sb->format->cpp[0];
>  
> -	params->draw_screen(sb);
> +	ret = params->draw_screen(sb);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
Similarly here in drm_test_panic_screen_user_page(), if KUNIT_ASSERT_EQ()
aborts the test on failure, will it skip the free_pages: cleanup label,
resulting in a permanent memory leak?

>  
>  	for (p = 0; p < npages; p++) {
>  		int bytes_in_page = (p == npages - 1) ? fb_size - p * PAGE_SIZE : PAGE_SIZE;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824125201.218863-1-tzimmermann@suse.de?part=4