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=-7.1 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 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 8CC1BC64E79 for ; Mon, 24 Dec 2018 14:53:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2422421850 for ; Mon, 24 Dec 2018 14:53:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=tronnes.org header.i=@tronnes.org header.b="r2kIItjh" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725920AbeLXOxD (ORCPT ); Mon, 24 Dec 2018 09:53:03 -0500 Received: from smtp.domeneshop.no ([194.63.252.55]:43422 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725554AbeLXOxC (ORCPT ); Mon, 24 Dec 2018 09:53:02 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tronnes.org; s=ds201810; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date:Message-ID:From:References:Cc:To:Subject; bh=cbvsgE01GRArP8hO9aXLfHPwiNsSUv2KHI7XVVxeLWQ=; b=r2kIItjhmNpI84EfvK4iSRxVT4bd2nTqpLW+H2KcTTMkLpultuJhBB2yu80sMqDiv8CWPMPMYNWpJb33GeKZHS33owntSeZk3LyA8TEmUEGVIQhsdQW/vlcP48sxxeu6TmypXiO/rrwCv3cSxmaOAruI+sGk9z1B7t5+y0HhkGIEkE5eHU0WQEEf1XdNB9eJBhqesDdLTyQpwjESZfqv949ddsN/hAKrut9nDyzRn1pM+oUXey1kDXc31KWq/Xyw9VxRRJUtBxuWXcKF0yTmHMy45jr85+/YWPUO1zsMBzqwUYbGdxM+z2QqpYyXoUtQq4RJZCRxqMh1JoNqgFRFSQ==; Received: from 211.81-166-168.customer.lyse.net ([81.166.168.211]:56556 helo=[192.168.10.173]) by smtp.domeneshop.no with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1gbRbH-0005hR-Lw; Mon, 24 Dec 2018 15:52:59 +0100 Subject: Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup To: Peter Wu 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 References: <20181223004315.GA11455@al> <20181223005507.28328-1-peter@lekensteyn.nl> <20181223231033.GA31596@al> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> Date: Mon, 24 Dec 2018 15:52:55 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.3.3 MIME-Version: 1.0 In-Reply-To: <20181223231033.GA31596@al> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Noralf. From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Subject: Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup Date: Mon, 24 Dec 2018 15:52:55 +0100 Message-ID: <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> References: <20181223004315.GA11455@al> <20181223005507.28328-1-peter@lekensteyn.nl> <20181223231033.GA31596@al> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from smtp.domeneshop.no (smtp.domeneshop.no [IPv6:2a01:5b40:0:3005::1]) by gabe.freedesktop.org (Postfix) with ESMTPS id E215F6E5DB for ; Mon, 24 Dec 2018 14:53:02 +0000 (UTC) In-Reply-To: <20181223231033.GA31596@al> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Peter Wu 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 CgpEZW4gMjQuMTIuMjAxOCAwMC4xMCwgc2tyZXYgUGV0ZXIgV3U6Cj4gT24gU3VuLCBEZWMgMjMs IDIwMTggYXQgMDI6NTU6NTJQTSArMDEwMCwgTm9yYWxmIFRyw7hubmVzIHdyb3RlOgo+Pgo+Pgo+ PiBEZW4gMjMuMTIuMjAxOCAwMS41NSwgc2tyZXYgUGV0ZXIgV3U6Cj4+PiBBZnRlciBkcm1fZmJf aGVscGVyX2ZiZGV2X3NldHVwIGNhbGxzIGRybV9mYl9oZWxwZXJfaW5pdCwKPj4+ICJkZXYtPmZi X2hlbHBlciIgd2lsbCBiZSBpbml0aWFsaXplZCAoYW5kIHRodXMgZHJtX2ZiX2hlbHBlcl9maW5p IHdpbGwKPj4+IGhhdmUgc29tZSBlZmZlY3QpLiBBZnRlciB0aGF0LCBkcm1fZmJfaGVscGVyX2lu aXRpYWxfY29uZmlnIGlzIGNhbGxlZAo+Pj4gd2hpY2ggbWF5IGNhbGwgdGhlICJmYl9wcm9iZSIg ZHJpdmVyIGNhbGxiYWNrLgo+Pj4KPj4+IFRoaXMgZHJpdmVyIGNhbGxiYWNrIG1heSBjYWxsIGRy bV9mYl9oZWxwZXJfZGVmaW9faW5pdCAoYXMgaXMgZG9uZSBieQo+Pj4gZHJtX2ZiX2hlbHBlcl9n ZW5lcmljX3Byb2JlKSBvciBzZXQgYSBmcmFtZWJ1ZmZlciAoYXMgaXMgZG9uZSBieSBib2NocykK Pj4+IGFzIGRvY3VtZW50ZWQuIFRoZXNlIGFyZSBub3JtYWxseSBjbGVhbmVkIHVwIG9uIGV4aXQg YnkKPj4+IGRybV9mYl9oZWxwZXJfZmJkZXZfdGVhcmRvd24gd2hpY2ggYWxzbyBjYWxscyBkcm1f ZmJfaGVscGVyX2ZpbmkuCj4+Pgo+Pj4gSWYgYW4gZXJyb3Igb2NjdXJzIGFmdGVyICJmYl9wcm9i ZSIsIGJ1dCBiZWZvcmUgc2V0dXAgaXMgY29tcGxldGUsIHRoZW4KPj4+IGNhbGxpbmcganVzdCBk cm1fZmJfaGVscGVyX2Zpbmkgd2lsbCBsZWFrIHJlc291cmNlcy4gVGhpcyB3YXMgdHJpZ2dlcmVk Cj4+PiBieSBkZjIwNTJjYzkyMiAoImJvY2hzOiBjb252ZXJ0IHRvIGRybV9mYl9oZWxwZXJfZmJk ZXZfc2V0dXAvdGVhcmRvd24iKToKPj4+Cj4+PiAgICAgICBbICAgNTAuMDA4MDMwXSBib2Noc2Ry bWZiOiBlbmFibGUgQ09ORklHX0ZCX0xJVFRMRV9FTkRJQU4gdG8gc3VwcG9ydCB0aGlzIGZyYW1l YnVmZmVyCj4+PiAgICAgICBbICAgNTAuMDA5NDM2XSBib2Nocy1kcm0gMDAwMDowMDowMi4wOiBb ZHJtOmRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXBdICpFUlJPUiogZmJkZXY6IEZhaWxlZCB0byBz ZXQgY29uZmlndXJhdGlvbiAocmV0PS0zOCkKPj4+ICAgICAgIFsgICA1MC4wMTE0NTZdIFtkcm1d IEluaXRpYWxpemVkIGJvY2hzLWRybSAxLjAuMCAyMDEzMDkyNSBmb3IgMDAwMDowMDowMi4wIG9u IG1pbm9yIDIKPj4+ICAgICAgIFsgICA1MC4wMTM2MDRdIFdBUk5JTkc6IENQVTogMSBQSUQ6IDEg YXQgZHJpdmVycy9ncHUvZHJtL2RybV9tb2RlX2NvbmZpZy5jOjQ3NyBkcm1fbW9kZV9jb25maWdf Y2xlYW51cCsweDI4MC8weDJhMAo+Pj4gICAgICAgWyAgIDUwLjAxNjE3NV0gQ1BVOiAxIFBJRDog MSBDb21tOiBzd2FwcGVyLzAgVGFpbnRlZDogRyAgICAgICAgICAgICAgICBUIDQuMjAuMC1yYzcg IzEKPj4+ICAgICAgIFsgICA1MC4wMTc3MzJdIEVJUDogZHJtX21vZGVfY29uZmlnX2NsZWFudXAr MHgyODAvMHgyYTAKPj4+ICAgICAgIC4uLgo+Pj4gICAgICAgWyAgIDUwLjAyMzE1NV0gQ2FsbCBU cmFjZToKPj4+ICAgICAgIFsgICA1MC4wMjMxNTVdICA/IGJvY2hzX2ttc19maW5pKzB4MWUvMHgz MAo+Pj4gICAgICAgWyAgIDUwLjAyMzE1NV0gID8gYm9jaHNfdW5sb2FkKzB4MTgvMHg0MAo+Pj4K Pj4+IFRoaXMgY2FuIGJlIHJlcHJvZHVjZWQgd2l0aCBRRU1VIGFuZCBDT05GSUdfRkJfTElUVExF X0VORElBTj1uLgo+Pj4KPj4+IExpbms6IGh0dHBzOi8vbGttbC5rZXJuZWwub3JnL3IvMjAxODEy MjEwODMyMjYuR0kyMzMzMkBzaGFvMi1kZWJpYW4KPj4+IExpbms6IGh0dHBzOi8vbGttbC5rZXJu ZWwub3JnL3IvMjAxODEyMjMwMDQzMTUuR0ExMTQ1NUBhbAo+Pj4gRml4ZXM6IDg3NDEyMTYzOTZi MiAoImRybS9mYi1oZWxwZXI6IEFkZCBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwL3RlYXJkb3du KCkiKQo+Pj4gUmVwb3J0ZWQtYnk6IGtlcm5lbCB0ZXN0IHJvYm90IDxyb25nLmEuY2hlbkBpbnRl bC5jb20+Cj4+PiBDYzogTm9yYWxmIFRyw7hubmVzIDxub3JhbGZAdHJvbm5lcy5vcmc+Cj4+PiBT aWduZWQtb2ZmLWJ5OiBQZXRlciBXdSA8cGV0ZXJAbGVrZW5zdGV5bi5ubD4KPj4+IC0tLQo+Pj4g ICAgZHJpdmVycy9ncHUvZHJtL2RybV9mYl9oZWxwZXIuYyB8IDIgKy0KPj4+ICAgIDEgZmlsZSBj aGFuZ2VkLCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlvbigtKQo+Pj4KPj4+IGRpZmYgLS1naXQg YS9kcml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jIGIvZHJpdmVycy9ncHUvZHJtL2RybV9m Yl9oZWxwZXIuYwo+Pj4gaW5kZXggOWQ2NGY4NzRmOTY1Li40MzJlMGYzYjkyNjcgMTAwNjQ0Cj4+ PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4+PiArKysgYi9kcml2ZXJz L2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4+PiBAQCAtMjg2MCw3ICsyODYwLDcgQEAgaW50IGRy bV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAoc3RydWN0IGRybV9kZXZpY2UgKmRldiwKPj4+ICAgIAly ZXR1cm4gMDsKPj4+ICAgIGVycl9kcm1fZmJfaGVscGVyX2Zpbmk6Cj4+PiAtCWRybV9mYl9oZWxw ZXJfZmluaShmYl9oZWxwZXIpOwo+Pj4gKwlkcm1fZmJfaGVscGVyX2ZiZGV2X3RlYXJkb3duKGRl dik7Cj4+Cj4+IFRoaXMgY2hhbmdlIHdpbGwgYnJlYWsgdGhlIGVycm9yIHBhdGggZm9yIGRybV9m YmRldl9nZW5lcmljX3NldHVwKCkKPj4gYmVjYXVzZSBkcm1fZmJfaGVscGVyX2dlbmVyaWNfcHJv YmUoKSBjbGVhbnMgdXAgb24gZXJyb3IgYnV0IGRvZXNuJ3QKPj4gY2xlYXIgZHJtX2ZiX2hlbHBl ci0+ZmIgcmVzdWx0aW5nIGluIGEgZG91YmxlIGRybV9mcmFtZWJ1ZmZlcl9yZW1vdmUoKS4KPiAK PiBUaGlzIHNob3VsZCBwcm9iYWJseSBjb25zaWRlcmVkIGEgYnVnIG9mIGRybV9mYl9oZWxwZXJf Z2VuZXJpY19wcm9iZS4KPiBPd25lcnNoaXAgb2YgZmJfaGVscGVyIHNob3VsZCByZW1haW4gd2l0 aCB0aGUgY2FsbGVyLiBUaGUgY2FsbGVyIGNhbgo+IGRldGVjdCBhbiBlcnJvciBhbmQgYWN0IGFj Y29yZGluZ2x5Lgo+IAo+PiBNeSBhc3N1bXB0aW9uIGhhcyBiZWVuIHRoYXQgdGhlIGRybV9mYl9o ZWxwZXJfZnVuY3MtPmZiX3Byb2JlIGNhbGxiYWNrCj4+IGNsZWFucyB1cCBpdHMgcmVzb3VyY2Vz IG9uIGVycm9yLiBDbGVhcmx5IHRoaXMgaXMgbm90IHRoZSBjYXNlIGZvciBib2Nocywgc28KPj4g bXkgdGFrZSBvbiB0aGlzIGlzIHRoYXQgYm9jaHNmYl9jcmVhdGUoKSBuZWVkcyB0byBjbGVhbiB1 cCBvbiBlcnJvci4KPiAKPiBUaGF0IGFzc3VtcHRpb24gc3RpbGwgaG9sZHMgZm9yIGJvY2hzLiBU aGUgcHJvYmxlbSBpcyB0aGlzIHNlcXVlbmNlOgo+IC0gZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1 cCBpcyBjYWxsZWQuCj4gLSBmYl9wcm9iZSBzdWNjZWVkcyAodGhpcyBpcyBjcnVjaWFsKS4KPiAt IHJlZ2lzdGVyX2ZyYW1lYnVmZmVyIGZhaWxzLgo+IC0gZXJyb3IgcGF0aCBvZiBzZXR1cCBpcyB0 cmlnZ2VyZWQuCj4gCj4gQXMgZmJfaGVscGVyIGlzIGZ1bGx5IHNldHVwIGJ5IGRyaXZlcnMsIHRo ZSBkcm1fZmJfaGVscGVyIGNvcmUgc2hvdWxkCj4gZnVsbHkgZGVhbGxvY2F0ZSBpdCBhZ2FpbiBv biB0aGUgZXJyb3IgcGF0aCBvciBlbHNlIGEgbGVhayBvY2N1cnMuCj4gCj4+IEdlcmQgaGFzIGEg cGF0Y2hzZXQgdGhhdCBzd2l0Y2hlcyBib2NocyBvdmVyIHRvIHRoZSBnZW5lcmljIGZiZGV2Cj4+ IGVtdWxhdGlvbiwgYnV0IG9mYyB0aGF0IGRvZXNuJ3QgaGVscCB3aXRoIDQuMjA6Cj4+IGh0dHBz Oi8vcGF0Y2h3b3JrLmZyZWVkZXNrdG9wLm9yZy9zZXJpZXMvNTQyNjkvCj4gCj4gQW5kIHRoYXQg ZG9lcyBub3QgaGVscCB3aXRoIG90aGVyIHVzZXJzIG9mIHRoZSBkcm1fZmJfaGVscGVyIHdobyB1 c2UKPiBmdW5jdGlvbnMgbGlrZSBkcm1fZmJfaGVscGVyX2RlZmlvX2luaXQuIFRoZXkgd2lsbCBs aWtlbHkgcnVuIGluIHRoZQo+IHNhbWUgcHJvYmxlbS4KPiAKPiBJIGRvbid0IGhhdmUgYSB3YXkg dG8gdGVzdCB0aW55ZHJtIG9yIG90aGVyIGRyaXZlcnMsIGJ1dCBpZiB5b3UgZm9yY2UKPiByZWdp c3Rlcl9mcmFtZWJ1ZmZlciB0byBmYWlsLCB5b3Ugc2hvdWxkIGJlIGFibGUgdG8gcmVwcm9kdWNl IHRoZQo+IHByb2JsZW0gd2l0aCBkcm1fZmJfaGVscGVyX2dlbmVyaWNfcHJvYmUuCj4gCgpOb3cg SSB1bmRlcnN0YW5kLiBJIGhhdmUgbG9va2VkIGF0IHRoZSBkcml2ZXJzIHRoYXQgdXNlIGRybV9m Yl9oZWxwZXIKYW5kIG5vIG9uZSBzZWVtIHRvIGhhbmRsZSB0aGUgY2FzZSB3aGVyZSByZWdpc3Rl cl9mcmFtZWJ1ZmZlcigpIGlzCmZhaWxpbmcuCgpIZXJlJ3Mgd2hhdCBkcml2ZXJzIGRvIHdoZW4g ZHJtX2ZiX2hlbHBlcl9pbml0aWFsX2NvbmZpZygpIGZhaWxzOgoKRG9lc24ndCBjaGVjazoKYW1k Z3B1CnZpcnRpbwoKQ2FsbHMgZHJtX2ZiX2hlbHBlcl9maW5pKCk6CmFybWFkYQphc3QKZXh5bm9z CmdtYTUwMApoaXNpbGljb24KbWdhZzIwMAptc20Kbm91dmVhdQpvbWFwCnJhZGVvbgpyb2NrY2hp cAp0ZWdyYQp1ZGwKYm9jaHMgLSBVc2VzIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAoKQpxeGwg LSBVc2VzIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAoKQp2Ym94dmlkZW8gLSBVc2VzIGRybV9m Yl9oZWxwZXJfZmJkZXZfc2V0dXAoKQoKTWlnaHQgY2xlYW4gdXAsIG5vdCBzdXJlOgpjaXJydXMK Ckxvb2tzIHN1c3BpY2lvdXM6Cmk5MTUKCkkgbG9va2VkIGF0IGJvY2hzIGJlZm9yZSBpdCBzd2l0 Y2hlZCB0byBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwKCkgYW5kCml0IGFsc28ganVzdCBjYWxs ZWQgZHJtX2ZiX2hlbHBlcl9maW5pKCkuCgpJdCBsb29rcyBsaWtlIHlvdSd2ZSB1bmNvdmVyZWQg c29tZXRoaW5nIG5vIG9uZSBoYXMgdGhvdWdoIGFib3V0IChvcgpub3QgaW1wbGVtZW50ZWQgYXQg bGVhc3QpLgoKSXQncyBub3QganVzdCB0aGUgZnJhbWVidWZmZXIgdGhhdCdzIG5vdCBkZXN0cm95 ZWQsIHRoZSBidWZmZXIgb2JqZWN0CmlzIGFsc28gbGVha2VkLiBkcm1fbW9kZV9jb25maWdfY2xl YW51cCgpIHllbGxzIGFib3V0IHRoZSBmcmFtZWJ1ZmZlcgooYW5kIGZyZWVzIGl0KSwgYnV0IHNh eXMgbm90aGluZyBhYm91dCB0aGUgYnVmZmVyIG9iamVjdC4gSXQgbWlnaHQgYmUKdGhhdCBpdCBj YW4ndCBldmVuIGJlIG1hZGUgdG8gZGV0ZWN0IHRoYXQgc2luY2Ugc29tZSBkcml2ZXJzIGRvIHNw ZWNpYWwKc3R1ZmYgZm9yIHRoZSBmYmRldiBidWZmZXIuCgpJJ2xsIHBpY2sgdXAgb24gdGhpcyBh bmQgZG8gc29tZSB0ZXN0aW5nIGFmdGVyIHRoZSBDaHJpc3RtYXMgaG9saWRheXMuCgpOb3JhbGYu Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xp c3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============4383417594877528738==" MIME-Version: 1.0 From: Noralf Trønnes 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 15:52:55 +0100 Message-ID: <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> In-Reply-To: <20181223231033.GA31596@al> List-Id: --===============4383417594877528738== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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 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 supp= ort this framebuffer >>> [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_s= etup] *ERROR* fbdev: Failed to set configuration (ret=3D-38) >>> [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 000= 0: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-debian >>> Link: https://lkml.kernel.org/r/20181223004315.GA11455(a)al >>> Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/tear= down()") >>> 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_h= elper.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. --===============4383417594877528738==--