[bug report] drm/vkms: Allow to configure multiple CRTCs

[bug report] drm/vkms: Allow to configure multiple CRTCs

[Dan Carpenter]

Hello José Expósito,

Commit 600df32dac40 ("drm/vkms: Allow to configure multiple CRTCs")
from Feb 18, 2025 (linux-next), leads to the following Smatch static
checker warning:

drivers/gpu/drm/vkms/tests/vkms_config_test.c:220 vkms_config_test_get_planes() error: 'plane_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:258 vkms_config_test_get_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:300 vkms_config_test_get_encoders() error: 'encoder_cfg2' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:345 vkms_config_test_get_connectors() error: 'connector_cfg2' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:672 vkms_config_test_plane_attach_crtc() error: 'overlay_cfg' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:674 vkms_config_test_plane_attach_crtc() error: 'primary_cfg' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:676 vkms_config_test_plane_attach_crtc() error: 'cursor_cfg' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:685 vkms_config_test_plane_attach_crtc() error: 'crtc_cfg' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:746 vkms_config_test_plane_get_possible_crtcs() error: 'crtc_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:746 vkms_config_test_plane_get_possible_crtcs() error: 'plane_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:748 vkms_config_test_plane_get_possible_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:810 vkms_config_test_encoder_get_possible_crtcs() error: 'crtc_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:810 vkms_config_test_encoder_get_possible_crtcs() error: 'encoder_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:812 vkms_config_test_encoder_get_possible_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:876 vkms_config_test_connector_get_possible_encoders() error: 'connector_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:876 vkms_config_test_connector_get_possible_encoders() error: 'encoder_cfg1' dereferencing possible ERR_PTR()
drivers/gpu/drm/vkms/tests/vkms_config_test.c:878 vkms_config_test_connector_get_possible_encoders() error: 'encoder_cfg2' dereferencing possible ERR_PTR()

drivers/gpu/drm/vkms/tests/vkms_config_test.c
    231 static void vkms_config_test_get_crtcs(struct kunit *test)
    232 {
    233         struct vkms_config *config;
    234         struct vkms_config_crtc *crtc_cfg;
    235         struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
    236 
    237         config = vkms_config_create("test");
    238         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
    239 
    240         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 0);
    241         vkms_config_for_each_crtc(config, crtc_cfg)
    242                 KUNIT_FAIL(test, "Unexpected CRTC");
    243 
    244         crtc_cfg1 = vkms_config_create_crtc(config);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This file has no error checking.

I didn't send an email about it at first because this is just test code so
who cares, but I was recently burned by ignoring errors so now I'm going
through a bunch of old warnings to say that, "Hey, if the author ignores the
error checking that's fine, but I'm in the clear."

    245         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);
    246         vkms_config_for_each_crtc(config, crtc_cfg) {
    247                 if (crtc_cfg != crtc_cfg1)
    248                         KUNIT_FAIL(test, "Unexpected CRTC");
    249         }
    250 
    251         crtc_cfg2 = vkms_config_create_crtc(config);
    252         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 2);
    253         vkms_config_for_each_crtc(config, crtc_cfg) {
    254                 if (crtc_cfg != crtc_cfg1 && crtc_cfg != crtc_cfg2)
    255                         KUNIT_FAIL(test, "Unexpected CRTC");
    256         }
    257 
--> 258         vkms_config_destroy_crtc(config, crtc_cfg2);
    259         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);
    260         vkms_config_for_each_crtc(config, crtc_cfg) {
    261                 if (crtc_cfg != crtc_cfg1)
    262                         KUNIT_FAIL(test, "Unexpected CRTC");
    263         }
    264 
    265         vkms_config_destroy(config);
    266 }

regards,
dan carpenter

Re: [bug report] drm/vkms: Allow to configure multiple CRTCs

[José Expósito]

Hi Dan,

Thanks for sharing these warnings.

On Thu, Aug 07, 2025 at 06:53:12PM +0300, Dan Carpenter wrote:
> Hello José Expósito,
> 
> Commit 600df32dac40 ("drm/vkms: Allow to configure multiple CRTCs")
> from Feb 18, 2025 (linux-next), leads to the following Smatch static
> checker warning:
> 
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:220 vkms_config_test_get_planes() error: 'plane_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:258 vkms_config_test_get_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:300 vkms_config_test_get_encoders() error: 'encoder_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:345 vkms_config_test_get_connectors() error: 'connector_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:672 vkms_config_test_plane_attach_crtc() error: 'overlay_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:674 vkms_config_test_plane_attach_crtc() error: 'primary_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:676 vkms_config_test_plane_attach_crtc() error: 'cursor_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:685 vkms_config_test_plane_attach_crtc() error: 'crtc_cfg' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:746 vkms_config_test_plane_get_possible_crtcs() error: 'crtc_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:746 vkms_config_test_plane_get_possible_crtcs() error: 'plane_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:748 vkms_config_test_plane_get_possible_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:810 vkms_config_test_encoder_get_possible_crtcs() error: 'crtc_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:810 vkms_config_test_encoder_get_possible_crtcs() error: 'encoder_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:812 vkms_config_test_encoder_get_possible_crtcs() error: 'crtc_cfg2' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:876 vkms_config_test_connector_get_possible_encoders() error: 'connector_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:876 vkms_config_test_connector_get_possible_encoders() error: 'encoder_cfg1' dereferencing possible ERR_PTR()
> drivers/gpu/drm/vkms/tests/vkms_config_test.c:878 vkms_config_test_connector_get_possible_encoders() error: 'encoder_cfg2' dereferencing possible ERR_PTR()
> 
> drivers/gpu/drm/vkms/tests/vkms_config_test.c
>     231 static void vkms_config_test_get_crtcs(struct kunit *test)
>     232 {
>     233         struct vkms_config *config;
>     234         struct vkms_config_crtc *crtc_cfg;
>     235         struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
>     236 
>     237         config = vkms_config_create("test");
>     238         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
>     239 
>     240         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 0);
>     241         vkms_config_for_each_crtc(config, crtc_cfg)
>     242                 KUNIT_FAIL(test, "Unexpected CRTC");
>     243 
>     244         crtc_cfg1 = vkms_config_create_crtc(config);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This file has no error checking.
> 
> I didn't send an email about it at first because this is just test code so
> who cares, but I was recently burned by ignoring errors so now I'm going
> through a bunch of old warnings to say that, "Hey, if the author ignores the
> error checking that's fine, but I'm in the clear."
> 
>     245         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);

While the "crtc_cfg1" pointer is not checked, we check that the	number 
of CRTCs matches the expected value and...

>     246         vkms_config_for_each_crtc(config, crtc_cfg) {
>     247                 if (crtc_cfg != crtc_cfg1)
>     248                         KUNIT_FAIL(test, "Unexpected CRTC");
>     249         }

...that the stored CRTC matches the one returned by vkms_config_create_crtc().

By program logic, vkms_config_create_crtc() returns an error if allocation
fails:

```
struct vkms_config_crtc *vkms_config_create_crtc(struct vkms_config *config)
{
	struct vkms_config_crtc *crtc_cfg;

	crtc_cfg = kzalloc(sizeof(*crtc_cfg), GFP_KERNEL);
	if (!crtc_cfg)
		return ERR_PTR(-ENOMEM);
```

Therefore, the current validations make sure that the pointer is valid.

In other places, for example vkms_config_test_connector_get_possible_encoders(),
the return value of vkms_config_create_connector/encoder() is indeed not
checked. It is not a big deal, since it is test code, but anyways, I'll send a
patch checking for return values.

Not because I think it can be problematic, but because test code can be used to
learn how the API works and I prefer to be explicit about its usage.

Best wishes,
Jose

>     250 
>     251         crtc_cfg2 = vkms_config_create_crtc(config);
>     252         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 2);
>     253         vkms_config_for_each_crtc(config, crtc_cfg) {
>     254                 if (crtc_cfg != crtc_cfg1 && crtc_cfg != crtc_cfg2)
>     255                         KUNIT_FAIL(test, "Unexpected CRTC");
>     256         }
>     257 
> --> 258         vkms_config_destroy_crtc(config, crtc_cfg2);
>     259         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);
>     260         vkms_config_for_each_crtc(config, crtc_cfg) {
>     261                 if (crtc_cfg != crtc_cfg1)
>     262                         KUNIT_FAIL(test, "Unexpected CRTC");
>     263         }
>     264 
>     265         vkms_config_destroy(config);
>     266 }
> 
> regards,
> dan carpenter

Re: [bug report] drm/vkms: Allow to configure multiple CRTCs

[Dan Carpenter]

On Fri, Aug 08, 2025 at 01:15:03PM +0200, José Expósito wrote:
> > drivers/gpu/drm/vkms/tests/vkms_config_test.c
> >     231 static void vkms_config_test_get_crtcs(struct kunit *test)
> >     232 {
> >     233         struct vkms_config *config;
> >     234         struct vkms_config_crtc *crtc_cfg;
> >     235         struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
> >     236 
> >     237         config = vkms_config_create("test");
> >     238         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> >     239 
> >     240         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 0);
> >     241         vkms_config_for_each_crtc(config, crtc_cfg)
> >     242                 KUNIT_FAIL(test, "Unexpected CRTC");
> >     243 
> >     244         crtc_cfg1 = vkms_config_create_crtc(config);
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This file has no error checking.
> > 
> > I didn't send an email about it at first because this is just test code so
> > who cares, but I was recently burned by ignoring errors so now I'm going
> > through a bunch of old warnings to say that, "Hey, if the author ignores the
> > error checking that's fine, but I'm in the clear."
> > 
> >     245         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);
> 
> While the "crtc_cfg1" pointer is not checked, we check that the	number 
> of CRTCs matches the expected value and...
> 

Ah yes.  That does work...  Sorry for the noise.

regards,
dan carpenter

Re: [bug report] drm/vkms: Allow to configure multiple CRTCs

[José Expósito]

Hi Dan,

On Fri, Aug 08, 2025 at 05:01:35PM +0300, Dan Carpenter wrote:
> On Fri, Aug 08, 2025 at 01:15:03PM +0200, José Expósito wrote:
> > > drivers/gpu/drm/vkms/tests/vkms_config_test.c
> > >     231 static void vkms_config_test_get_crtcs(struct kunit *test)
> > >     232 {
> > >     233         struct vkms_config *config;
> > >     234         struct vkms_config_crtc *crtc_cfg;
> > >     235         struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
> > >     236 
> > >     237         config = vkms_config_create("test");
> > >     238         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> > >     239 
> > >     240         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 0);
> > >     241         vkms_config_for_each_crtc(config, crtc_cfg)
> > >     242                 KUNIT_FAIL(test, "Unexpected CRTC");
> > >     243 
> > >     244         crtc_cfg1 = vkms_config_create_crtc(config);
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > This file has no error checking.
> > > 
> > > I didn't send an email about it at first because this is just test code so
> > > who cares, but I was recently burned by ignoring errors so now I'm going
> > > through a bunch of old warnings to say that, "Hey, if the author ignores the
> > > error checking that's fine, but I'm in the clear."
> > > 
> > >     245         KUNIT_ASSERT_EQ(test, vkms_config_get_num_crtcs(config), 1);
> > 
> > While the "crtc_cfg1" pointer is not checked, we check that the	number 
> > of CRTCs matches the expected value and...
> > 
> 
> Ah yes.  That does work...  Sorry for the noise.

No noise at all, there were other places were the check made sense.

I sent a patch fixing them:
https://lore.kernel.org/dri-devel/20250811101529.150716-1-jose.exposito89@gmail.com/T/#u

Thanks a lot for reporting this issue!!
Jose

> 
> regards,
> dan carpenter
>