| Message ID | 20190105181846.26495-1-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fb-helper: generic: Fix setup error path | expand |
Hi, > If register_framebuffer() fails during fbdev setup we will leak the > framebuffer, the GEM buffer and the shadow buffer for defio. This is > because drm_fb_helper_fbdev_setup() just calls drm_fb_helper_fini() on > error not taking into account that register_framebuffer() can fail. > > Since the generic emulation uses DRM client for its framebuffer and > backing buffer in addition to a shadow buffer, it's necessary to open code > drm_fb_helper_fbdev_setup() to properly handle the error path. > > Error cleanup is removed from .fb_probe and is handled by one function for > all paths. > > Fixes: 9060d7f49376 ("drm/fb-helper: Finish the generic fbdev emulation") > Reported-by: Peter Wu <peter@lekensteyn.nl> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Looks sane to me. Acked-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 98 +++++++++++++++++++-------------- > 1 file changed, 58 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 5e9ca6f96379..80136a7a5af1 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -2961,18 +2961,16 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user) > return 0; > } > > -/* > - * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of > - * unregister_framebuffer() or fb_release(). > - */ > -static void drm_fbdev_fb_destroy(struct fb_info *info) > +static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper) > { > - struct drm_fb_helper *fb_helper = info->par; > struct fb_info *fbi = fb_helper->fbdev; > struct fb_ops *fbops = NULL; > void *shadow = NULL; > > - if (fbi->fbdefio) { > + if (!fb_helper->dev) > + return; > + > + if (fbi && fbi->fbdefio) { > fb_deferred_io_cleanup(fbi); > shadow = fbi->screen_buffer; > fbops = fbi->fbops; > @@ -2986,6 +2984,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info) > } > > drm_client_framebuffer_delete(fb_helper->buffer); > +} > + > +static void drm_fbdev_release(struct drm_fb_helper *fb_helper) > +{ > + drm_fbdev_cleanup(fb_helper); > + > /* > * FIXME: > * Remove conditional when all CMA drivers have been moved over to using > @@ -2997,6 +3001,15 @@ static void drm_fbdev_fb_destroy(struct fb_info *info) > } > } > > +/* > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of > + * unregister_framebuffer() or fb_release(). > + */ > +static void drm_fbdev_fb_destroy(struct fb_info *info) > +{ > + drm_fbdev_release(info->par); > +} > + > static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > { > struct drm_fb_helper *fb_helper = info->par; > @@ -3047,7 +3060,6 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > struct drm_framebuffer *fb; > struct fb_info *fbi; > u32 format; > - int ret; > > DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > sizes->surface_width, sizes->surface_height, > @@ -3064,10 +3076,8 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > fb = buffer->fb; > > fbi = drm_fb_helper_alloc_fbi(fb_helper); > - if (IS_ERR(fbi)) { > - ret = PTR_ERR(fbi); > - goto err_free_buffer; > - } > + if (IS_ERR(fbi)) > + return PTR_ERR(fbi); > > fbi->par = fb_helper; > fbi->fbops = &drm_fbdev_fb_ops; > @@ -3098,8 +3108,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > if (!fbops || !shadow) { > kfree(fbops); > vfree(shadow); > - ret = -ENOMEM; > - goto err_fb_info_destroy; > + return -ENOMEM; > } > > *fbops = *fbi->fbops; > @@ -3111,13 +3120,6 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > } > > return 0; > - > -err_fb_info_destroy: > - drm_fb_helper_fini(fb_helper); > -err_free_buffer: > - drm_client_framebuffer_delete(buffer); > - > - return ret; > } > EXPORT_SYMBOL(drm_fb_helper_generic_probe); > > @@ -3129,18 +3131,11 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) > { > struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); > > - if (fb_helper->fbdev) { > - drm_fb_helper_unregister_fbi(fb_helper); > + if (fb_helper->fbdev) > /* drm_fbdev_fb_destroy() takes care of cleanup */ > - return; > - } > - > - /* Did drm_fb_helper_fbdev_setup() run? */ > - if (fb_helper->dev) > - drm_fb_helper_fini(fb_helper); > - > - drm_client_release(client); > - kfree(fb_helper); > + drm_fb_helper_unregister_fbi(fb_helper); > + else > + drm_fbdev_release(fb_helper); > } > > static int drm_fbdev_client_restore(struct drm_client_dev *client) > @@ -3158,7 +3153,7 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) > struct drm_device *dev = client->dev; > int ret; > > - /* If drm_fb_helper_fbdev_setup() failed, we only try once */ > + /* Setup is not retried if it has failed */ > if (!fb_helper->dev && fb_helper->funcs) > return 0; > > @@ -3170,15 +3165,34 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) > return 0; > } > > - ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs, > - fb_helper->preferred_bpp, 0); > - if (ret) { > - fb_helper->dev = NULL; > - fb_helper->fbdev = NULL; > - return ret; > - } > + drm_fb_helper_prepare(dev, fb_helper, &drm_fb_helper_generic_funcs); > + > + ret = drm_fb_helper_init(dev, fb_helper, dev->mode_config.num_connector); > + if (ret) > + goto err; > + > + ret = drm_fb_helper_single_add_all_connectors(fb_helper); > + if (ret) > + goto err_cleanup; > + > + if (!drm_drv_uses_atomic_modeset(dev)) > + drm_helper_disable_unused_functions(dev); > + > + ret = drm_fb_helper_initial_config(fb_helper, fb_helper->preferred_bpp); > + if (ret) > + goto err_cleanup; > > return 0; > + > +err_cleanup: > + drm_fbdev_cleanup(fb_helper); > +err: > + fb_helper->dev = NULL; > + fb_helper->fbdev = NULL; > + > + DRM_DEV_ERROR(dev->dev, "fbdev: Failed to setup generic emulation (ret=%d)\n", ret); > + > + return ret; > } > > static const struct drm_client_funcs drm_fbdev_client_funcs = { > @@ -3237,6 +3251,10 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp) > > drm_client_add(&fb_helper->client); > > + if (!preferred_bpp) > + preferred_bpp = dev->mode_config.preferred_depth; > + if (!preferred_bpp) > + preferred_bpp = 32; > fb_helper->preferred_bpp = preferred_bpp; > > ret = drm_fbdev_client_hotplug(&fb_helper->client); > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 07.01.2019 13.39, skrev Gerd Hoffmann: > Hi, > >> If register_framebuffer() fails during fbdev setup we will leak the >> framebuffer, the GEM buffer and the shadow buffer for defio. This is >> because drm_fb_helper_fbdev_setup() just calls drm_fb_helper_fini() on >> error not taking into account that register_framebuffer() can fail. >> >> Since the generic emulation uses DRM client for its framebuffer and >> backing buffer in addition to a shadow buffer, it's necessary to open code >> drm_fb_helper_fbdev_setup() to properly handle the error path. >> >> Error cleanup is removed from .fb_probe and is handled by one function for >> all paths. >> >> Fixes: 9060d7f49376 ("drm/fb-helper: Finish the generic fbdev emulation") >> Reported-by: Peter Wu <peter@lekensteyn.nl> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > Looks sane to me. > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> Thanks, applied to drm-misc-next. Noralf.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5e9ca6f96379..80136a7a5af1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2961,18 +2961,16 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user) return 0; } -/* - * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of - * unregister_framebuffer() or fb_release(). - */ -static void drm_fbdev_fb_destroy(struct fb_info *info) +static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper) { - struct drm_fb_helper *fb_helper = info->par; struct fb_info *fbi = fb_helper->fbdev; struct fb_ops *fbops = NULL; void *shadow = NULL; - if (fbi->fbdefio) { + if (!fb_helper->dev) + return; + + if (fbi && fbi->fbdefio) { fb_deferred_io_cleanup(fbi); shadow = fbi->screen_buffer; fbops = fbi->fbops; @@ -2986,6 +2984,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info) } drm_client_framebuffer_delete(fb_helper->buffer); +} + +static void drm_fbdev_release(struct drm_fb_helper *fb_helper) +{ + drm_fbdev_cleanup(fb_helper); + /* * FIXME: * Remove conditional when all CMA drivers have been moved over to using @@ -2997,6 +3001,15 @@ static void drm_fbdev_fb_destroy(struct fb_info *info) } } +/* + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of + * unregister_framebuffer() or fb_release(). + */ +static void drm_fbdev_fb_destroy(struct fb_info *info) +{ + drm_fbdev_release(info->par); +} + static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) { struct drm_fb_helper *fb_helper = info->par; @@ -3047,7 +3060,6 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, struct drm_framebuffer *fb; struct fb_info *fbi; u32 format; - int ret; DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", sizes->surface_width, sizes->surface_height, @@ -3064,10 +3076,8 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, fb = buffer->fb; fbi = drm_fb_helper_alloc_fbi(fb_helper); - if (IS_ERR(fbi)) { - ret = PTR_ERR(fbi); - goto err_free_buffer; - } + if (IS_ERR(fbi)) + return PTR_ERR(fbi); fbi->par = fb_helper; fbi->fbops = &drm_fbdev_fb_ops; @@ -3098,8 +3108,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, if (!fbops || !shadow) { kfree(fbops); vfree(shadow); - ret = -ENOMEM; - goto err_fb_info_destroy; + return -ENOMEM; } *fbops = *fbi->fbops; @@ -3111,13 +3120,6 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, } return 0; - -err_fb_info_destroy: - drm_fb_helper_fini(fb_helper); -err_free_buffer: - drm_client_framebuffer_delete(buffer); - - return ret; } EXPORT_SYMBOL(drm_fb_helper_generic_probe); @@ -3129,18 +3131,11 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) { struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); - if (fb_helper->fbdev) { - drm_fb_helper_unregister_fbi(fb_helper); + if (fb_helper->fbdev) /* drm_fbdev_fb_destroy() takes care of cleanup */ - return; - } - - /* Did drm_fb_helper_fbdev_setup() run? */ - if (fb_helper->dev) - drm_fb_helper_fini(fb_helper); - - drm_client_release(client); - kfree(fb_helper); + drm_fb_helper_unregister_fbi(fb_helper); + else + drm_fbdev_release(fb_helper); } static int drm_fbdev_client_restore(struct drm_client_dev *client) @@ -3158,7 +3153,7 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) struct drm_device *dev = client->dev; int ret; - /* If drm_fb_helper_fbdev_setup() failed, we only try once */ + /* Setup is not retried if it has failed */ if (!fb_helper->dev && fb_helper->funcs) return 0; @@ -3170,15 +3165,34 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) return 0; } - ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs, - fb_helper->preferred_bpp, 0); - if (ret) { - fb_helper->dev = NULL; - fb_helper->fbdev = NULL; - return ret; - } + drm_fb_helper_prepare(dev, fb_helper, &drm_fb_helper_generic_funcs); + + ret = drm_fb_helper_init(dev, fb_helper, dev->mode_config.num_connector); + if (ret) + goto err; + + ret = drm_fb_helper_single_add_all_connectors(fb_helper); + if (ret) + goto err_cleanup; + + if (!drm_drv_uses_atomic_modeset(dev)) + drm_helper_disable_unused_functions(dev); + + ret = drm_fb_helper_initial_config(fb_helper, fb_helper->preferred_bpp); + if (ret) + goto err_cleanup; return 0; + +err_cleanup: + drm_fbdev_cleanup(fb_helper); +err: + fb_helper->dev = NULL; + fb_helper->fbdev = NULL; + + DRM_DEV_ERROR(dev->dev, "fbdev: Failed to setup generic emulation (ret=%d)\n", ret); + + return ret; } static const struct drm_client_funcs drm_fbdev_client_funcs = { @@ -3237,6 +3251,10 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp) drm_client_add(&fb_helper->client); + if (!preferred_bpp) + preferred_bpp = dev->mode_config.preferred_depth; + if (!preferred_bpp) + preferred_bpp = 32; fb_helper->preferred_bpp = preferred_bpp; ret = drm_fbdev_client_hotplug(&fb_helper->client);
If register_framebuffer() fails during fbdev setup we will leak the framebuffer, the GEM buffer and the shadow buffer for defio. This is because drm_fb_helper_fbdev_setup() just calls drm_fb_helper_fini() on error not taking into account that register_framebuffer() can fail. Since the generic emulation uses DRM client for its framebuffer and backing buffer in addition to a shadow buffer, it's necessary to open code drm_fb_helper_fbdev_setup() to properly handle the error path. Error cleanup is removed from .fb_probe and is handled by one function for all paths. Fixes: 9060d7f49376 ("drm/fb-helper: Finish the generic fbdev emulation") Reported-by: Peter Wu <peter@lekensteyn.nl> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_fb_helper.c | 98 +++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 40 deletions(-)