* Re: [LKP] [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup [not found] ` <CAHk-=whM09ZxdrrN-o3k1JOAn0keF5aXtPAcEYiukn7kW7yp2g@mail.gmail.com> @ 2018-12-23 0:43 ` Peter Wu 2018-12-23 0:55 ` [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Peter Wu 0 siblings, 1 reply; 8+ messages in thread From: Peter Wu @ 2018-12-23 0:43 UTC (permalink / raw) To: Linus Torvalds Cc: rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, dri-devel, kraxel, lkp On Fri, Dec 21, 2018 at 11:25:41AM -0800, Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 12:32 AM kernel test robot > <rong.a.chen@intel.com> wrote: > > > > FYI, we noticed commit df2052cc9221 ("bochs: convert to > > drm_fb_helper_fbdev_setup/teardown") caused > > > > [ 487.591733] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_mode_config.c:478 drm_mode_config_cleanup+0x270/0x290 > > Ok, this is apparently just a leak for what appears to be a not > particularly interesting error case, but the warning is new to 4.20 > (*) so it would be nice to have somebody look at it. > > That commit is supposed to fix a leak, but there's apparently > something still there. > (*) the *problem* is probably not new, it's just now exposed by the > switch to drm_mode_config_cleanup(). I concur, the issue was only revealed because a (not so interesting error path was triggered). Reproduced this on current master (v4.20-rc7-274-g23203e3f34c9), the trace leading up to the warning is the same: [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 ... [ 50.023155] Call Trace: [ 50.023155] ? bochs_kms_fini+0x1e/0x30 [ 50.023155] ? bochs_unload+0x18/0x40 [ 50.023155] ? bochs_pci_remove+0x18/0x30 [ 50.023155] ? pci_device_remove+0x1c/0x50 [ 50.031880] ? really_probe+0xf3/0x2d0 [ 50.031880] ? driver_probe_device+0x23/0xa0 The warning suggests that drm_framebuffer_init was called at some point without a matching call to drm_framebuffer_cleanup. Adding dump_stack() reveals: [ 97.673399] drm_framebuffer_init+0x17d/0x190 [ 97.674134] drm_gem_fb_alloc+0xbe/0x120 [ 97.674678] drm_gem_fbdev_fb_create+0x184/0x1c0 [ 97.675322] ? drm_gem_fb_simple_display_pipe_prepare_fb+0x20/0x20 [ 97.676771] ? drm_fb_helper_alloc_fbi+0xe1/0x120 [ 97.677408] bochsfb_create+0x245/0x5f0 [ 97.677935] ? bochsfb_mmap+0x60/0x60 [ 97.678421] __drm_fb_helper_initial_config_and_unlock+0x3d3/0x7b0 [ 97.678827] ? drm_setup_crtcs+0x1430/0x1430 [ 97.678827] drm_fb_helper_fbdev_setup+0x12b/0x230 [ 97.678827] bochs_fbdev_init+0x33/0x40 [ 97.678827] bochs_pci_probe+0x197/0x1a0 [ 97.678827] pci_device_probe+0xe9/0x180 [ 97.693848] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer [ 97.694880] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) More precisely, this is the call chain (obtained via GDB): drm_fb_helper_fbdev_setup -> drm_fb_helper_initial_config -> __drm_fb_helper_initial_config_and_unlock -> drm_fb_helper_single_fb_probe -> bochsfb_create -> drm_gem_fbdev_fb_create -> drm_gem_fb_alloc -> drm_framebuffer_init Let's have a look at the source of the error message, keep in mind that drm_fb_helper_fini is called on the error path: int drm_fb_helper_fbdev_setup(struct drm_device *dev, ...) { /* ... */ ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp); if (ret < 0) { DRM_DEV_ERROR(dev->dev, "fbdev: Failed to set configuration (ret=%d)\n", ret); goto err_drm_fb_helper_fini; } return 0; err_drm_fb_helper_fini: drm_fb_helper_fini(fb_helper); return ret; } The CONFIG_FB_LITTLE_ENDIAN error above suggests that this code path is reached *after* calling bochsfb_create: drm_fb_helper_fbdev_setup -> drm_fb_helper_initial_config -> __drm_fb_helper_initial_config_and_unlock -> register_framebuffer -> do_register_framebuffer -> fb_check_foreignness (prints error and propagates error code back to drm_fb_helper_fbdev_setup). What does "drm_fb_helper_fini" do? Among other things it basically kfree's memory associated with "fb_helper->fbdev" which was created using "drm_fb_helper_alloc_fbi" in the "fb_probe" callback. This is sufficient for "drm_fb_helper_generic_probe" (introduced by Noralf), but not for "bochsfb_create" which additionally calls "drm_gem_fbdev_fb_create": info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { DRM_ERROR("Failed to allocate fbi: %ld\n", PTR_ERR(info)); return PTR_ERR(info); } info->par = &bochs->fb.helper; fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL); if (IS_ERR(fb)) { DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb)); return PTR_ERR(fb); } /* setup helper */ bochs->fb.helper.fb = fb; Note that "fb" is unhandled by "drm_fb_helper_fini", so it leaks. What is the usual behavior? drm_fb_helper_fbdev_setup succeeds and on unload drm_fb_helper_fbdev_teardown is called which properly releases "fb": void drm_fb_helper_fbdev_teardown(struct drm_device *dev) { struct drm_fb_helper *fb_helper = dev->fb_helper; struct fb_ops *fbops = NULL; if (!fb_helper) return; /* Unregister if it hasn't been done already */ if (fb_helper->fbdev && fb_helper->fbdev->dev) drm_fb_helper_unregister_fbi(fb_helper); if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) { fb_deferred_io_cleanup(fb_helper->fbdev); kfree(fb_helper->fbdev->fbdefio); fbops = fb_helper->fbdev->fbops; } drm_fb_helper_fini(fb_helper); kfree(fbops); if (fb_helper->fb) drm_framebuffer_remove(fb_helper->fb); // yay! } Due to calling "drm_fb_helper_fini" however, "dev->fb_helper" will be NULL and thus this function does nothing on the error path. So in summary, "drm_fb_helper_fbdev_setup" calls the driver callback drm_fb_helper_funcs::fb_probe, detects an error but does not properly release all resources from the callback even after calling "drm_fb_helper_fini". On unload, "drm_fb_helper_fbdev_teardown" has no effect because the earlier call to "drm_fb_helper_fini" and skips the required "drm_framebuffer_remove" call. I'll send a proposed patch in a reply. -- Kind regards, Peter Wu https://lekensteyn.nl _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2018-12-23 0:43 ` [LKP] [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup Peter Wu @ 2018-12-23 0:55 ` Peter Wu 2018-12-23 13:55 ` Noralf Trønnes 0 siblings, 1 reply; 8+ messages in thread From: Peter Wu @ 2018-12-23 0:55 UTC (permalink / raw) To: dri-devel Cc: rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, Linus Torvalds, kraxel, lkp After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will have some effect). After that, drm_fb_helper_initial_config is called which may call the "fb_probe" driver callback. This driver callback may call drm_fb_helper_defio_init (as is done by drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs) as documented. These are normally cleaned up on exit by drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. If an error occurs after "fb_probe", but before setup is complete, then calling just drm_fb_helper_fini will leak resources. This was triggered by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"): [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 ... [ 50.023155] Call Trace: [ 50.023155] ? bochs_kms_fini+0x1e/0x30 [ 50.023155] ? bochs_unload+0x18/0x40 This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian Link: https://lkml.kernel.org/r/20181223004315.GA11455@al Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()") Reported-by: kernel test robot <rong.a.chen@intel.com> Cc: Noralf Trønnes <noralf@tronnes.org> Signed-off-by: Peter Wu <peter@lekensteyn.nl> --- drivers/gpu/drm/drm_fb_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 9d64f874f965..432e0f3b9267 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, return 0; err_drm_fb_helper_fini: - drm_fb_helper_fini(fb_helper); + drm_fb_helper_fbdev_teardown(dev); return ret; } -- 2.20.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2018-12-23 0:55 ` [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Peter Wu @ 2018-12-23 13:55 ` Noralf Trønnes 2018-12-23 23:10 ` Peter Wu 0 siblings, 1 reply; 8+ messages in thread From: Noralf Trønnes @ 2018-12-23 13:55 UTC (permalink / raw) To: Peter Wu, dri-devel Cc: rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, Linus Torvalds, kraxel, lkp Den 23.12.2018 01.55, skrev Peter Wu: > After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, > "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will > have some effect). After that, drm_fb_helper_initial_config is called > which may call the "fb_probe" driver callback. > > This driver callback may call drm_fb_helper_defio_init (as is done by > drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs) > as documented. These are normally cleaned up on exit by > drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. > > If an error occurs after "fb_probe", but before setup is complete, then > calling just drm_fb_helper_fini will leak resources. This was triggered > by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"): > > [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer > [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) > [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 > [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 > [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 > [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 > ... > [ 50.023155] Call Trace: > [ 50.023155] ? bochs_kms_fini+0x1e/0x30 > [ 50.023155] ? bochs_unload+0x18/0x40 > > This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. > > Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian > Link: https://lkml.kernel.org/r/20181223004315.GA11455@al > Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()") > Reported-by: kernel test robot <rong.a.chen@intel.com> > Cc: Noralf Trønnes <noralf@tronnes.org> > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > --- > drivers/gpu/drm/drm_fb_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 9d64f874f965..432e0f3b9267 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, > return 0; > > err_drm_fb_helper_fini: > - drm_fb_helper_fini(fb_helper); > + drm_fb_helper_fbdev_teardown(dev); This change will break the error path for drm_fbdev_generic_setup() because drm_fb_helper_generic_probe() cleans up on error but doesn't clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove(). My assumption has been that the drm_fb_helper_funcs->fb_probe callback cleans up its resources on error. Clearly this is not the case for bochs, so my take on this is that bochsfb_create() needs to clean up on error. Gerd has a patchset that switches bochs over to the generic fbdev emulation, but ofc that doesn't help with 4.20: https://patchwork.freedesktop.org/series/54269/ Noralf. > > return ret; > } > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2018-12-23 13:55 ` Noralf Trønnes @ 2018-12-23 23:10 ` Peter Wu 2018-12-24 14:52 ` Noralf Trønnes 0 siblings, 1 reply; 8+ messages in thread From: Peter Wu @ 2018-12-23 23:10 UTC (permalink / raw) To: Noralf Trønnes Cc: lkp, rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, dri-devel, kraxel, Linus Torvalds On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote: > > > Den 23.12.2018 01.55, skrev Peter Wu: > > After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, > > "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will > > have some effect). After that, drm_fb_helper_initial_config is called > > which may call the "fb_probe" driver callback. > > > > This driver callback may call drm_fb_helper_defio_init (as is done by > > drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs) > > as documented. These are normally cleaned up on exit by > > drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. > > > > If an error occurs after "fb_probe", but before setup is complete, then > > calling just drm_fb_helper_fini will leak resources. This was triggered > > by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"): > > > > [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer > > [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) > > [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 > > [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 > > [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 > > [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 > > ... > > [ 50.023155] Call Trace: > > [ 50.023155] ? bochs_kms_fini+0x1e/0x30 > > [ 50.023155] ? bochs_unload+0x18/0x40 > > > > This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. > > > > Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian > > Link: https://lkml.kernel.org/r/20181223004315.GA11455@al > > Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()") > > Reported-by: kernel test robot <rong.a.chen@intel.com> > > Cc: Noralf Trønnes <noralf@tronnes.org> > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > --- > > drivers/gpu/drm/drm_fb_helper.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 9d64f874f965..432e0f3b9267 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, > > return 0; > > err_drm_fb_helper_fini: > > - drm_fb_helper_fini(fb_helper); > > + drm_fb_helper_fbdev_teardown(dev); > > This change will break the error path for drm_fbdev_generic_setup() > because drm_fb_helper_generic_probe() cleans up on error but doesn't > clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove(). This should probably considered a bug of drm_fb_helper_generic_probe. Ownership of fb_helper should remain with the caller. The caller can detect an error and act accordingly. > My assumption has been that the drm_fb_helper_funcs->fb_probe callback > cleans up its resources on error. Clearly this is not the case for bochs, so > my take on this is that bochsfb_create() needs to clean up on error. That assumption still holds for bochs. The problem is this sequence: - drm_fb_helper_fbdev_setup is called. - fb_probe succeeds (this is crucial). - register_framebuffer fails. - error path of setup is triggered. As fb_helper is fully setup by drivers, the drm_fb_helper core should fully deallocate it again on the error path or else a leak occurs. > Gerd has a patchset that switches bochs over to the generic fbdev > emulation, but ofc that doesn't help with 4.20: > https://patchwork.freedesktop.org/series/54269/ And that does not help with other users of the drm_fb_helper who use functions like drm_fb_helper_defio_init. They will likely run in the same problem. I don't have a way to test tinydrm or other drivers, but if you force register_framebuffer to fail, you should be able to reproduce the problem with drm_fb_helper_generic_probe. -- Kind regards, Peter Wu https://lekensteyn.nl _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2018-12-23 23:10 ` Peter Wu @ 2018-12-24 14:52 ` Noralf Trønnes 2018-12-24 15:03 ` Peter Wu 0 siblings, 1 reply; 8+ messages in thread From: Noralf Trønnes @ 2018-12-24 14:52 UTC (permalink / raw) To: Peter Wu Cc: lkp, rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, dri-devel, kraxel, Linus Torvalds Den 24.12.2018 00.10, skrev Peter Wu: > On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote: >> >> >> Den 23.12.2018 01.55, skrev Peter Wu: >>> After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, >>> "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will >>> have some effect). After that, drm_fb_helper_initial_config is called >>> which may call the "fb_probe" driver callback. >>> >>> This driver callback may call drm_fb_helper_defio_init (as is done by >>> drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs) >>> as documented. These are normally cleaned up on exit by >>> drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. >>> >>> If an error occurs after "fb_probe", but before setup is complete, then >>> calling just drm_fb_helper_fini will leak resources. This was triggered >>> by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"): >>> >>> [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer >>> [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) >>> [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 >>> [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 >>> [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 >>> [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 >>> ... >>> [ 50.023155] Call Trace: >>> [ 50.023155] ? bochs_kms_fini+0x1e/0x30 >>> [ 50.023155] ? bochs_unload+0x18/0x40 >>> >>> This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. >>> >>> Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian >>> Link: https://lkml.kernel.org/r/20181223004315.GA11455@al >>> Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()") >>> Reported-by: kernel test robot <rong.a.chen@intel.com> >>> Cc: Noralf Trønnes <noralf@tronnes.org> >>> Signed-off-by: Peter Wu <peter@lekensteyn.nl> >>> --- >>> drivers/gpu/drm/drm_fb_helper.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>> index 9d64f874f965..432e0f3b9267 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, >>> return 0; >>> err_drm_fb_helper_fini: >>> - drm_fb_helper_fini(fb_helper); >>> + drm_fb_helper_fbdev_teardown(dev); >> >> This change will break the error path for drm_fbdev_generic_setup() >> because drm_fb_helper_generic_probe() cleans up on error but doesn't >> clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove(). > > This should probably considered a bug of drm_fb_helper_generic_probe. > Ownership of fb_helper should remain with the caller. The caller can > detect an error and act accordingly. > >> My assumption has been that the drm_fb_helper_funcs->fb_probe callback >> cleans up its resources on error. Clearly this is not the case for bochs, so >> my take on this is that bochsfb_create() needs to clean up on error. > > That assumption still holds for bochs. The problem is this sequence: > - drm_fb_helper_fbdev_setup is called. > - fb_probe succeeds (this is crucial). > - register_framebuffer fails. > - error path of setup is triggered. > > As fb_helper is fully setup by drivers, the drm_fb_helper core should > fully deallocate it again on the error path or else a leak occurs. > >> Gerd has a patchset that switches bochs over to the generic fbdev >> emulation, but ofc that doesn't help with 4.20: >> https://patchwork.freedesktop.org/series/54269/ > > And that does not help with other users of the drm_fb_helper who use > functions like drm_fb_helper_defio_init. They will likely run in the > same problem. > > I don't have a way to test tinydrm or other drivers, but if you force > register_framebuffer to fail, you should be able to reproduce the > problem with drm_fb_helper_generic_probe. > Now I understand. I have looked at the drivers that use drm_fb_helper and no one seem to handle the case where register_framebuffer() is failing. Here's what drivers do when drm_fb_helper_initial_config() fails: Doesn't check: amdgpu virtio Calls drm_fb_helper_fini(): armada ast exynos gma500 hisilicon mgag200 msm nouveau omap radeon rockchip tegra udl bochs - Uses drm_fb_helper_fbdev_setup() qxl - Uses drm_fb_helper_fbdev_setup() vboxvideo - Uses drm_fb_helper_fbdev_setup() Might clean up, not sure: cirrus Looks suspicious: i915 I looked at bochs before it switched to drm_fb_helper_fbdev_setup() and it also just called drm_fb_helper_fini(). It looks like you've uncovered something no one has though about (or not implemented at least). It's not just the framebuffer that's not destroyed, the buffer object is also leaked. drm_mode_config_cleanup() yells about the framebuffer (and frees it), but says nothing about the buffer object. It might be that it can't even be made to detect that since some drivers do special stuff for the fbdev buffer. I'll pick up on this and do some testing after the Christmas holidays. Noralf. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2018-12-24 14:52 ` Noralf Trønnes @ 2018-12-24 15:03 ` Peter Wu 2019-01-05 18:25 ` Noralf Trønnes 0 siblings, 1 reply; 8+ messages in thread From: Peter Wu @ 2018-12-24 15:03 UTC (permalink / raw) To: Noralf Trønnes Cc: lkp, rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, dri-devel, kraxel, Linus Torvalds On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Trønnes wrote: > > > Den 24.12.2018 00.10, skrev Peter Wu: > > On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote: > > > > > > > > > Den 23.12.2018 01.55, skrev Peter Wu: > > > > After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, > > > > "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will > > > > have some effect). After that, drm_fb_helper_initial_config is called > > > > which may call the "fb_probe" driver callback. > > > > > > > > This driver callback may call drm_fb_helper_defio_init (as is done by > > > > drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs) > > > > as documented. These are normally cleaned up on exit by > > > > drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. > > > > > > > > If an error occurs after "fb_probe", but before setup is complete, then > > > > calling just drm_fb_helper_fini will leak resources. This was triggered > > > > by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"): > > > > > > > > [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer > > > > [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) > > > > [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 > > > > [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 > > > > [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 > > > > [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 > > > > ... > > > > [ 50.023155] Call Trace: > > > > [ 50.023155] ? bochs_kms_fini+0x1e/0x30 > > > > [ 50.023155] ? bochs_unload+0x18/0x40 > > > > > > > > This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. > > > > > > > > Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian > > > > Link: https://lkml.kernel.org/r/20181223004315.GA11455@al > > > > Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()") > > > > Reported-by: kernel test robot <rong.a.chen@intel.com> > > > > Cc: Noralf Trønnes <noralf@tronnes.org> > > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > > > --- > > > > drivers/gpu/drm/drm_fb_helper.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > > index 9d64f874f965..432e0f3b9267 100644 > > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > > @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, > > > > return 0; > > > > err_drm_fb_helper_fini: > > > > - drm_fb_helper_fini(fb_helper); > > > > + drm_fb_helper_fbdev_teardown(dev); > > > > > > This change will break the error path for drm_fbdev_generic_setup() > > > because drm_fb_helper_generic_probe() cleans up on error but doesn't > > > clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove(). > > > > This should probably considered a bug of drm_fb_helper_generic_probe. > > Ownership of fb_helper should remain with the caller. The caller can > > detect an error and act accordingly. > > > > > My assumption has been that the drm_fb_helper_funcs->fb_probe callback > > > cleans up its resources on error. Clearly this is not the case for bochs, so > > > my take on this is that bochsfb_create() needs to clean up on error. > > > > That assumption still holds for bochs. The problem is this sequence: > > - drm_fb_helper_fbdev_setup is called. > > - fb_probe succeeds (this is crucial). > > - register_framebuffer fails. > > - error path of setup is triggered. > > > > As fb_helper is fully setup by drivers, the drm_fb_helper core should > > fully deallocate it again on the error path or else a leak occurs. > > > > > Gerd has a patchset that switches bochs over to the generic fbdev > > > emulation, but ofc that doesn't help with 4.20: > > > https://patchwork.freedesktop.org/series/54269/ > > > > And that does not help with other users of the drm_fb_helper who use > > functions like drm_fb_helper_defio_init. They will likely run in the > > same problem. > > > > I don't have a way to test tinydrm or other drivers, but if you force > > register_framebuffer to fail, you should be able to reproduce the > > problem with drm_fb_helper_generic_probe. > > > > Now I understand. I have looked at the drivers that use drm_fb_helper > and no one seem to handle the case where register_framebuffer() is > failing. > > Here's what drivers do when drm_fb_helper_initial_config() fails: > > Doesn't check: > amdgpu > virtio > > Calls drm_fb_helper_fini(): > armada > ast > exynos > gma500 > hisilicon > mgag200 > msm > nouveau > omap > radeon > rockchip > tegra > udl > bochs - Uses drm_fb_helper_fbdev_setup() > qxl - Uses drm_fb_helper_fbdev_setup() > vboxvideo - Uses drm_fb_helper_fbdev_setup() > > Might clean up, not sure: > cirrus > > Looks suspicious: > i915 > > I looked at bochs before it switched to drm_fb_helper_fbdev_setup() and > it also just called drm_fb_helper_fini(). > > It looks like you've uncovered something no one has though about (or > not implemented at least). > > It's not just the framebuffer that's not destroyed, the buffer object > is also leaked. drm_mode_config_cleanup() yells about the framebuffer > (and frees it), but says nothing about the buffer object. It might be > that it can't even be made to detect that since some drivers do special > stuff for the fbdev buffer. > > I'll pick up on this and do some testing after the Christmas holidays. Thanks, the warning is bad for CI (which uses QEMU), but otherwise it should not have any effect on regular users so it can wait. -- Kind regards, Peter Wu https://lekensteyn.nl _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2018-12-24 15:03 ` Peter Wu @ 2019-01-05 18:25 ` Noralf Trønnes 2019-01-08 17:55 ` Noralf Trønnes 0 siblings, 1 reply; 8+ messages in thread From: Noralf Trønnes @ 2019-01-05 18:25 UTC (permalink / raw) To: Peter Wu Cc: lkp, rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, dri-devel, kraxel, Linus Torvalds Den 24.12.2018 16.03, skrev Peter Wu: > On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Trønnes wrote: >> >> >> Den 24.12.2018 00.10, skrev Peter Wu: >>> On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote: >>>> >>>> >>>> Den 23.12.2018 01.55, skrev Peter Wu: >>>>> After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, >>>>> "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will >>>>> have some effect). After that, drm_fb_helper_initial_config is called >>>>> which may call the "fb_probe" driver callback. >>>>> >>>>> This driver callback may call drm_fb_helper_defio_init (as is done by >>>>> drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs) >>>>> as documented. These are normally cleaned up on exit by >>>>> drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. >>>>> >>>>> If an error occurs after "fb_probe", but before setup is complete, then >>>>> calling just drm_fb_helper_fini will leak resources. This was triggered >>>>> by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"): >>>>> >>>>> [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer >>>>> [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) >>>>> [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 >>>>> [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 >>>>> [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 >>>>> [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 >>>>> ... >>>>> [ 50.023155] Call Trace: >>>>> [ 50.023155] ? bochs_kms_fini+0x1e/0x30 >>>>> [ 50.023155] ? bochs_unload+0x18/0x40 >>>>> >>>>> This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. >>>>> >>>>> Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian >>>>> Link: https://lkml.kernel.org/r/20181223004315.GA11455@al >>>>> Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()") >>>>> Reported-by: kernel test robot <rong.a.chen@intel.com> >>>>> Cc: Noralf Trønnes <noralf@tronnes.org> >>>>> Signed-off-by: Peter Wu <peter@lekensteyn.nl> >>>>> --- >>>>> drivers/gpu/drm/drm_fb_helper.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>>>> index 9d64f874f965..432e0f3b9267 100644 >>>>> --- a/drivers/gpu/drm/drm_fb_helper.c >>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>>>> @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, >>>>> return 0; >>>>> err_drm_fb_helper_fini: >>>>> - drm_fb_helper_fini(fb_helper); >>>>> + drm_fb_helper_fbdev_teardown(dev); >>>> >>>> This change will break the error path for drm_fbdev_generic_setup() >>>> because drm_fb_helper_generic_probe() cleans up on error but doesn't >>>> clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove(). >>> >>> This should probably considered a bug of drm_fb_helper_generic_probe. >>> Ownership of fb_helper should remain with the caller. The caller can >>> detect an error and act accordingly. >>> >>>> My assumption has been that the drm_fb_helper_funcs->fb_probe callback >>>> cleans up its resources on error. Clearly this is not the case for bochs, so >>>> my take on this is that bochsfb_create() needs to clean up on error. >>> >>> That assumption still holds for bochs. The problem is this sequence: >>> - drm_fb_helper_fbdev_setup is called. >>> - fb_probe succeeds (this is crucial). >>> - register_framebuffer fails. >>> - error path of setup is triggered. >>> >>> As fb_helper is fully setup by drivers, the drm_fb_helper core should >>> fully deallocate it again on the error path or else a leak occurs. >>> >>>> Gerd has a patchset that switches bochs over to the generic fbdev >>>> emulation, but ofc that doesn't help with 4.20: >>>> https://patchwork.freedesktop.org/series/54269/ >>> >>> And that does not help with other users of the drm_fb_helper who use >>> functions like drm_fb_helper_defio_init. They will likely run in the >>> same problem. >>> >>> I don't have a way to test tinydrm or other drivers, but if you force >>> register_framebuffer to fail, you should be able to reproduce the >>> problem with drm_fb_helper_generic_probe. >>> >> >> Now I understand. I have looked at the drivers that use drm_fb_helper >> and no one seem to handle the case where register_framebuffer() is >> failing. >> >> Here's what drivers do when drm_fb_helper_initial_config() fails: >> >> Doesn't check: >> amdgpu >> virtio >> >> Calls drm_fb_helper_fini(): >> armada >> ast >> exynos >> gma500 >> hisilicon >> mgag200 >> msm >> nouveau >> omap >> radeon >> rockchip >> tegra >> udl >> bochs - Uses drm_fb_helper_fbdev_setup() >> qxl - Uses drm_fb_helper_fbdev_setup() >> vboxvideo - Uses drm_fb_helper_fbdev_setup() >> >> Might clean up, not sure: >> cirrus >> >> Looks suspicious: >> i915 >> >> I looked at bochs before it switched to drm_fb_helper_fbdev_setup() and >> it also just called drm_fb_helper_fini(). >> >> It looks like you've uncovered something no one has though about (or >> not implemented at least). >> >> It's not just the framebuffer that's not destroyed, the buffer object >> is also leaked. drm_mode_config_cleanup() yells about the framebuffer >> (and frees it), but says nothing about the buffer object. It might be >> that it can't even be made to detect that since some drivers do special >> stuff for the fbdev buffer. >> >> I'll pick up on this and do some testing after the Christmas holidays. > > Thanks, the warning is bad for CI (which uses QEMU), but otherwise it > should not have any effect on regular users so it can wait. > This patch is good as long as it's applied along side the fix[1] to the generic emulation: Reviewed-by: Noralf Trønnes <noralf@tronnes.org> I can apply them both when I get an ack/rb on the other patch. Thanks for fixing this. Noralf. [1] https://patchwork.freedesktop.org/patch/275002/ _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup 2019-01-05 18:25 ` Noralf Trønnes @ 2019-01-08 17:55 ` Noralf Trønnes 0 siblings, 0 replies; 8+ messages in thread From: Noralf Trønnes @ 2019-01-08 17:55 UTC (permalink / raw) To: Peter Wu Cc: rong.a.chen, Daniel Vetter, Linux List Kernel Mailing, dri-devel, Linus Torvalds, kraxel, lkp Den 05.01.2019 19.25, skrev Noralf Trønnes: > > > Den 24.12.2018 16.03, skrev Peter Wu: >> On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Trønnes wrote: >>> >>> >>> Den 24.12.2018 00.10, skrev Peter Wu: >>>> On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote: >>>>> >>>>> >>>>> Den 23.12.2018 01.55, skrev Peter Wu: >>>>>> After drm_fb_helper_fbdev_setup calls drm_fb_helper_init, >>>>>> "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini >>>>>> will >>>>>> have some effect). After that, drm_fb_helper_initial_config is called >>>>>> which may call the "fb_probe" driver callback. >>>>>> >>>>>> This driver callback may call drm_fb_helper_defio_init (as is done by >>>>>> drm_fb_helper_generic_probe) or set a framebuffer (as is done by >>>>>> bochs) >>>>>> as documented. These are normally cleaned up on exit by >>>>>> drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini. >>>>>> >>>>>> If an error occurs after "fb_probe", but before setup is complete, >>>>>> then >>>>>> calling just drm_fb_helper_fini will leak resources. This was >>>>>> triggered >>>>>> by df2052cc922 ("bochs: convert to >>>>>> drm_fb_helper_fbdev_setup/teardown"): >>>>>> >>>>>> [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN >>>>>> to support this framebuffer >>>>>> [ 50.009436] bochs-drm 0000:00:02.0: >>>>>> [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set >>>>>> configuration (ret=-38) >>>>>> [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 >>>>>> for 0000:00:02.0 on minor 2 >>>>>> [ 50.013604] WARNING: CPU: 1 PID: 1 at >>>>>> drivers/gpu/drm/drm_mode_config.c:477 >>>>>> drm_mode_config_cleanup+0x280/0x2a0 >>>>>> [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: >>>>>> G T 4.20.0-rc7 #1 >>>>>> [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 >>>>>> ... >>>>>> [ 50.023155] Call Trace: >>>>>> [ 50.023155] ? bochs_kms_fini+0x1e/0x30 >>>>>> [ 50.023155] ? bochs_unload+0x18/0x40 >>>>>> >>>>>> This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n. >>>>>> >>>>>> Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian >>>>>> Link: https://lkml.kernel.org/r/20181223004315.GA11455@al >>>>>> Fixes: 8741216396b2 ("drm/fb-helper: Add >>>>>> drm_fb_helper_fbdev_setup/teardown()") >>>>>> Reported-by: kernel test robot <rong.a.chen@intel.com> >>>>>> Cc: Noralf Trønnes <noralf@tronnes.org> >>>>>> Signed-off-by: Peter Wu <peter@lekensteyn.nl> >>>>>> --- >>>>>> drivers/gpu/drm/drm_fb_helper.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c >>>>>> b/drivers/gpu/drm/drm_fb_helper.c >>>>>> index 9d64f874f965..432e0f3b9267 100644 >>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>>>>> @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct >>>>>> drm_device *dev, >>>>>> return 0; >>>>>> err_drm_fb_helper_fini: >>>>>> - drm_fb_helper_fini(fb_helper); >>>>>> + drm_fb_helper_fbdev_teardown(dev); >>>>> >>>>> This change will break the error path for drm_fbdev_generic_setup() >>>>> because drm_fb_helper_generic_probe() cleans up on error but doesn't >>>>> clear drm_fb_helper->fb resulting in a double >>>>> drm_framebuffer_remove(). >>>> >>>> This should probably considered a bug of drm_fb_helper_generic_probe. >>>> Ownership of fb_helper should remain with the caller. The caller can >>>> detect an error and act accordingly. >>>> >>>>> My assumption has been that the drm_fb_helper_funcs->fb_probe callback >>>>> cleans up its resources on error. Clearly this is not the case for >>>>> bochs, so >>>>> my take on this is that bochsfb_create() needs to clean up on error. >>>> >>>> That assumption still holds for bochs. The problem is this sequence: >>>> - drm_fb_helper_fbdev_setup is called. >>>> - fb_probe succeeds (this is crucial). >>>> - register_framebuffer fails. >>>> - error path of setup is triggered. >>>> >>>> As fb_helper is fully setup by drivers, the drm_fb_helper core should >>>> fully deallocate it again on the error path or else a leak occurs. >>>> >>>>> Gerd has a patchset that switches bochs over to the generic fbdev >>>>> emulation, but ofc that doesn't help with 4.20: >>>>> https://patchwork.freedesktop.org/series/54269/ >>>> >>>> And that does not help with other users of the drm_fb_helper who use >>>> functions like drm_fb_helper_defio_init. They will likely run in the >>>> same problem. >>>> >>>> I don't have a way to test tinydrm or other drivers, but if you force >>>> register_framebuffer to fail, you should be able to reproduce the >>>> problem with drm_fb_helper_generic_probe. >>>> >>> >>> Now I understand. I have looked at the drivers that use drm_fb_helper >>> and no one seem to handle the case where register_framebuffer() is >>> failing. >>> >>> Here's what drivers do when drm_fb_helper_initial_config() fails: >>> >>> Doesn't check: >>> amdgpu >>> virtio >>> >>> Calls drm_fb_helper_fini(): >>> armada >>> ast >>> exynos >>> gma500 >>> hisilicon >>> mgag200 >>> msm >>> nouveau >>> omap >>> radeon >>> rockchip >>> tegra >>> udl >>> bochs - Uses drm_fb_helper_fbdev_setup() >>> qxl - Uses drm_fb_helper_fbdev_setup() >>> vboxvideo - Uses drm_fb_helper_fbdev_setup() >>> >>> Might clean up, not sure: >>> cirrus >>> >>> Looks suspicious: >>> i915 >>> >>> I looked at bochs before it switched to drm_fb_helper_fbdev_setup() and >>> it also just called drm_fb_helper_fini(). >>> >>> It looks like you've uncovered something no one has though about (or >>> not implemented at least). >>> >>> It's not just the framebuffer that's not destroyed, the buffer object >>> is also leaked. drm_mode_config_cleanup() yells about the framebuffer >>> (and frees it), but says nothing about the buffer object. It might be >>> that it can't even be made to detect that since some drivers do special >>> stuff for the fbdev buffer. >>> >>> I'll pick up on this and do some testing after the Christmas holidays. >> >> Thanks, the warning is bad for CI (which uses QEMU), but otherwise it >> should not have any effect on regular users so it can wait. >> > > This patch is good as long as it's applied along side the fix[1] to the > generic emulation: > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > > I can apply them both when I get an ack/rb on the other patch. > Applied to drm-misc-next. Noralf. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-08 17:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20181221083226.GI23332@shao2-debian> [not found] ` <CAHk-=whM09ZxdrrN-o3k1JOAn0keF5aXtPAcEYiukn7kW7yp2g@mail.gmail.com> 2018-12-23 0:43 ` [LKP] [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup Peter Wu 2018-12-23 0:55 ` [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Peter Wu 2018-12-23 13:55 ` Noralf Trønnes 2018-12-23 23:10 ` Peter Wu 2018-12-24 14:52 ` Noralf Trønnes 2018-12-24 15:03 ` Peter Wu 2019-01-05 18:25 ` Noralf Trønnes 2019-01-08 17:55 ` Noralf Trønnes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).