| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d2b91aea-02d6-4fbf-9ada-ef831936c773@redhat.com>
Date: Thu, 25 Sep 2025 17:50:47 +0200
From: Jocelyn Falempe <jfalempe@...hat.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>,
Javier Martinez Canillas <javierm@...hat.com>, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/3] drm/panic: Add kunit tests for drm_panic
On 23/09/2025 11:57, Maxime Ripard wrote:
> On Wed, Sep 10, 2025 at 05:16:49PM +0200, Jocelyn Falempe wrote:
>> On 10/09/2025 10:33, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Mon, Sep 08, 2025 at 11:00:30AM +0200, Jocelyn Falempe wrote:
>>>> Add kunit tests for drm_panic.
>>>> They check that drawing the panic screen doesn't crash, but they
>>>> don't check the correctness of the resulting image.
>>>>
>>>> Signed-off-by: Jocelyn Falempe <jfalempe@...hat.com>
>>>> ---
>>>>
>>>> v2:
>>>> * Add a few checks, and more comments in the kunit tests. (Maxime Ripard).
>>>>
>>>> MAINTAINERS | 1 +
>>>> drivers/gpu/drm/drm_panic.c | 4 +
>>>> drivers/gpu/drm/tests/drm_panic_test.c | 198 +++++++++++++++++++++++++
>>>> 3 files changed, 203 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/tests/drm_panic_test.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 402fe14091f1..e9be893d6741 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -8480,6 +8480,7 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
>>>> F: drivers/gpu/drm/drm_draw.c
>>>> F: drivers/gpu/drm/drm_draw_internal.h
>>>> F: drivers/gpu/drm/drm_panic*.c
>>>> +F: drivers/gpu/drm/tests/drm_panic_test.c
>>>> F: include/drm/drm_panic*
>>>> DRM PANIC QR CODE
>>>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>>>> index 1e06e3a18d09..d89812ff1935 100644
>>>> --- a/drivers/gpu/drm/drm_panic.c
>>>> +++ b/drivers/gpu/drm/drm_panic.c
>>>> @@ -986,3 +986,7 @@ void drm_panic_exit(void)
>>>> {
>>>> drm_panic_qr_exit();
>>>> }
>>>> +
>>>> +#ifdef CONFIG_DRM_KUNIT_TEST
>>>> +#include "tests/drm_panic_test.c"
>>>> +#endif
>>>> diff --git a/drivers/gpu/drm/tests/drm_panic_test.c b/drivers/gpu/drm/tests/drm_panic_test.c
>>>> new file mode 100644
>>>> index 000000000000..d5d20dd2aa7c
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/tests/drm_panic_test.c
>>>> @@ -0,0 +1,198 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 or MIT
>>>> +/*
>>>> + * Copyright (c) 2025 Red Hat.
>>>> + * Author: Jocelyn Falempe <jfalempe@...hat.com>
>>>> + *
>>>> + * KUNIT tests for drm panic
>>>> + */
>>>> +
>>>> +#include <drm/drm_fourcc.h>
>>>> +#include <drm/drm_panic.h>
>>>> +
>>>> +#include <kunit/test.h>
>>>> +
>>>> +#include <linux/units.h>
>>>> +#include <linux/vmalloc.h>
>>>> +
>>>> +/* Check the framebuffer color only if the panic colors are the default */
>>>> +#if (CONFIG_DRM_PANIC_BACKGROUND_COLOR == 0 && \
>>>> + CONFIG_DRM_PANIC_FOREGROUND_COLOR == 0xffffff)
>>>> +#define DRM_PANIC_CHECK_COLOR
>>>> +#endif
>>>> +
>>>> +struct drm_test_mode {
>>>> + const int width;
>>>> + const int height;
>>>> + const u32 format;
>>>> + void (*draw_screen)(struct drm_scanout_buffer *sb);
>>>> + const char *fname;
>>>> +};
>>>> +
>>>> +/*
>>>> + * Run all tests for the 3 panic screens: user, kmsg and qr_code
>>>> + */
>>>> +#define DRM_TEST_MODE_LIST(func) \
>>>> + DRM_PANIC_TEST_MODE(1024, 768, DRM_FORMAT_XRGB8888, func) \
>>>> + DRM_PANIC_TEST_MODE(300, 200, DRM_FORMAT_XRGB8888, func) \
>>>> + DRM_PANIC_TEST_MODE(1920, 1080, DRM_FORMAT_XRGB8888, func) \
>>>> + DRM_PANIC_TEST_MODE(1024, 768, DRM_FORMAT_RGB565, func) \
>>>> + DRM_PANIC_TEST_MODE(1024, 768, DRM_FORMAT_RGB888, func) \
>>>> +
>>>> +#define DRM_PANIC_TEST_MODE(w, h, f, name) { \
>>>> + .width = w, \
>>>> + .height = h, \
>>>> + .format = f, \
>>>> + .draw_screen = draw_panic_screen_##name, \
>>>> + .fname = #name, \
>>>> + }, \
>>>> +
>>>> +static const struct drm_test_mode drm_test_modes_cases[] = {
>>>> + DRM_TEST_MODE_LIST(user)
>>>> + DRM_TEST_MODE_LIST(kmsg)
>>>> + DRM_TEST_MODE_LIST(qr_code)
>>>> +};
>>>> +#undef DRM_PANIC_TEST_MODE
>>>> +
>>>> +static int drm_test_panic_init(struct kunit *test)
>>>> +{
>>>> + struct drm_scanout_buffer *priv;
>>>> +
>>>> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
>>>> + KUNIT_ASSERT_NOT_NULL(test, priv);
>>>> +
>>>> + test->priv = priv;
>>>> +
>>>> + drm_panic_set_description("Kunit testing");
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Test drawing the panic screen, using a memory mapped framebuffer
>>>> + * Set the whole buffer to 0xa5, and then check that all pixels have been
>>>> + * written.
>>>> + */
>>>> +static void drm_test_panic_screen_user_map(struct kunit *test)
>>>> +{
>>>> + struct drm_scanout_buffer *sb = test->priv;
>>>> + const struct drm_test_mode *params = test->param_value;
>>>> + char *fb;
>>>> + int fb_size;
>>>> +
>>>> + sb->format = drm_format_info(params->format);
>>>> + fb_size = params->width * params->height * sb->format->cpp[0];
>>>> +
>>>> + fb = vmalloc(fb_size);
>>>> + KUNIT_ASSERT_NOT_NULL(test, fb);
>>>> +
>>>> + memset(fb, 0xa5, fb_size);
>>>> +
>>>> + iosys_map_set_vaddr(&sb->map[0], fb);
>>>> + sb->width = params->width;
>>>> + sb->height = params->height;
>>>> + sb->pitch[0] = params->width * sb->format->cpp[0];
>>>> +
>>>> + params->draw_screen(sb);
>>>> +
>>>> +#ifdef DRM_PANIC_CHECK_COLOR
>>>> + {
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < fb_size; i++)
>>>> + KUNIT_ASSERT_TRUE(test, fb[i] == 0 || fb[i] == 0xff);
>>>> + }
>>>> +#endif
>>>
>>> I'm not really fond of the ifdef here. Could you turn this into a
>>> function, and return that it's valid if the colors don't match what you
>>> expect?
>>
>> Yes, I can rework this.
>>>
>>>> + vfree(fb);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Test drawing the panic screen, using a list of pages framebuffer
>>>> + * No checks are performed
>>>
>>> What are you testing then if you aren't checking anything?
>>
>> It tests that there are no access to an unmapped page.
>> But I can add the same check that with the "map" case.
>> It just requires more work to map the pages.
>
> I wasn't really arguing about adding more stuff, just that the
> documentation didn't really explain what was going on. Just saying "I'm
> checking that doing this succeeds" is definitely enough for me.
>
Ok, I will update the documentation accordingly.
>>>
>>>> + */
>>>> +static void drm_test_panic_screen_user_page(struct kunit *test)
>>>> +{
>>>> + struct drm_scanout_buffer *sb = test->priv;
>>>> + const struct drm_test_mode *params = test->param_value;
>>>> + int fb_size;
>>>> + struct page **pages;
>>>> + int i;
>>>> + int npages;
>>>> +
>>>> + sb->format = drm_format_info(params->format);
>>>> + fb_size = params->width * params->height * sb->format->cpp[0];
>>>> + npages = DIV_ROUND_UP(fb_size, PAGE_SIZE);
>>>> +
>>>> + pages = kmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
>>>> + KUNIT_ASSERT_NOT_NULL(test, pages);
>>>> +
>>>> + for (i = 0; i < npages; i++) {
>>>> + pages[i] = alloc_page(GFP_KERNEL);
>>>> + KUNIT_ASSERT_NOT_NULL(test, pages[i]);
>>>
>>> KUNIT_ASSERT_* return immediately, so you're leaking the pages array
>>> here.
>>>
>> yes, I can fix that, but is it important to not leak when the test fails?
>
> kunit tests can be compiled as module and run on live systems, so yes.
> It can also lead to subsequent test failures if you deplete the system
> of a resource the next test will need.
ok, understood, I will fix the leaks in those tests.
> Maxime
--
Jocelyn
Powered by blists - more mailing lists