| 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: <20250923-silent-mole-of-luxury-9feabc@penduick>
Date: Tue, 23 Sep 2025 11:57:25 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Jocelyn Falempe <jfalempe@...hat.com>
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 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.
> >
> > > + */
> > > +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.
Maxime
Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)
Powered by blists - more mailing lists