| Message ID | 20181001145732.53712-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/cma-helper: Fix crash in fbdev error path | expand |
On Mon, Oct 01, 2018 at 04:57:32PM +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. > > 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> > --- > > Changes since version 1: > - Split drm_client_new() instead of letting drm_client hang around (Daniel) > > > Just to make sure I don't break anything, would this procedure do the right thing: > > dim update-branches > dim checkout drm-misc-fixes I recommend a dim create-worktree drm-misc-fixes here so that you have a separate checkout. Avoids accidentally thrashin stuff when switching between branches. > curl -sS https://patchwork.freedesktop.org/patch/$1/mbox/ > /tmp/patch > cat /tmp/patch | dim apply-branch drm-misc-fixes > > <build test> > > dim push-branch drm-misc-fixes Yup. On the patch, with the nits below addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > drivers/gpu/drm/drm_client.c | 29 +++++++++++++++++++++-------- > 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, 30 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > index 17d9a64e885e..b466e08931fa 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,7 +106,23 @@ 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. I think this needs a few more details clarified in the doc: "@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, 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 Grep says you missed one occurrence of drm_client_new() later on in one of the kerneldoc comments. > 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); Shouldn't we move this to later on, with an explicit drm_client_release() cleanup path, just for safety? Otoh, this is the minimal change, so maybe in a follow-up patch? Cheers, Daniel > + > 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); > -- > 2.15.1 >
Den 01.10.2018 18.44, skrev Daniel Vetter: > On Mon, Oct 01, 2018 at 04:57:32PM +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. >> >> 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> >> --- >> >> Changes since version 1: >> - Split drm_client_new() instead of letting drm_client hang around (Daniel) >> >> >> Just to make sure I don't break anything, would this procedure do the right thing: >> >> dim update-branches >> dim checkout drm-misc-fixes > I recommend a > > dim create-worktree drm-misc-fixes > > here so that you have a separate checkout. Avoids accidentally thrashin > stuff when switching between branches. Nice, I didn't know about git worktree. >> curl -sS https://patchwork.freedesktop.org/patch/$1/mbox/ > /tmp/patch >> cat /tmp/patch | dim apply-branch drm-misc-fixes >> >> <build test> >> >> dim push-branch drm-misc-fixes > Yup. On the patch, with the nits below addressed: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> >> drivers/gpu/drm/drm_client.c | 29 +++++++++++++++++++++-------- >> 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, 30 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 17d9a64e885e..b466e08931fa 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,7 +106,23 @@ 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. > I think this needs a few more details clarified in the doc: > > "@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, 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 > Grep says you missed one occurrence of drm_client_new() later on in one of > the kerneldoc comments. > >> 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); > Shouldn't we move this to later on, with an explicit drm_client_release() > cleanup path, just for safety? Otoh, this is the minimal change, so maybe > in a follow-up patch? Yep, I'll do that later. Thanks, Noralf. > Cheers, Daniel > >> + >> 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); >> -- >> 2.15.1 >>
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 17d9a64e885e..b466e08931fa 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,7 +106,23 @@ 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. + */ +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 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> --- Changes since version 1: - Split drm_client_new() instead of letting drm_client hang around (Daniel) Just to make sure I don't break anything, would this procedure do the right thing: dim update-branches dim checkout drm-misc-fixes curl -sS https://patchwork.freedesktop.org/patch/$1/mbox/ > /tmp/patch cat /tmp/patch | dim apply-branch drm-misc-fixes <build test> dim push-branch drm-misc-fixes drivers/gpu/drm/drm_client.c | 29 +++++++++++++++++++++-------- 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, 30 insertions(+), 12 deletions(-)