diff mbox series

[v3] drm/ssd130x: Fix an oops when attempting to update a disabled plane

Message ID 20230717163027.480918-1-javierm@redhat.com
State New
Headers
Series [v3] drm/ssd130x: Fix an oops when attempting to update a disabled plane |

Commit Message

Javier Martinez Canillas July 17, 2023, 4:30 p.m. UTC 
  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 allocating the buffers in the struct drm_plane_helper_funcs
.begin_fb_access callback and free them in to .end_fb_access callback,
instead of doing it when the encoder is enabled.

Fixes: 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v3:
- Move the buffers allocation to the plane helper funcs .begin_fb_access
  and the free to .end_fb_access callbacks.
- Always allocate intermediate buffer because is use in ssd130x_clear_screen().
- Don't allocate the buffers as device managed resources.

Changes in v2:
- Move the buffers allocation to .fb_create instead of preventing atomic
  updates for disable planes.
- Don't allocate the intermediate buffer when using the native R1 format.
- Allocate buffers as device managed resources.

 drivers/gpu/drm/solomon/ssd130x.c | 33 ++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Maxime Ripard July 20, 2023, 10:02 a.m. UTC | #1 
Hi,

On Mon, Jul 17, 2023 at 06:30:22PM +0200, Javier Martinez Canillas wrote:
> 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.

I think that's where the problem lies: you must not allocate a buffer in
atomic_enable.

After drm_atomic_helper_swap_state(), the new commit is being applied
and you aren't allowed to fail, and an allocation can fail.

Everything needs to be prepared by the time _swap_state is called, and
it's one of the point of atomic_check.

So you need to allocate your buffer there, and use it in whatever
atomic_commit related hook you need it in.

The typical way of doing this would be to create a custom state
structure that embeds the global one, create your own reset,
atomic_duplicate_state and atomic_destroy_state implementations, and
store the buffer pointer there.

Maxime
Javier Martinez Canillas July 20, 2023, 10:42 a.m. UTC | #2 
Maxime Ripard <mripard@redhat.com> writes:

Hello Maxime,

Thanks a lot for your feedback.

> Hi,
>
> On Mon, Jul 17, 2023 at 06:30:22PM +0200, Javier Martinez Canillas wrote:
>> 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.
>
> I think that's where the problem lies: you must not allocate a buffer in
> atomic_enable.
>
> After drm_atomic_helper_swap_state(), the new commit is being applied
> and you aren't allowed to fail, and an allocation can fail.
>
> Everything needs to be prepared by the time _swap_state is called, and
> it's one of the point of atomic_check.
>
> So you need to allocate your buffer there, and use it in whatever
> atomic_commit related hook you need it in.
>
> The typical way of doing this would be to create a custom state
> structure that embeds the global one, create your own reset,
> atomic_duplicate_state and atomic_destroy_state implementations, and
> store the buffer pointer there.
>

I see. That makes totally sense to me. I'll do that in v4 then!

> Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index afb08a8aa9fc..be68b63200a0 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -612,6 +612,30 @@  static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	return ret;
 }
 
+static int ssd130x_primary_plane_helper_begin_fb_access(struct drm_plane *plane,
+							struct drm_plane_state *state)
+{
+	struct drm_device *drm = plane->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	int ret = ssd130x_buf_alloc(ssd130x);
+
+	if (ret)
+		return ret;
+
+	return drm_gem_begin_shadow_fb_access(plane, state);
+}
+
+static void ssd130x_primary_plane_helper_end_fb_access(struct drm_plane *plane,
+						       struct drm_plane_state *state)
+{
+	struct drm_device *drm = plane->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+
+	ssd130x_buf_free(ssd130x);
+
+	return drm_gem_end_shadow_fb_access(plane, state);
+}
+
 static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						       struct drm_atomic_state *state)
 {
@@ -656,7 +680,8 @@  static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
-	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.begin_fb_access = ssd130x_primary_plane_helper_begin_fb_access,
+	.end_fb_access = ssd130x_primary_plane_helper_end_fb_access,
 	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
 	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
@@ -719,10 +744,6 @@  static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
 	if (ret)
 		goto power_off;
 
-	ret = ssd130x_buf_alloc(ssd130x);
-	if (ret)
-		goto power_off;
-
 	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
 
 	backlight_enable(ssd130x->bl_dev);
@@ -744,8 +765,6 @@  static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder,
 
 	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF);
 
-	ssd130x_buf_free(ssd130x);
-
 	ssd130x_power_off(ssd130x);
 }