From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============3210116387802913281==" MIME-Version: 1.0 From: Peter Wu To: lkp@lists.01.org Subject: Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Date: Mon, 24 Dec 2018 16:03:02 +0100 Message-ID: <20181224150302.GA2259@al> In-Reply-To: <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> List-Id: --===============3210116387802913281== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Tr=C3=B8nnes wrote: > = > = > Den 24.12.2018 00.10, skrev Peter Wu: > > On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Tr=C3=B8nnes 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 w= ill > > > > have some effect). After that, drm_fb_helper_initial_config is call= ed > > > > 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 bo= chs) > > > > 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 trigg= ered > > > > by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardo= wn"): > > > > = > > > > [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to = support this framebuffer > > > > [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbd= ev_setup] *ERROR* fbdev: Failed to set configuration (ret=3D-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=3Dn. > > > > = > > > > Link: https://lkml.kernel.org/r/20181221083226.GI23332(a)shao2-debi= an > > > > Link: https://lkml.kernel.org/r/20181223004315.GA11455(a)al > > > > Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/= teardown()") > > > > Reported-by: kernel test robot > > > > Cc: Noralf Tr=C3=B8nnes > > > > Signed-off-by: Peter Wu > > > > --- > > > > 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_devi= ce *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 bo= chs, 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 --===============3210116387802913281==-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Wu Subject: Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Date: Mon, 24 Dec 2018 16:03:02 +0100 Message-ID: <20181224150302.GA2259@al> References: <20181223004315.GA11455@al> <20181223005507.28328-1-peter@lekensteyn.nl> <20181223231033.GA31596@al> <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail.lekensteyn.nl (mail.lekensteyn.nl [IPv6:2a02:2308::360:1:25]) by gabe.freedesktop.org (Postfix) with ESMTPS id AC5CC6E5E5 for ; Mon, 24 Dec 2018 15:03:10 +0000 (UTC) Content-Disposition: inline In-Reply-To: <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: lkp@01.org, rong.a.chen@intel.com, Daniel Vetter , Linux List Kernel Mailing , dri-devel@lists.freedesktop.org, kraxel@redhat.com, Linus Torvalds List-Id: dri-devel@lists.freedesktop.org T24gTW9uLCBEZWMgMjQsIDIwMTggYXQgMDM6NTI6NTVQTSArMDEwMCwgTm9yYWxmIFRyw7hubmVz IHdyb3RlOgo+IAo+IAo+IERlbiAyNC4xMi4yMDE4IDAwLjEwLCBza3JldiBQZXRlciBXdToKPiA+ IE9uIFN1biwgRGVjIDIzLCAyMDE4IGF0IDAyOjU1OjUyUE0gKzAxMDAsIE5vcmFsZiBUcsO4bm5l cyB3cm90ZToKPiA+ID4gCj4gPiA+IAo+ID4gPiBEZW4gMjMuMTIuMjAxOCAwMS41NSwgc2tyZXYg UGV0ZXIgV3U6Cj4gPiA+ID4gQWZ0ZXIgZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cCBjYWxscyBk cm1fZmJfaGVscGVyX2luaXQsCj4gPiA+ID4gImRldi0+ZmJfaGVscGVyIiB3aWxsIGJlIGluaXRp YWxpemVkIChhbmQgdGh1cyBkcm1fZmJfaGVscGVyX2Zpbmkgd2lsbAo+ID4gPiA+IGhhdmUgc29t ZSBlZmZlY3QpLiBBZnRlciB0aGF0LCBkcm1fZmJfaGVscGVyX2luaXRpYWxfY29uZmlnIGlzIGNh bGxlZAo+ID4gPiA+IHdoaWNoIG1heSBjYWxsIHRoZSAiZmJfcHJvYmUiIGRyaXZlciBjYWxsYmFj ay4KPiA+ID4gPiAKPiA+ID4gPiBUaGlzIGRyaXZlciBjYWxsYmFjayBtYXkgY2FsbCBkcm1fZmJf aGVscGVyX2RlZmlvX2luaXQgKGFzIGlzIGRvbmUgYnkKPiA+ID4gPiBkcm1fZmJfaGVscGVyX2dl bmVyaWNfcHJvYmUpIG9yIHNldCBhIGZyYW1lYnVmZmVyIChhcyBpcyBkb25lIGJ5IGJvY2hzKQo+ ID4gPiA+IGFzIGRvY3VtZW50ZWQuIFRoZXNlIGFyZSBub3JtYWxseSBjbGVhbmVkIHVwIG9uIGV4 aXQgYnkKPiA+ID4gPiBkcm1fZmJfaGVscGVyX2ZiZGV2X3RlYXJkb3duIHdoaWNoIGFsc28gY2Fs bHMgZHJtX2ZiX2hlbHBlcl9maW5pLgo+ID4gPiA+IAo+ID4gPiA+IElmIGFuIGVycm9yIG9jY3Vy cyBhZnRlciAiZmJfcHJvYmUiLCBidXQgYmVmb3JlIHNldHVwIGlzIGNvbXBsZXRlLCB0aGVuCj4g PiA+ID4gY2FsbGluZyBqdXN0IGRybV9mYl9oZWxwZXJfZmluaSB3aWxsIGxlYWsgcmVzb3VyY2Vz LiBUaGlzIHdhcyB0cmlnZ2VyZWQKPiA+ID4gPiBieSBkZjIwNTJjYzkyMiAoImJvY2hzOiBjb252 ZXJ0IHRvIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAvdGVhcmRvd24iKToKPiA+ID4gPiAKPiA+ ID4gPiAgICAgICBbICAgNTAuMDA4MDMwXSBib2Noc2RybWZiOiBlbmFibGUgQ09ORklHX0ZCX0xJ VFRMRV9FTkRJQU4gdG8gc3VwcG9ydCB0aGlzIGZyYW1lYnVmZmVyCj4gPiA+ID4gICAgICAgWyAg IDUwLjAwOTQzNl0gYm9jaHMtZHJtIDAwMDA6MDA6MDIuMDogW2RybTpkcm1fZmJfaGVscGVyX2Zi ZGV2X3NldHVwXSAqRVJST1IqIGZiZGV2OiBGYWlsZWQgdG8gc2V0IGNvbmZpZ3VyYXRpb24gKHJl dD0tMzgpCj4gPiA+ID4gICAgICAgWyAgIDUwLjAxMTQ1Nl0gW2RybV0gSW5pdGlhbGl6ZWQgYm9j aHMtZHJtIDEuMC4wIDIwMTMwOTI1IGZvciAwMDAwOjAwOjAyLjAgb24gbWlub3IgMgo+ID4gPiA+ ICAgICAgIFsgICA1MC4wMTM2MDRdIFdBUk5JTkc6IENQVTogMSBQSUQ6IDEgYXQgZHJpdmVycy9n cHUvZHJtL2RybV9tb2RlX2NvbmZpZy5jOjQ3NyBkcm1fbW9kZV9jb25maWdfY2xlYW51cCsweDI4 MC8weDJhMAo+ID4gPiA+ICAgICAgIFsgICA1MC4wMTYxNzVdIENQVTogMSBQSUQ6IDEgQ29tbTog c3dhcHBlci8wIFRhaW50ZWQ6IEcgICAgICAgICAgICAgICAgVCA0LjIwLjAtcmM3ICMxCj4gPiA+ ID4gICAgICAgWyAgIDUwLjAxNzczMl0gRUlQOiBkcm1fbW9kZV9jb25maWdfY2xlYW51cCsweDI4 MC8weDJhMAo+ID4gPiA+ICAgICAgIC4uLgo+ID4gPiA+ICAgICAgIFsgICA1MC4wMjMxNTVdIENh bGwgVHJhY2U6Cj4gPiA+ID4gICAgICAgWyAgIDUwLjAyMzE1NV0gID8gYm9jaHNfa21zX2Zpbmkr MHgxZS8weDMwCj4gPiA+ID4gICAgICAgWyAgIDUwLjAyMzE1NV0gID8gYm9jaHNfdW5sb2FkKzB4 MTgvMHg0MAo+ID4gPiA+IAo+ID4gPiA+IFRoaXMgY2FuIGJlIHJlcHJvZHVjZWQgd2l0aCBRRU1V IGFuZCBDT05GSUdfRkJfTElUVExFX0VORElBTj1uLgo+ID4gPiA+IAo+ID4gPiA+IExpbms6IGh0 dHBzOi8vbGttbC5rZXJuZWwub3JnL3IvMjAxODEyMjEwODMyMjYuR0kyMzMzMkBzaGFvMi1kZWJp YW4KPiA+ID4gPiBMaW5rOiBodHRwczovL2xrbWwua2VybmVsLm9yZy9yLzIwMTgxMjIzMDA0MzE1 LkdBMTE0NTVAYWwKPiA+ID4gPiBGaXhlczogODc0MTIxNjM5NmIyICgiZHJtL2ZiLWhlbHBlcjog QWRkIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAvdGVhcmRvd24oKSIpCj4gPiA+ID4gUmVwb3J0 ZWQtYnk6IGtlcm5lbCB0ZXN0IHJvYm90IDxyb25nLmEuY2hlbkBpbnRlbC5jb20+Cj4gPiA+ID4g Q2M6IE5vcmFsZiBUcsO4bm5lcyA8bm9yYWxmQHRyb25uZXMub3JnPgo+ID4gPiA+IFNpZ25lZC1v ZmYtYnk6IFBldGVyIFd1IDxwZXRlckBsZWtlbnN0ZXluLm5sPgo+ID4gPiA+IC0tLQo+ID4gPiA+ ICAgIGRyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgfCAyICstCj4gPiA+ID4gICAgMSBm aWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pCj4gPiA+ID4gCj4gPiA+ ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgYi9kcml2ZXJz L2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4gPiA+ID4gaW5kZXggOWQ2NGY4NzRmOTY1Li40MzJl MGYzYjkyNjcgMTAwNjQ0Cj4gPiA+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2RybV9mYl9oZWxw ZXIuYwo+ID4gPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMKPiA+ID4g PiBAQCAtMjg2MCw3ICsyODYwLDcgQEAgaW50IGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAoc3Ry dWN0IGRybV9kZXZpY2UgKmRldiwKPiA+ID4gPiAgICAJcmV0dXJuIDA7Cj4gPiA+ID4gICAgZXJy X2RybV9mYl9oZWxwZXJfZmluaToKPiA+ID4gPiAtCWRybV9mYl9oZWxwZXJfZmluaShmYl9oZWxw ZXIpOwo+ID4gPiA+ICsJZHJtX2ZiX2hlbHBlcl9mYmRldl90ZWFyZG93bihkZXYpOwo+ID4gPiAK PiA+ID4gVGhpcyBjaGFuZ2Ugd2lsbCBicmVhayB0aGUgZXJyb3IgcGF0aCBmb3IgZHJtX2ZiZGV2 X2dlbmVyaWNfc2V0dXAoKQo+ID4gPiBiZWNhdXNlIGRybV9mYl9oZWxwZXJfZ2VuZXJpY19wcm9i ZSgpIGNsZWFucyB1cCBvbiBlcnJvciBidXQgZG9lc24ndAo+ID4gPiBjbGVhciBkcm1fZmJfaGVs cGVyLT5mYiByZXN1bHRpbmcgaW4gYSBkb3VibGUgZHJtX2ZyYW1lYnVmZmVyX3JlbW92ZSgpLgo+ ID4gCj4gPiBUaGlzIHNob3VsZCBwcm9iYWJseSBjb25zaWRlcmVkIGEgYnVnIG9mIGRybV9mYl9o ZWxwZXJfZ2VuZXJpY19wcm9iZS4KPiA+IE93bmVyc2hpcCBvZiBmYl9oZWxwZXIgc2hvdWxkIHJl bWFpbiB3aXRoIHRoZSBjYWxsZXIuIFRoZSBjYWxsZXIgY2FuCj4gPiBkZXRlY3QgYW4gZXJyb3Ig YW5kIGFjdCBhY2NvcmRpbmdseS4KPiA+IAo+ID4gPiBNeSBhc3N1bXB0aW9uIGhhcyBiZWVuIHRo YXQgdGhlIGRybV9mYl9oZWxwZXJfZnVuY3MtPmZiX3Byb2JlIGNhbGxiYWNrCj4gPiA+IGNsZWFu cyB1cCBpdHMgcmVzb3VyY2VzIG9uIGVycm9yLiBDbGVhcmx5IHRoaXMgaXMgbm90IHRoZSBjYXNl IGZvciBib2Nocywgc28KPiA+ID4gbXkgdGFrZSBvbiB0aGlzIGlzIHRoYXQgYm9jaHNmYl9jcmVh dGUoKSBuZWVkcyB0byBjbGVhbiB1cCBvbiBlcnJvci4KPiA+IAo+ID4gVGhhdCBhc3N1bXB0aW9u IHN0aWxsIGhvbGRzIGZvciBib2Nocy4gVGhlIHByb2JsZW0gaXMgdGhpcyBzZXF1ZW5jZToKPiA+ IC0gZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cCBpcyBjYWxsZWQuCj4gPiAtIGZiX3Byb2JlIHN1 Y2NlZWRzICh0aGlzIGlzIGNydWNpYWwpLgo+ID4gLSByZWdpc3Rlcl9mcmFtZWJ1ZmZlciBmYWls cy4KPiA+IC0gZXJyb3IgcGF0aCBvZiBzZXR1cCBpcyB0cmlnZ2VyZWQuCj4gPiAKPiA+IEFzIGZi X2hlbHBlciBpcyBmdWxseSBzZXR1cCBieSBkcml2ZXJzLCB0aGUgZHJtX2ZiX2hlbHBlciBjb3Jl IHNob3VsZAo+ID4gZnVsbHkgZGVhbGxvY2F0ZSBpdCBhZ2FpbiBvbiB0aGUgZXJyb3IgcGF0aCBv ciBlbHNlIGEgbGVhayBvY2N1cnMuCj4gPiAKPiA+ID4gR2VyZCBoYXMgYSBwYXRjaHNldCB0aGF0 IHN3aXRjaGVzIGJvY2hzIG92ZXIgdG8gdGhlIGdlbmVyaWMgZmJkZXYKPiA+ID4gZW11bGF0aW9u LCBidXQgb2ZjIHRoYXQgZG9lc24ndCBoZWxwIHdpdGggNC4yMDoKPiA+ID4gaHR0cHM6Ly9wYXRj aHdvcmsuZnJlZWRlc2t0b3Aub3JnL3Nlcmllcy81NDI2OS8KPiA+IAo+ID4gQW5kIHRoYXQgZG9l cyBub3QgaGVscCB3aXRoIG90aGVyIHVzZXJzIG9mIHRoZSBkcm1fZmJfaGVscGVyIHdobyB1c2UK PiA+IGZ1bmN0aW9ucyBsaWtlIGRybV9mYl9oZWxwZXJfZGVmaW9faW5pdC4gVGhleSB3aWxsIGxp a2VseSBydW4gaW4gdGhlCj4gPiBzYW1lIHByb2JsZW0uCj4gPiAKPiA+IEkgZG9uJ3QgaGF2ZSBh IHdheSB0byB0ZXN0IHRpbnlkcm0gb3Igb3RoZXIgZHJpdmVycywgYnV0IGlmIHlvdSBmb3JjZQo+ ID4gcmVnaXN0ZXJfZnJhbWVidWZmZXIgdG8gZmFpbCwgeW91IHNob3VsZCBiZSBhYmxlIHRvIHJl cHJvZHVjZSB0aGUKPiA+IHByb2JsZW0gd2l0aCBkcm1fZmJfaGVscGVyX2dlbmVyaWNfcHJvYmUu Cj4gPiAKPiAKPiBOb3cgSSB1bmRlcnN0YW5kLiBJIGhhdmUgbG9va2VkIGF0IHRoZSBkcml2ZXJz IHRoYXQgdXNlIGRybV9mYl9oZWxwZXIKPiBhbmQgbm8gb25lIHNlZW0gdG8gaGFuZGxlIHRoZSBj YXNlIHdoZXJlIHJlZ2lzdGVyX2ZyYW1lYnVmZmVyKCkgaXMKPiBmYWlsaW5nLgo+IAo+IEhlcmUn cyB3aGF0IGRyaXZlcnMgZG8gd2hlbiBkcm1fZmJfaGVscGVyX2luaXRpYWxfY29uZmlnKCkgZmFp bHM6Cj4gCj4gRG9lc24ndCBjaGVjazoKPiBhbWRncHUKPiB2aXJ0aW8KPiAKPiBDYWxscyBkcm1f ZmJfaGVscGVyX2ZpbmkoKToKPiBhcm1hZGEKPiBhc3QKPiBleHlub3MKPiBnbWE1MDAKPiBoaXNp bGljb24KPiBtZ2FnMjAwCj4gbXNtCj4gbm91dmVhdQo+IG9tYXAKPiByYWRlb24KPiByb2NrY2hp cAo+IHRlZ3JhCj4gdWRsCj4gYm9jaHMgLSBVc2VzIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAo KQo+IHF4bCAtIFVzZXMgZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cCgpCj4gdmJveHZpZGVvIC0g VXNlcyBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwKCkKPiAKPiBNaWdodCBjbGVhbiB1cCwgbm90 IHN1cmU6Cj4gY2lycnVzCj4gCj4gTG9va3Mgc3VzcGljaW91czoKPiBpOTE1Cj4gCj4gSSBsb29r ZWQgYXQgYm9jaHMgYmVmb3JlIGl0IHN3aXRjaGVkIHRvIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0 dXAoKSBhbmQKPiBpdCBhbHNvIGp1c3QgY2FsbGVkIGRybV9mYl9oZWxwZXJfZmluaSgpLgo+IAo+ IEl0IGxvb2tzIGxpa2UgeW91J3ZlIHVuY292ZXJlZCBzb21ldGhpbmcgbm8gb25lIGhhcyB0aG91 Z2ggYWJvdXQgKG9yCj4gbm90IGltcGxlbWVudGVkIGF0IGxlYXN0KS4KPiAKPiBJdCdzIG5vdCBq dXN0IHRoZSBmcmFtZWJ1ZmZlciB0aGF0J3Mgbm90IGRlc3Ryb3llZCwgdGhlIGJ1ZmZlciBvYmpl Y3QKPiBpcyBhbHNvIGxlYWtlZC4gZHJtX21vZGVfY29uZmlnX2NsZWFudXAoKSB5ZWxscyBhYm91 dCB0aGUgZnJhbWVidWZmZXIKPiAoYW5kIGZyZWVzIGl0KSwgYnV0IHNheXMgbm90aGluZyBhYm91 dCB0aGUgYnVmZmVyIG9iamVjdC4gSXQgbWlnaHQgYmUKPiB0aGF0IGl0IGNhbid0IGV2ZW4gYmUg bWFkZSB0byBkZXRlY3QgdGhhdCBzaW5jZSBzb21lIGRyaXZlcnMgZG8gc3BlY2lhbAo+IHN0dWZm IGZvciB0aGUgZmJkZXYgYnVmZmVyLgo+IAo+IEknbGwgcGljayB1cCBvbiB0aGlzIGFuZCBkbyBz b21lIHRlc3RpbmcgYWZ0ZXIgdGhlIENocmlzdG1hcyBob2xpZGF5cy4KClRoYW5rcywgdGhlIHdh cm5pbmcgaXMgYmFkIGZvciBDSSAod2hpY2ggdXNlcyBRRU1VKSwgYnV0IG90aGVyd2lzZSBpdApz aG91bGQgbm90IGhhdmUgYW55IGVmZmVjdCBvbiByZWd1bGFyIHVzZXJzIHNvIGl0IGNhbiB3YWl0 LgotLSAKS2luZCByZWdhcmRzLApQZXRlciBXdQpodHRwczovL2xla2Vuc3RleW4ubmwKX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 467B9C64E75 for ; Mon, 24 Dec 2018 15:03:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 00E5C21871 for ; Mon, 24 Dec 2018 15:03:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lekensteyn.nl header.i=@lekensteyn.nl header.b="WX8iWyI9" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725991AbeLXPDV (ORCPT ); Mon, 24 Dec 2018 10:03:21 -0500 Received: from lekensteyn.nl ([178.21.112.251]:40181 "EHLO lekensteyn.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725827AbeLXPDV (ORCPT ); Mon, 24 Dec 2018 10:03:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lekensteyn.nl; s=s2048-2015-q1; h=In-Reply-To:Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=ohS3+PHUm3lehlZuXJw8J4yZcIA4GZafGJfx8hEq4NQ=; b=WX8iWyI9IZ89I72r5Ogspbnj/wP4I5AbgeghQT00xGWGvu5R+H6X0JC/X2ZWLJJFh8MpvwYRM2LHSSO+IXrdWGQqAqolRf5S6vfQimPWTlCQ4DUpapef8nWHkNC0QzWdwz6+AOpNfDprpBbJH5HKwtKDrIXSQ8dazkDBlJQ3So3q1yJx947lDLJKuCjmQAt0f+oJVcxSYUbzDmunlzpTHcVgzaAlsSBmH2hchfefm+EVTdErcvweZqARtpHVuvkmj2adq4Sx9VHHdVwPMwRzt+aq8jCbTXC8HbSYftbj2rV4j4ARFadAriFVe+ArJpc79anQ6IubjCTDJblFBuZw8Q==; Received: by lekensteyn.nl with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1gbRl2-0003o9-Hh; Mon, 24 Dec 2018 16:03:05 +0100 Date: Mon, 24 Dec 2018 16:03:02 +0100 From: Peter Wu To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: dri-devel@lists.freedesktop.org, Linus Torvalds , rong.a.chen@intel.com, kraxel@redhat.com, Daniel Vetter , Linux List Kernel Mailing , lkp@01.org Subject: Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Message-ID: <20181224150302.GA2259@al> References: <20181223004315.GA11455@al> <20181223005507.28328-1-peter@lekensteyn.nl> <20181223231033.GA31596@al> <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> User-Agent: Mutt/1.11.1 (2018-12-01) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > Cc: Noralf Trønnes > > > > Signed-off-by: Peter Wu > > > > --- > > > > 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