diff mbox series

drm/fb-helper: only unmap if buffer not null

Message ID 20210228044625.171151-1-ztong0001@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/fb-helper: only unmap if buffer not null | expand

Commit Message

Tong Zhang Feb. 28, 2021, 4:46 a.m. UTC 
drm_fbdev_cleanup() can be called when fb_helper->buffer is null, hence
fb_helper->buffer should be checked before calling
drm_client_buffer_vunmap(). This buffer is also checked in
drm_client_framebuffer_delete(), so we should also do the same thing for
drm_client_buffer_vunmap().

[  199.128742] RIP: 0010:drm_client_buffer_vunmap+0xd/0x20
[  199.129031] Code: 43 18 48 8b 53 20 49 89 45 00 49 89 55 08 5b 44 89 e0 41 5c 41 5d 41 5e 5d
c3 0f 1f 00 53 48 89 fb 48 8d 7f 10 e8 73 7d a1 ff <48> 8b 7b 10 48 8d 73 18 5b e9 75 53 fc ff 0
f 1f 44 00 00 48 b8 00
[  199.130041] RSP: 0018:ffff888103f3fc88 EFLAGS: 00010282
[  199.130329] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffffffff8214d46d
[  199.130733] RDX: 1ffffffff079c6b9 RSI: 0000000000000246 RDI: ffffffff83ce35c8
[  199.131119] RBP: ffff888103d25458 R08: 0000000000000001 R09: fffffbfff0791761
[  199.131505] R10: ffffffff83c8bb07 R11: fffffbfff0791760 R12: 0000000000000000
[  199.131891] R13: ffff888103d25468 R14: ffff888103d25418 R15: ffff888103f18120
[  199.132277] FS:  00007f36fdcbb6a0(0000) GS:ffff88815b400000(0000) knlGS:0000000000000000
[  199.132721] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  199.133033] CR2: 0000000000000010 CR3: 0000000103d26000 CR4: 00000000000006f0
[  199.133420] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  199.133807] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  199.134195] Call Trace:
[  199.134333]  drm_fbdev_cleanup+0x179/0x1a0
[  199.134562]  drm_fbdev_client_unregister+0x2b/0x40
[  199.134828]  drm_client_dev_unregister+0xa8/0x180
[  199.135088]  drm_dev_unregister+0x61/0x110
[  199.135315]  mgag200_pci_remove+0x38/0x52 [mgag200]
[  199.135586]  pci_device_remove+0x62/0xe0
[  199.135806]  device_release_driver_internal+0x148/0x270
[  199.136094]  driver_detach+0x76/0xe0
[  199.136294]  bus_remove_driver+0x7e/0x100
[  199.136521]  pci_unregister_driver+0x28/0xf0
[  199.136759]  __x64_sys_delete_module+0x268/0x300
[  199.137016]  ? __ia32_sys_delete_module+0x300/0x300
[  199.137285]  ? call_rcu+0x3e4/0x580
[  199.137481]  ? fpregs_assert_state_consistent+0x4d/0x60
[  199.137767]  ? exit_to_user_mode_prepare+0x2f/0x130
[  199.138037]  do_syscall_64+0x33/0x40
[  199.138237]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  199.138517] RIP: 0033:0x7f36fdc3dcf7

Signed-off-by: Tong Zhang <ztong0001@gmail.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thomas Zimmermann March 1, 2021, 8:26 a.m. UTC | #1 
Hi

Am 28.02.21 um 05:46 schrieb Tong Zhang:
> drm_fbdev_cleanup() can be called when fb_helper->buffer is null, hence
> fb_helper->buffer should be checked before calling
> drm_client_buffer_vunmap(). This buffer is also checked in
> drm_client_framebuffer_delete(), so we should also do the same thing for
> drm_client_buffer_vunmap().

I think a lot of drivers are affected by this problem; probably most of 
the ones that use the generic fbdev code. How did you produce the error?

What I'm more concerned about is why the buffer is NULL. Was ther eno 
hotplug event? Do you have a display attached?

Best regards
Thomas


> 
> [  199.128742] RIP: 0010:drm_client_buffer_vunmap+0xd/0x20
> [  199.129031] Code: 43 18 48 8b 53 20 49 89 45 00 49 89 55 08 5b 44 89 e0 41 5c 41 5d 41 5e 5d
> c3 0f 1f 00 53 48 89 fb 48 8d 7f 10 e8 73 7d a1 ff <48> 8b 7b 10 48 8d 73 18 5b e9 75 53 fc ff 0
> f 1f 44 00 00 48 b8 00
> [  199.130041] RSP: 0018:ffff888103f3fc88 EFLAGS: 00010282
> [  199.130329] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffffffff8214d46d
> [  199.130733] RDX: 1ffffffff079c6b9 RSI: 0000000000000246 RDI: ffffffff83ce35c8
> [  199.131119] RBP: ffff888103d25458 R08: 0000000000000001 R09: fffffbfff0791761
> [  199.131505] R10: ffffffff83c8bb07 R11: fffffbfff0791760 R12: 0000000000000000
> [  199.131891] R13: ffff888103d25468 R14: ffff888103d25418 R15: ffff888103f18120
> [  199.132277] FS:  00007f36fdcbb6a0(0000) GS:ffff88815b400000(0000) knlGS:0000000000000000
> [  199.132721] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  199.133033] CR2: 0000000000000010 CR3: 0000000103d26000 CR4: 00000000000006f0
> [  199.133420] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  199.133807] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [  199.134195] Call Trace:
> [  199.134333]  drm_fbdev_cleanup+0x179/0x1a0
> [  199.134562]  drm_fbdev_client_unregister+0x2b/0x40
> [  199.134828]  drm_client_dev_unregister+0xa8/0x180
> [  199.135088]  drm_dev_unregister+0x61/0x110
> [  199.135315]  mgag200_pci_remove+0x38/0x52 [mgag200]
> [  199.135586]  pci_device_remove+0x62/0xe0
> [  199.135806]  device_release_driver_internal+0x148/0x270
> [  199.136094]  driver_detach+0x76/0xe0
> [  199.136294]  bus_remove_driver+0x7e/0x100
> [  199.136521]  pci_unregister_driver+0x28/0xf0
> [  199.136759]  __x64_sys_delete_module+0x268/0x300
> [  199.137016]  ? __ia32_sys_delete_module+0x300/0x300
> [  199.137285]  ? call_rcu+0x3e4/0x580
> [  199.137481]  ? fpregs_assert_state_consistent+0x4d/0x60
> [  199.137767]  ? exit_to_user_mode_prepare+0x2f/0x130
> [  199.138037]  do_syscall_64+0x33/0x40
> [  199.138237]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [  199.138517] RIP: 0033:0x7f36fdc3dcf7
> 
> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b9a616737c0e..f6baa2046124 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2048,7 +2048,7 @@ static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper)
>   
>   	if (shadow)
>   		vfree(shadow);
> -	else
> +	else if (fb_helper->buffer)
>   		drm_client_buffer_vunmap(fb_helper->buffer);
>   
>   	drm_client_framebuffer_delete(fb_helper->buffer);
>
Tong Zhang March 2, 2021, 3:29 a.m. UTC | #2 
Hi Tomas,

I think the issue could be possibly caused by the following,
Please correct me if I'm wrong.

drm_fb_helper_single_fb_probe() can fail with
"Cannot find any crtc or sizes"
which will cause fb_helper->funcs->fb_probe not being called,
thus fb_helper->buffer remains NULL --
Since there could be the case that the fb_probe is never called,
a subsequent modprobe -r will cause the following
drm_client_buffer_vunmap(NULL) in drm_fbdev_cleanup()

Best,
- Tong

On Mon, Mar 1, 2021 at 3:26 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 28.02.21 um 05:46 schrieb Tong Zhang:
> > drm_fbdev_cleanup() can be called when fb_helper->buffer is null, hence
> > fb_helper->buffer should be checked before calling
> > drm_client_buffer_vunmap(). This buffer is also checked in
> > drm_client_framebuffer_delete(), so we should also do the same thing for
> > drm_client_buffer_vunmap().
>
> I think a lot of drivers are affected by this problem; probably most of
> the ones that use the generic fbdev code. How did you produce the error?
>
> What I'm more concerned about is why the buffer is NULL. Was ther eno
> hotplug event? Do you have a display attached?
>
> Best regards
> Thomas
>
>
> >
> > [  199.128742] RIP: 0010:drm_client_buffer_vunmap+0xd/0x20
> > [  199.129031] Code: 43 18 48 8b 53 20 49 89 45 00 49 89 55 08 5b 44 89 e0 41 5c 41 5d 41 5e 5d
> > c3 0f 1f 00 53 48 89 fb 48 8d 7f 10 e8 73 7d a1 ff <48> 8b 7b 10 48 8d 73 18 5b e9 75 53 fc ff 0
> > f 1f 44 00 00 48 b8 00
> > [  199.130041] RSP: 0018:ffff888103f3fc88 EFLAGS: 00010282
> > [  199.130329] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffffffff8214d46d
> > [  199.130733] RDX: 1ffffffff079c6b9 RSI: 0000000000000246 RDI: ffffffff83ce35c8
> > [  199.131119] RBP: ffff888103d25458 R08: 0000000000000001 R09: fffffbfff0791761
> > [  199.131505] R10: ffffffff83c8bb07 R11: fffffbfff0791760 R12: 0000000000000000
> > [  199.131891] R13: ffff888103d25468 R14: ffff888103d25418 R15: ffff888103f18120
> > [  199.132277] FS:  00007f36fdcbb6a0(0000) GS:ffff88815b400000(0000) knlGS:0000000000000000
> > [  199.132721] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  199.133033] CR2: 0000000000000010 CR3: 0000000103d26000 CR4: 00000000000006f0
> > [  199.133420] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [  199.133807] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [  199.134195] Call Trace:
> > [  199.134333]  drm_fbdev_cleanup+0x179/0x1a0
> > [  199.134562]  drm_fbdev_client_unregister+0x2b/0x40
> > [  199.134828]  drm_client_dev_unregister+0xa8/0x180
> > [  199.135088]  drm_dev_unregister+0x61/0x110
> > [  199.135315]  mgag200_pci_remove+0x38/0x52 [mgag200]
> > [  199.135586]  pci_device_remove+0x62/0xe0
> > [  199.135806]  device_release_driver_internal+0x148/0x270
> > [  199.136094]  driver_detach+0x76/0xe0
> > [  199.136294]  bus_remove_driver+0x7e/0x100
> > [  199.136521]  pci_unregister_driver+0x28/0xf0
> > [  199.136759]  __x64_sys_delete_module+0x268/0x300
> > [  199.137016]  ? __ia32_sys_delete_module+0x300/0x300
> > [  199.137285]  ? call_rcu+0x3e4/0x580
> > [  199.137481]  ? fpregs_assert_state_consistent+0x4d/0x60
> > [  199.137767]  ? exit_to_user_mode_prepare+0x2f/0x130
> > [  199.138037]  do_syscall_64+0x33/0x40
> > [  199.138237]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > [  199.138517] RIP: 0033:0x7f36fdc3dcf7
> >
> > Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> > ---
> >   drivers/gpu/drm/drm_fb_helper.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index b9a616737c0e..f6baa2046124 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2048,7 +2048,7 @@ static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper)
> >
> >       if (shadow)
> >               vfree(shadow);
> > -     else
> > +     else if (fb_helper->buffer)
> >               drm_client_buffer_vunmap(fb_helper->buffer);
> >
> >       drm_client_framebuffer_delete(fb_helper->buffer);
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
Thomas Zimmermann March 2, 2021, 10:06 a.m. UTC | #3 
Hi

Am 02.03.21 um 04:29 schrieb Tong Zhang:
> Hi Tomas,
> 
> I think the issue could be possibly caused by the following,
> Please correct me if I'm wrong.
> 
> drm_fb_helper_single_fb_probe() can fail with
> "Cannot find any crtc or sizes"
> which will cause fb_helper->funcs->fb_probe not being called,
> thus fb_helper->buffer remains NULL --
> Since there could be the case that the fb_probe is never called,
> a subsequent modprobe -r will cause the following
> drm_client_buffer_vunmap(NULL) in drm_fbdev_cleanup()

Thanks! I added Fixes tags to your patch and merged it into drm-misc-fixes.

Best regards
Thomas

> 
> Best,
> - Tong
> 
> On Mon, Mar 1, 2021 at 3:26 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 28.02.21 um 05:46 schrieb Tong Zhang:
>>> drm_fbdev_cleanup() can be called when fb_helper->buffer is null, hence
>>> fb_helper->buffer should be checked before calling
>>> drm_client_buffer_vunmap(). This buffer is also checked in
>>> drm_client_framebuffer_delete(), so we should also do the same thing for
>>> drm_client_buffer_vunmap().
>>
>> I think a lot of drivers are affected by this problem; probably most of
>> the ones that use the generic fbdev code. How did you produce the error?
>>
>> What I'm more concerned about is why the buffer is NULL. Was ther eno
>> hotplug event? Do you have a display attached?
>>
>> Best regards
>> Thomas
>>
>>
>>>
>>> [  199.128742] RIP: 0010:drm_client_buffer_vunmap+0xd/0x20
>>> [  199.129031] Code: 43 18 48 8b 53 20 49 89 45 00 49 89 55 08 5b 44 89 e0 41 5c 41 5d 41 5e 5d
>>> c3 0f 1f 00 53 48 89 fb 48 8d 7f 10 e8 73 7d a1 ff <48> 8b 7b 10 48 8d 73 18 5b e9 75 53 fc ff 0
>>> f 1f 44 00 00 48 b8 00
>>> [  199.130041] RSP: 0018:ffff888103f3fc88 EFLAGS: 00010282
>>> [  199.130329] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffffffff8214d46d
>>> [  199.130733] RDX: 1ffffffff079c6b9 RSI: 0000000000000246 RDI: ffffffff83ce35c8
>>> [  199.131119] RBP: ffff888103d25458 R08: 0000000000000001 R09: fffffbfff0791761
>>> [  199.131505] R10: ffffffff83c8bb07 R11: fffffbfff0791760 R12: 0000000000000000
>>> [  199.131891] R13: ffff888103d25468 R14: ffff888103d25418 R15: ffff888103f18120
>>> [  199.132277] FS:  00007f36fdcbb6a0(0000) GS:ffff88815b400000(0000) knlGS:0000000000000000
>>> [  199.132721] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [  199.133033] CR2: 0000000000000010 CR3: 0000000103d26000 CR4: 00000000000006f0
>>> [  199.133420] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> [  199.133807] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>> [  199.134195] Call Trace:
>>> [  199.134333]  drm_fbdev_cleanup+0x179/0x1a0
>>> [  199.134562]  drm_fbdev_client_unregister+0x2b/0x40
>>> [  199.134828]  drm_client_dev_unregister+0xa8/0x180
>>> [  199.135088]  drm_dev_unregister+0x61/0x110
>>> [  199.135315]  mgag200_pci_remove+0x38/0x52 [mgag200]
>>> [  199.135586]  pci_device_remove+0x62/0xe0
>>> [  199.135806]  device_release_driver_internal+0x148/0x270
>>> [  199.136094]  driver_detach+0x76/0xe0
>>> [  199.136294]  bus_remove_driver+0x7e/0x100
>>> [  199.136521]  pci_unregister_driver+0x28/0xf0
>>> [  199.136759]  __x64_sys_delete_module+0x268/0x300
>>> [  199.137016]  ? __ia32_sys_delete_module+0x300/0x300
>>> [  199.137285]  ? call_rcu+0x3e4/0x580
>>> [  199.137481]  ? fpregs_assert_state_consistent+0x4d/0x60
>>> [  199.137767]  ? exit_to_user_mode_prepare+0x2f/0x130
>>> [  199.138037]  do_syscall_64+0x33/0x40
>>> [  199.138237]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>>> [  199.138517] RIP: 0033:0x7f36fdc3dcf7
>>>
>>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
>>> ---
>>>    drivers/gpu/drm/drm_fb_helper.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index b9a616737c0e..f6baa2046124 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -2048,7 +2048,7 @@ static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper)
>>>
>>>        if (shadow)
>>>                vfree(shadow);
>>> -     else
>>> +     else if (fb_helper->buffer)
>>>                drm_client_buffer_vunmap(fb_helper->buffer);
>>>
>>>        drm_client_framebuffer_delete(fb_helper->buffer);
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index b9a616737c0e..f6baa2046124 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2048,7 +2048,7 @@  static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper)
 
 	if (shadow)
 		vfree(shadow);
-	else
+	else if (fb_helper->buffer)
 		drm_client_buffer_vunmap(fb_helper->buffer);
 
 	drm_client_framebuffer_delete(fb_helper->buffer);