| Message ID | 20181001194536.57756-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/cma-helper: Fix crash in fbdev error path | expand |
Den 01.10.2018 21.45, skrev Noralf Trønnes: > 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. > > Split drm_client_new() into drm_client_init() and drm_client_add() to fix > removal in the error path. > > 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> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- Applied to drm-misc-fixes, thanks for the review. Noralf. > Changes since version 2: > - Fix drm_client_new() ref in docs (Daniel) > - Expand drm_client_add() docs (Daniel) > > Changes since version 1: > - Split drm_client_new() instead of letting drm_client hang around (Daniel) > > drivers/gpu/drm/drm_client.c | 35 ++++++++++++++++++++++++++--------- > drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++- > drivers/gpu/drm/drm_fb_helper.c | 4 +++- > include/drm/drm_client.h | 5 +++-- > 4 files changed, 35 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > index 17d9a64e885e..fc03d26fcacc 100644 > --- a/drivers/gpu/drm/drm_client.c > +++ b/drivers/gpu/drm/drm_client.c > @@ -63,20 +63,21 @@ static void drm_client_close(struct drm_client_dev *client) > EXPORT_SYMBOL(drm_client_close); > > /** > - * drm_client_new - Create a DRM client > + * drm_client_init - Initialise a DRM client > * @dev: DRM device > * @client: DRM client > * @name: Client name > * @funcs: DRM client functions (optional) > * > + * This initialises the client and opens a &drm_file. Use drm_client_add() to complete the process. > * The caller needs to hold a reference on @dev before calling this function. > * The client is freed when the &drm_device is unregistered. See drm_client_release(). > * > * Returns: > * Zero on success or negative error code on failure. > */ > -int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, > - const char *name, const struct drm_client_funcs *funcs) > +int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, > + const char *name, const struct drm_client_funcs *funcs) > { > int ret; > > @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, > if (ret) > goto err_put_module; > > - mutex_lock(&dev->clientlist_mutex); > - list_add(&client->list, &dev->clientlist); > - mutex_unlock(&dev->clientlist_mutex); > - > drm_dev_get(dev); > > return 0; > @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, > > return ret; > } > -EXPORT_SYMBOL(drm_client_new); > +EXPORT_SYMBOL(drm_client_init); > + > +/** > + * drm_client_add - Add client to the device list > + * @client: DRM client > + * > + * Add the client to the &drm_device client list to activate its callbacks. > + * @client must be initialized by a call to drm_client_init(). After > + * drm_client_add() it is no longer permissible to call drm_client_release() > + * directly (outside the unregister callback), instead cleanup will happen > + * automatically on driver unload. > + */ > +void drm_client_add(struct drm_client_dev *client) > +{ > + struct drm_device *dev = client->dev; > + > + mutex_lock(&dev->clientlist_mutex); > + list_add(&client->list, &dev->clientlist); > + mutex_unlock(&dev->clientlist_mutex); > +} > +EXPORT_SYMBOL(drm_client_add); > > /** > * drm_client_release - Release DRM client resources > * @client: DRM client > * > - * Releases resources by closing the &drm_file that was opened by drm_client_new(). > + * Releases resources by closing the &drm_file that was opened by drm_client_init(). > * It is called automatically if the &drm_client_funcs.unregister callback is _not_ set. > * > * This function should only be called from the unregister callback. An exception > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index 47e0e2f6642d..fb0dfc62b1b6 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -167,7 +167,7 @@ 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); > + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL); > if (ret) > goto err_free; > > @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, > if (ret) > goto err_client_put; > > + drm_client_add(&fb_helper->client); > + > return fbdev_cma; > > err_client_put: > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index a504a5e05676..e9095537e0c0 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp) > if (!fb_helper) > return -ENOMEM; > > - ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs); > + ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs); > if (ret) { > kfree(fb_helper); > DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret); > return ret; > } > > + drm_client_add(&fb_helper->client); > + > fb_helper->preferred_bpp = preferred_bpp; > > ret = drm_fbdev_client_hotplug(&fb_helper->client); > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > index 989f8e52864d..971bb7853776 100644 > --- a/include/drm/drm_client.h > +++ b/include/drm/drm_client.h > @@ -87,9 +87,10 @@ struct drm_client_dev { > struct drm_file *file; > }; > > -int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, > - const char *name, const struct drm_client_funcs *funcs); > +int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, > + const char *name, const struct drm_client_funcs *funcs); > void drm_client_release(struct drm_client_dev *client); > +void drm_client_add(struct drm_client_dev *client); > > void drm_client_dev_unregister(struct drm_device *dev); > void drm_client_dev_hotplug(struct drm_device *dev);
This doesn't seem to fix the initially reported issue. The problem still exists with 4.19. On 10/2/18 2:14 PM, Noralf Trønnes wrote: > > Den 01.10.2018 21.45, skrev Noralf Trønnes: >> 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. >> >> Split drm_client_new() into drm_client_init() and drm_client_add() to >> fix >> removal in the error path. >> >> 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> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- > > Applied to drm-misc-fixes, thanks for the review. > > Noralf. > >> Changes since version 2: >> - Fix drm_client_new() ref in docs (Daniel) >> - Expand drm_client_add() docs (Daniel) >> >> Changes since version 1: >> - Split drm_client_new() instead of letting drm_client hang around >> (Daniel) >> >> drivers/gpu/drm/drm_client.c | 35 >> ++++++++++++++++++++++++++--------- >> drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++- >> drivers/gpu/drm/drm_fb_helper.c | 4 +++- >> include/drm/drm_client.h | 5 +++-- >> 4 files changed, 35 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 17d9a64e885e..fc03d26fcacc 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -63,20 +63,21 @@ static void drm_client_close(struct >> drm_client_dev *client) >> EXPORT_SYMBOL(drm_client_close); >> /** >> - * drm_client_new - Create a DRM client >> + * drm_client_init - Initialise a DRM client >> * @dev: DRM device >> * @client: DRM client >> * @name: Client name >> * @funcs: DRM client functions (optional) >> * >> + * This initialises the client and opens a &drm_file. Use >> drm_client_add() to complete the process. >> * The caller needs to hold a reference on @dev before calling this >> function. >> * The client is freed when the &drm_device is unregistered. See >> drm_client_release(). >> * >> * Returns: >> * Zero on success or negative error code on failure. >> */ >> -int drm_client_new(struct drm_device *dev, struct drm_client_dev >> *client, >> - const char *name, const struct drm_client_funcs *funcs) >> +int drm_client_init(struct drm_device *dev, struct drm_client_dev >> *client, >> + const char *name, const struct drm_client_funcs *funcs) >> { >> int ret; >> @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, >> struct drm_client_dev *client, >> if (ret) >> goto err_put_module; >> - mutex_lock(&dev->clientlist_mutex); >> - list_add(&client->list, &dev->clientlist); >> - mutex_unlock(&dev->clientlist_mutex); >> - >> drm_dev_get(dev); >> return 0; >> @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, >> struct drm_client_dev *client, >> return ret; >> } >> -EXPORT_SYMBOL(drm_client_new); >> +EXPORT_SYMBOL(drm_client_init); >> + >> +/** >> + * drm_client_add - Add client to the device list >> + * @client: DRM client >> + * >> + * Add the client to the &drm_device client list to activate its >> callbacks. >> + * @client must be initialized by a call to drm_client_init(). After >> + * drm_client_add() it is no longer permissible to call >> drm_client_release() >> + * directly (outside the unregister callback), instead cleanup will >> happen >> + * automatically on driver unload. >> + */ >> +void drm_client_add(struct drm_client_dev *client) >> +{ >> + struct drm_device *dev = client->dev; >> + >> + mutex_lock(&dev->clientlist_mutex); >> + list_add(&client->list, &dev->clientlist); >> + mutex_unlock(&dev->clientlist_mutex); >> +} >> +EXPORT_SYMBOL(drm_client_add); >> /** >> * drm_client_release - Release DRM client resources >> * @client: DRM client >> * >> - * Releases resources by closing the &drm_file that was opened by >> drm_client_new(). >> + * Releases resources by closing the &drm_file that was opened by >> drm_client_init(). >> * It is called automatically if the &drm_client_funcs.unregister >> callback is _not_ set. >> * >> * This function should only be called from the unregister >> callback. An exception >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c >> b/drivers/gpu/drm/drm_fb_cma_helper.c >> index 47e0e2f6642d..fb0dfc62b1b6 100644 >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c >> @@ -167,7 +167,7 @@ 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); >> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL); >> if (ret) >> goto err_free; >> @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct >> drm_device *dev, >> if (ret) >> goto err_client_put; >> + drm_client_add(&fb_helper->client); >> + >> return fbdev_cma; >> err_client_put: >> diff --git a/drivers/gpu/drm/drm_fb_helper.c >> b/drivers/gpu/drm/drm_fb_helper.c >> index a504a5e05676..e9095537e0c0 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device >> *dev, unsigned int preferred_bpp) >> if (!fb_helper) >> return -ENOMEM; >> - ret = drm_client_new(dev, &fb_helper->client, "fbdev", >> &drm_fbdev_client_funcs); >> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", >> &drm_fbdev_client_funcs); >> if (ret) { >> kfree(fb_helper); >> DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", >> ret); >> return ret; >> } >> + drm_client_add(&fb_helper->client); >> + >> fb_helper->preferred_bpp = preferred_bpp; >> ret = drm_fbdev_client_hotplug(&fb_helper->client); >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index 989f8e52864d..971bb7853776 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -87,9 +87,10 @@ struct drm_client_dev { >> struct drm_file *file; >> }; >> -int drm_client_new(struct drm_device *dev, struct drm_client_dev >> *client, >> - const char *name, const struct drm_client_funcs *funcs); >> +int drm_client_init(struct drm_device *dev, struct drm_client_dev >> *client, >> + const char *name, const struct drm_client_funcs *funcs); >> void drm_client_release(struct drm_client_dev *client); >> +void drm_client_add(struct drm_client_dev *client); >> void drm_client_dev_unregister(struct drm_device *dev); >> void drm_client_dev_hotplug(struct drm_device *dev); >
To be exact, the issue does not happen in case I use default kernel config but only with the custom one. On 10/2/18 2:14 PM, Noralf Trønnes wrote: > > Den 01.10.2018 21.45, skrev Noralf Trønnes: >> 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. >> >> Split drm_client_new() into drm_client_init() and drm_client_add() to >> fix >> removal in the error path. >> >> 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> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- > > Applied to drm-misc-fixes, thanks for the review. > > Noralf. > >> Changes since version 2: >> - Fix drm_client_new() ref in docs (Daniel) >> - Expand drm_client_add() docs (Daniel) >> >> Changes since version 1: >> - Split drm_client_new() instead of letting drm_client hang around >> (Daniel) >> >> drivers/gpu/drm/drm_client.c | 35 >> ++++++++++++++++++++++++++--------- >> drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++- >> drivers/gpu/drm/drm_fb_helper.c | 4 +++- >> include/drm/drm_client.h | 5 +++-- >> 4 files changed, 35 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 17d9a64e885e..fc03d26fcacc 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -63,20 +63,21 @@ static void drm_client_close(struct >> drm_client_dev *client) >> EXPORT_SYMBOL(drm_client_close); >> /** >> - * drm_client_new - Create a DRM client >> + * drm_client_init - Initialise a DRM client >> * @dev: DRM device >> * @client: DRM client >> * @name: Client name >> * @funcs: DRM client functions (optional) >> * >> + * This initialises the client and opens a &drm_file. Use >> drm_client_add() to complete the process. >> * The caller needs to hold a reference on @dev before calling this >> function. >> * The client is freed when the &drm_device is unregistered. See >> drm_client_release(). >> * >> * Returns: >> * Zero on success or negative error code on failure. >> */ >> -int drm_client_new(struct drm_device *dev, struct drm_client_dev >> *client, >> - const char *name, const struct drm_client_funcs *funcs) >> +int drm_client_init(struct drm_device *dev, struct drm_client_dev >> *client, >> + const char *name, const struct drm_client_funcs *funcs) >> { >> int ret; >> @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, >> struct drm_client_dev *client, >> if (ret) >> goto err_put_module; >> - mutex_lock(&dev->clientlist_mutex); >> - list_add(&client->list, &dev->clientlist); >> - mutex_unlock(&dev->clientlist_mutex); >> - >> drm_dev_get(dev); >> return 0; >> @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, >> struct drm_client_dev *client, >> return ret; >> } >> -EXPORT_SYMBOL(drm_client_new); >> +EXPORT_SYMBOL(drm_client_init); >> + >> +/** >> + * drm_client_add - Add client to the device list >> + * @client: DRM client >> + * >> + * Add the client to the &drm_device client list to activate its >> callbacks. >> + * @client must be initialized by a call to drm_client_init(). After >> + * drm_client_add() it is no longer permissible to call >> drm_client_release() >> + * directly (outside the unregister callback), instead cleanup will >> happen >> + * automatically on driver unload. >> + */ >> +void drm_client_add(struct drm_client_dev *client) >> +{ >> + struct drm_device *dev = client->dev; >> + >> + mutex_lock(&dev->clientlist_mutex); >> + list_add(&client->list, &dev->clientlist); >> + mutex_unlock(&dev->clientlist_mutex); >> +} >> +EXPORT_SYMBOL(drm_client_add); >> /** >> * drm_client_release - Release DRM client resources >> * @client: DRM client >> * >> - * Releases resources by closing the &drm_file that was opened by >> drm_client_new(). >> + * Releases resources by closing the &drm_file that was opened by >> drm_client_init(). >> * It is called automatically if the &drm_client_funcs.unregister >> callback is _not_ set. >> * >> * This function should only be called from the unregister >> callback. An exception >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c >> b/drivers/gpu/drm/drm_fb_cma_helper.c >> index 47e0e2f6642d..fb0dfc62b1b6 100644 >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c >> @@ -167,7 +167,7 @@ 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); >> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL); >> if (ret) >> goto err_free; >> @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct >> drm_device *dev, >> if (ret) >> goto err_client_put; >> + drm_client_add(&fb_helper->client); >> + >> return fbdev_cma; >> err_client_put: >> diff --git a/drivers/gpu/drm/drm_fb_helper.c >> b/drivers/gpu/drm/drm_fb_helper.c >> index a504a5e05676..e9095537e0c0 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device >> *dev, unsigned int preferred_bpp) >> if (!fb_helper) >> return -ENOMEM; >> - ret = drm_client_new(dev, &fb_helper->client, "fbdev", >> &drm_fbdev_client_funcs); >> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", >> &drm_fbdev_client_funcs); >> if (ret) { >> kfree(fb_helper); >> DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", >> ret); >> return ret; >> } >> + drm_client_add(&fb_helper->client); >> + >> fb_helper->preferred_bpp = preferred_bpp; >> ret = drm_fbdev_client_hotplug(&fb_helper->client); >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index 989f8e52864d..971bb7853776 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -87,9 +87,10 @@ struct drm_client_dev { >> struct drm_file *file; >> }; >> -int drm_client_new(struct drm_device *dev, struct drm_client_dev >> *client, >> - const char *name, const struct drm_client_funcs *funcs); >> +int drm_client_init(struct drm_device *dev, struct drm_client_dev >> *client, >> + const char *name, const struct drm_client_funcs *funcs); >> void drm_client_release(struct drm_client_dev *client); >> +void drm_client_add(struct drm_client_dev *client); >> void drm_client_dev_unregister(struct drm_device *dev); >> void drm_client_dev_hotplug(struct drm_device *dev); >
On Sun, Nov 04, 2018 at 08:10:57PM +0300, Sergey Suloev wrote: > To be exact, the issue does not happen in case I use default kernel config > but only with the custom one. Can you pls grab a new backtrace on latest kernels? Thanks, Daniel > > > On 10/2/18 2:14 PM, Noralf Trønnes wrote: > > > > Den 01.10.2018 21.45, skrev Noralf Trønnes: > > > 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. > > > > > > Split drm_client_new() into drm_client_init() and drm_client_add() > > > to fix > > > removal in the error path. > > > > > > 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> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > > Applied to drm-misc-fixes, thanks for the review. > > > > Noralf. > > > > > Changes since version 2: > > > - Fix drm_client_new() ref in docs (Daniel) > > > - Expand drm_client_add() docs (Daniel) > > > > > > Changes since version 1: > > > - Split drm_client_new() instead of letting drm_client hang around > > > (Daniel) > > > > > > drivers/gpu/drm/drm_client.c | 35 > > > ++++++++++++++++++++++++++--------- > > > drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++- > > > drivers/gpu/drm/drm_fb_helper.c | 4 +++- > > > include/drm/drm_client.h | 5 +++-- > > > 4 files changed, 35 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > > > index 17d9a64e885e..fc03d26fcacc 100644 > > > --- a/drivers/gpu/drm/drm_client.c > > > +++ b/drivers/gpu/drm/drm_client.c > > > @@ -63,20 +63,21 @@ static void drm_client_close(struct > > > drm_client_dev *client) > > > EXPORT_SYMBOL(drm_client_close); > > > /** > > > - * drm_client_new - Create a DRM client > > > + * drm_client_init - Initialise a DRM client > > > * @dev: DRM device > > > * @client: DRM client > > > * @name: Client name > > > * @funcs: DRM client functions (optional) > > > * > > > + * This initialises the client and opens a &drm_file. Use > > > drm_client_add() to complete the process. > > > * The caller needs to hold a reference on @dev before calling > > > this function. > > > * The client is freed when the &drm_device is unregistered. See > > > drm_client_release(). > > > * > > > * Returns: > > > * Zero on success or negative error code on failure. > > > */ > > > -int drm_client_new(struct drm_device *dev, struct drm_client_dev > > > *client, > > > - const char *name, const struct drm_client_funcs *funcs) > > > +int drm_client_init(struct drm_device *dev, struct drm_client_dev > > > *client, > > > + const char *name, const struct drm_client_funcs *funcs) > > > { > > > int ret; > > > @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, > > > struct drm_client_dev *client, > > > if (ret) > > > goto err_put_module; > > > - mutex_lock(&dev->clientlist_mutex); > > > - list_add(&client->list, &dev->clientlist); > > > - mutex_unlock(&dev->clientlist_mutex); > > > - > > > drm_dev_get(dev); > > > return 0; > > > @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, > > > struct drm_client_dev *client, > > > return ret; > > > } > > > -EXPORT_SYMBOL(drm_client_new); > > > +EXPORT_SYMBOL(drm_client_init); > > > + > > > +/** > > > + * drm_client_add - Add client to the device list > > > + * @client: DRM client > > > + * > > > + * Add the client to the &drm_device client list to activate its > > > callbacks. > > > + * @client must be initialized by a call to drm_client_init(). After > > > + * drm_client_add() it is no longer permissible to call > > > drm_client_release() > > > + * directly (outside the unregister callback), instead cleanup will > > > happen > > > + * automatically on driver unload. > > > + */ > > > +void drm_client_add(struct drm_client_dev *client) > > > +{ > > > + struct drm_device *dev = client->dev; > > > + > > > + mutex_lock(&dev->clientlist_mutex); > > > + list_add(&client->list, &dev->clientlist); > > > + mutex_unlock(&dev->clientlist_mutex); > > > +} > > > +EXPORT_SYMBOL(drm_client_add); > > > /** > > > * drm_client_release - Release DRM client resources > > > * @client: DRM client > > > * > > > - * Releases resources by closing the &drm_file that was opened by > > > drm_client_new(). > > > + * Releases resources by closing the &drm_file that was opened by > > > drm_client_init(). > > > * It is called automatically if the &drm_client_funcs.unregister > > > callback is _not_ set. > > > * > > > * This function should only be called from the unregister > > > callback. An exception > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c > > > b/drivers/gpu/drm/drm_fb_cma_helper.c > > > index 47e0e2f6642d..fb0dfc62b1b6 100644 > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > > > @@ -167,7 +167,7 @@ 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); > > > + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL); > > > if (ret) > > > goto err_free; > > > @@ -176,6 +176,8 @@ struct drm_fbdev_cma > > > *drm_fbdev_cma_init(struct drm_device *dev, > > > if (ret) > > > goto err_client_put; > > > + drm_client_add(&fb_helper->client); > > > + > > > return fbdev_cma; > > > err_client_put: > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > > b/drivers/gpu/drm/drm_fb_helper.c > > > index a504a5e05676..e9095537e0c0 100644 > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct > > > drm_device *dev, unsigned int preferred_bpp) > > > if (!fb_helper) > > > return -ENOMEM; > > > - ret = drm_client_new(dev, &fb_helper->client, "fbdev", > > > &drm_fbdev_client_funcs); > > > + ret = drm_client_init(dev, &fb_helper->client, "fbdev", > > > &drm_fbdev_client_funcs); > > > if (ret) { > > > kfree(fb_helper); > > > DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", > > > ret); > > > return ret; > > > } > > > + drm_client_add(&fb_helper->client); > > > + > > > fb_helper->preferred_bpp = preferred_bpp; > > > ret = drm_fbdev_client_hotplug(&fb_helper->client); > > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > > > index 989f8e52864d..971bb7853776 100644 > > > --- a/include/drm/drm_client.h > > > +++ b/include/drm/drm_client.h > > > @@ -87,9 +87,10 @@ struct drm_client_dev { > > > struct drm_file *file; > > > }; > > > -int drm_client_new(struct drm_device *dev, struct drm_client_dev > > > *client, > > > - const char *name, const struct drm_client_funcs *funcs); > > > +int drm_client_init(struct drm_device *dev, struct drm_client_dev > > > *client, > > > + const char *name, const struct drm_client_funcs *funcs); > > > void drm_client_release(struct drm_client_dev *client); > > > +void drm_client_add(struct drm_client_dev *client); > > > void drm_client_dev_unregister(struct drm_device *dev); > > > void drm_client_dev_hotplug(struct drm_device *dev); > >
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 17d9a64e885e..fc03d26fcacc 100644 --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -63,20 +63,21 @@ static void drm_client_close(struct drm_client_dev *client) EXPORT_SYMBOL(drm_client_close); /** - * drm_client_new - Create a DRM client + * drm_client_init - Initialise a DRM client * @dev: DRM device * @client: DRM client * @name: Client name * @funcs: DRM client functions (optional) * + * This initialises the client and opens a &drm_file. Use drm_client_add() to complete the process. * The caller needs to hold a reference on @dev before calling this function. * The client is freed when the &drm_device is unregistered. See drm_client_release(). * * Returns: * Zero on success or negative error code on failure. */ -int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, - const char *name, const struct drm_client_funcs *funcs) +int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, + const char *name, const struct drm_client_funcs *funcs) { int ret; @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, if (ret) goto err_put_module; - mutex_lock(&dev->clientlist_mutex); - list_add(&client->list, &dev->clientlist); - mutex_unlock(&dev->clientlist_mutex); - drm_dev_get(dev); return 0; @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, return ret; } -EXPORT_SYMBOL(drm_client_new); +EXPORT_SYMBOL(drm_client_init); + +/** + * drm_client_add - Add client to the device list + * @client: DRM client + * + * Add the client to the &drm_device client list to activate its callbacks. + * @client must be initialized by a call to drm_client_init(). After + * drm_client_add() it is no longer permissible to call drm_client_release() + * directly (outside the unregister callback), instead cleanup will happen + * automatically on driver unload. + */ +void drm_client_add(struct drm_client_dev *client) +{ + struct drm_device *dev = client->dev; + + mutex_lock(&dev->clientlist_mutex); + list_add(&client->list, &dev->clientlist); + mutex_unlock(&dev->clientlist_mutex); +} +EXPORT_SYMBOL(drm_client_add); /** * drm_client_release - Release DRM client resources * @client: DRM client * - * Releases resources by closing the &drm_file that was opened by drm_client_new(). + * Releases resources by closing the &drm_file that was opened by drm_client_init(). * It is called automatically if the &drm_client_funcs.unregister callback is _not_ set. * * This function should only be called from the unregister callback. An exception diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..fb0dfc62b1b6 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -167,7 +167,7 @@ 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); + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL); if (ret) goto err_free; @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, if (ret) goto err_client_put; + drm_client_add(&fb_helper->client); + return fbdev_cma; err_client_put: diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index a504a5e05676..e9095537e0c0 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp) if (!fb_helper) return -ENOMEM; - ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs); + ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs); if (ret) { kfree(fb_helper); DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret); return ret; } + drm_client_add(&fb_helper->client); + fb_helper->preferred_bpp = preferred_bpp; ret = drm_fbdev_client_hotplug(&fb_helper->client); diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index 989f8e52864d..971bb7853776 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -87,9 +87,10 @@ struct drm_client_dev { struct drm_file *file; }; -int drm_client_new(struct drm_device *dev, struct drm_client_dev *client, - const char *name, const struct drm_client_funcs *funcs); +int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, + const char *name, const struct drm_client_funcs *funcs); void drm_client_release(struct drm_client_dev *client); +void drm_client_add(struct drm_client_dev *client); void drm_client_dev_unregister(struct drm_device *dev); void drm_client_dev_hotplug(struct drm_device *dev);
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. Split drm_client_new() into drm_client_init() and drm_client_add() to fix removal in the error path. 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> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- Changes since version 2: - Fix drm_client_new() ref in docs (Daniel) - Expand drm_client_add() docs (Daniel) Changes since version 1: - Split drm_client_new() instead of letting drm_client hang around (Daniel) drivers/gpu/drm/drm_client.c | 35 ++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++- drivers/gpu/drm/drm_fb_helper.c | 4 +++- include/drm/drm_client.h | 5 +++-- 4 files changed, 35 insertions(+), 13 deletions(-)