Date:   Mon, 17 Jul 2023 13:08:19 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Javier Martinez Canillas <javierm@...hat.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/ssd130x: Fix an oops when attempting to update a
 disabled plane

Hi

Am 17.07.23 um 11:04 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Mon, Jul 17, 2023 at 10:48 AM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>> Am 13.07.23 um 18:32 schrieb Javier Martinez Canillas:
>>> Geert reports that the following NULL pointer dereference happens for him
>>> after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each
>>> plane update"):
>>>
>>>       [drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
>>>       ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1)
>>>       and format(R1   little-endian (0x20203152))
>>>       Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>>       Oops [#1]
>>>       CPU: 0 PID: 1 Comm: swapper Not tainted
>>>       6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565
>>>       epc : ssd130x_update_rect.isra.0+0x13c/0x340
>>>        ra : ssd130x_update_rect.isra.0+0x2bc/0x340
>>>       ...
>>>       status: 00000120 badaddr: 00000000 cause: 0000000f
>>>       [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
>>>       [<c0304200>] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
>>>       [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
>>>       [<c02f9314>] drm_atomic_helper_commit_tail+0x5c/0xb4
>>>       [<c02f94fc>] commit_tail+0x190/0x1b8
>>>       [<c02f99fc>] drm_atomic_helper_commit+0x194/0x1c0
>>>       [<c02c5d00>] drm_atomic_commit+0xa4/0xe4
>>>       [<c02cce40>] drm_client_modeset_commit_atomic+0x244/0x278
>>>       [<c02ccef0>] drm_client_modeset_commit_locked+0x7c/0x1bc
>>>       [<c02cd064>] drm_client_modeset_commit+0x34/0x64
>>>       [<c0301a78>] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8
>>>       [<c0303424>] drm_fb_helper_set_par+0x38/0x58
>>>       [<c027c410>] fbcon_init+0x294/0x534
>>>       ...
>>>
>>> The problem is that fbcon calls fbcon_init() which triggers a DRM modeset
>>> and this leads to drm_atomic_helper_commit_planes() attempting to commit
>>> the atomic state for all planes, even the ones whose CRTC is not enabled.
>>>
>>> Since the primary plane buffer is allocated in the encoder .atomic_enable
>>> callback, this happens after that initial modeset commit and leads to the
>>> mentioned NULL pointer dereference.
>>>
>>> Fix this by not using the default drm_atomic_helper_commit_tail() helper,
>>> but instead the drm_atomic_helper_commit_tail_rpm() function that doesn't
>>> attempt to commit the atomic state for planes related to inactive CRTCs.
>>>
>>> Reported-by: Geert Uytterhoeven <geert@...ux-m68k.org>
>>> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
> 
>>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>>> @@ -795,6 +795,10 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
>>>        .atomic_commit = drm_atomic_helper_commit,
>>>    };
>>>
>>> +static const struct drm_mode_config_helper_funcs ssd130x_mode_config_helpers = {
>>> +     .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>>> +};
>>> +
>>
>> After some discussion on IRC, I'd suggest to allocate the buffer
>> somewhere within probe. So it will always be there when the plane code runs.
>>
>> A full fix would be to allocate the buffer memory as part of the plane
>> state and/or the plane's atomic_check. That's a bit more complicated if
>> you want to shared the buffer memory across plane updates.
> 
> Note that actually two buffers are involved: data_array (monochrome,
> needed for each update), and buffer (R8, only needed when converting
> from XR24 to R1).
> 
> For the former, I agree, as it's always needed.
> For the latter, I'm afraid it would set a bad example: while allocating
> a possibly-unused buffer doesn't hurt for small displays, it would
> mean wasting 1 MiB in e.g. the repaper driver (once it has gained
> support for R1 ;^).

Let me explain: a real DRM-ideomatic solution would allocate the 
required buffers at the correct size in the plane's atomic check. The 
pointer would be stored in the plane state and later be free'd as part 
of releasing that plane_state. But as this is temporary memory for the 
plane update, so it can be shared among plane states. Keeping track of 
the references requires a bit of work though.

Repaper appears to allocate buffer memory on each update, so anything is 
an improvement there.

Best regards
Thomsa

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists