| Message ID | 20180929171200.24391-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/cma-helper: Fix crash in fbdev error path | expand |
On Sat, Sep 29, 2018 at 07:12:00PM +0200, Noralf Trønnes wrote: > Sergey Suloev reported a crash happening in drm_client_dev_hotplug() > when fbdev had failed to register. > > [ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create component debugfs directory > [ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok > [ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name! > [ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4]) > [ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4]) > [ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4]) > [ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4]) > [ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4]) > [ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4]) > [ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4]) > [ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4]) > [ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0 > [ 9.297464] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 9.304600] [drm] Driver supports precise vblank timestamp query. > [ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup [drm_kms_helper]] *ERROR* Failed to set fbdev configuration > [ 10.404937] Unable to handle kernel paging request at virtual address 00330a656369768a > [ 10.441620] [00330a656369768a] address between user and kernel address ranges > [ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP > [ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper cfg80211 drm rfkill smsc95xx brcmutil usbnet drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables x_tables ipv6 > [ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted 4.19.0-rc5 #3 > [ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT) > [ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper] > [ 10.596515] Process kworker/2:1 (pid: 45, stack limit = 0x000000007e8924dc) > [ 10.603590] Call trace: > [ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm] > [ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper] > [ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper] > [ 10.623616] process_one_work+0x1c8/0x318 > [ 10.627695] worker_thread+0x48/0x428 > [ 10.631420] kthread+0xf8/0x128 > [ 10.634615] ret_from_fork+0x10/0x18 > [ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21) > [ 10.644456] ---[ end trace c75b4a4b0e141908 ]--- > > The reason for this is that drm_fbdev_cma_init() removes the drm_client > when fbdev registration fails, but it doesn't remove the client from the > drm_device client list. So the client list now has a pointer that points > into the unknown and we have a 'use after free' situation. > > It was decided during the drm_client API discussions that a client should > not be allowed to remove itself. This was based on the experience with > the fbdev/fbcon locking horror show. > > So the solution is to just leave the drm_client hanging around doing > nothing (it doesn't have any callbacks), and let it be removed when > drm_dev_unregister() is called. Slightly cleaner solution: Split drm_client_new into drm_client_alloc() and drm_client_add(). Call the later only when initialization succeeds, and only _add() will add it to the device list. But this here gets the job done for -fixes, so: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> But feel free to respin with the _alloc()/_add() idea. > > Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") > Reported-by: Sergey Suloev <ssuloev@orpaltech.com> > Cc: Stefan Wahren <stefan.wahren@i2se.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > > This bug is in 4.19 so it should go into drm-misc-fixes right? > (I have never applied a fix before) Yup. If you decide to push this, and do the _alloc/add() refactor afterwards you need to request a backmerge from drm-misc maintainers (since the refactor should go into drm-misc ofc). Cheers, Daniel > > Noralf. > > drivers/gpu/drm/drm_fb_cma_helper.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index 47e0e2f6642d..d82bb4776e3c 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -168,22 +168,21 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, > fb_helper = &fbdev_cma->fb_helper; > > ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL); > - if (ret) > - goto err_free; > + if (ret) { > + kfree(fbdev_cma); > + return ERR_PTR(ret); > + } > > ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_cma_helper_funcs, > preferred_bpp, max_conn_count); > if (ret) > - goto err_client_put; > + /* > + * A drm_client is not allowed to remove itself, so it has to > + * stay around until drm_dev_unregister(). > + */ > + return ERR_PTR(ret); > > return fbdev_cma; > - > -err_client_put: > - drm_client_release(&fb_helper->client); > -err_free: > - kfree(fbdev_cma); > - > - return ERR_PTR(ret); > } > EXPORT_SYMBOL_GPL(drm_fbdev_cma_init); > > -- > 2.15.1 >
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..d82bb4776e3c 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -168,22 +168,21 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, fb_helper = &fbdev_cma->fb_helper; ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL); - if (ret) - goto err_free; + if (ret) { + kfree(fbdev_cma); + return ERR_PTR(ret); + } ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_cma_helper_funcs, preferred_bpp, max_conn_count); if (ret) - goto err_client_put; + /* + * A drm_client is not allowed to remove itself, so it has to + * stay around until drm_dev_unregister(). + */ + return ERR_PTR(ret); return fbdev_cma; - -err_client_put: - drm_client_release(&fb_helper->client); -err_free: - kfree(fbdev_cma); - - return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
Sergey Suloev reported a crash happening in drm_client_dev_hotplug() when fbdev had failed to register. [ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create component debugfs directory [ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok [ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name! [ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4]) [ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4]) [ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4]) [ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4]) [ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4]) [ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0 [ 9.297464] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 9.304600] [drm] Driver supports precise vblank timestamp query. [ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup [drm_kms_helper]] *ERROR* Failed to set fbdev configuration [ 10.404937] Unable to handle kernel paging request at virtual address 00330a656369768a [ 10.441620] [00330a656369768a] address between user and kernel address ranges [ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper cfg80211 drm rfkill smsc95xx brcmutil usbnet drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables x_tables ipv6 [ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted 4.19.0-rc5 #3 [ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT) [ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper] [ 10.596515] Process kworker/2:1 (pid: 45, stack limit = 0x000000007e8924dc) [ 10.603590] Call trace: [ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm] [ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper] [ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper] [ 10.623616] process_one_work+0x1c8/0x318 [ 10.627695] worker_thread+0x48/0x428 [ 10.631420] kthread+0xf8/0x128 [ 10.634615] ret_from_fork+0x10/0x18 [ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21) [ 10.644456] ---[ end trace c75b4a4b0e141908 ]--- The reason for this is that drm_fbdev_cma_init() removes the drm_client when fbdev registration fails, but it doesn't remove the client from the drm_device client list. So the client list now has a pointer that points into the unknown and we have a 'use after free' situation. It was decided during the drm_client API discussions that a client should not be allowed to remove itself. This was based on the experience with the fbdev/fbcon locking horror show. So the solution is to just leave the drm_client hanging around doing nothing (it doesn't have any callbacks), and let it be removed when drm_dev_unregister() is called. Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation") Reported-by: Sergey Suloev <ssuloev@orpaltech.com> Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Eric Anholt <eric@anholt.net> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- This bug is in 4.19 so it should go into drm-misc-fixes right? (I have never applied a fix before) Noralf. drivers/gpu/drm/drm_fb_cma_helper.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)