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 DB522C43387 for ; Tue, 8 Jan 2019 17:55:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9C7112064C for ; Tue, 8 Jan 2019 17:55:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=tronnes.org header.i=@tronnes.org header.b="gQVtm6/q" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729206AbfAHRze (ORCPT ); Tue, 8 Jan 2019 12:55:34 -0500 Received: from smtp.domeneshop.no ([194.63.252.55]:39131 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727484AbfAHRze (ORCPT ); Tue, 8 Jan 2019 12:55:34 -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:References:Cc:To:From:Subject; bh=LVduWQzrwS5PSAUjQlYPzQwExsx1wFXehB/TIaFeW/k=; b=gQVtm6/qtsTfheWNdjg6QO8DhlnYu0VSpgx5MaMdjtf6P7Jp9+wMzHFgbyIzrruV8zI1Vlt+2eaJjg17mNApRk82pALv8d6EUkXVd3WG9A0uUpOgyURJigkq00FxkNgMCa5/gbclPc0nN8Q+1KwS8KRJ+nzjUCwGAxXGoK68bPhravjurM98dgpRFnznptVAnmgnvEPwtAZWHZAnICCDwt6T7qAVCw3wrDBoKfZUrGyd5fvrs5xv5WFUs1zOpWXkJe3r+tJEPk/szuwYWjA4SGM4a8ZwKMMmW0dK0vQMHSlP4yc9MeB8QFxY3DLZrq/HdudAiUTu6we5pQGii2Q0TA==; Received: from 211.81-166-168.customer.lyse.net ([81.166.168.211]:55277 helo=[192.168.10.174]) by smtp.domeneshop.no with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1ggvb7-0002P0-M1; Tue, 08 Jan 2019 18:55:29 +0100 Subject: Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= 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 References: <20181223004315.GA11455@al> <20181223005507.28328-1-peter@lekensteyn.nl> <20181223231033.GA31596@al> <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> <20181224150302.GA2259@al> <564896a7-e232-70e2-dd01-fec265f731eb@tronnes.org> Message-ID: Date: Tue, 8 Jan 2019 18:55:26 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <564896a7-e232-70e2-dd01-fec265f731eb@tronnes.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>> 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. >> > > This patch is good as long as it's applied along side the fix[1] to the > generic emulation: > > Reviewed-by: Noralf Trønnes > > I can apply them both when I get an ack/rb on the other patch. > Applied to drm-misc-next. 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: Tue, 8 Jan 2019 18:55:26 +0100 Message-ID: References: <20181223004315.GA11455@al> <20181223005507.28328-1-peter@lekensteyn.nl> <20181223231033.GA31596@al> <80d98ab2-dbf8-8d77-e5ca-07995cef0f1b@tronnes.org> <20181224150302.GA2259@al> <564896a7-e232-70e2-dd01-fec265f731eb@tronnes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" 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 8C5FA6EB72 for ; Tue, 8 Jan 2019 17:55:31 +0000 (UTC) In-Reply-To: <564896a7-e232-70e2-dd01-fec265f731eb@tronnes.org> 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: rong.a.chen@intel.com, Daniel Vetter , Linux List Kernel Mailing , dri-devel@lists.freedesktop.org, Linus Torvalds , kraxel@redhat.com, lkp@01.org List-Id: dri-devel@lists.freedesktop.org CgpEZW4gMDUuMDEuMjAxOSAxOS4yNSwgc2tyZXYgTm9yYWxmIFRyw7hubmVzOgo+IAo+IAo+IERl biAyNC4xMi4yMDE4IDE2LjAzLCBza3JldiBQZXRlciBXdToKPj4gT24gTW9uLCBEZWMgMjQsIDIw MTggYXQgMDM6NTI6NTVQTSArMDEwMCwgTm9yYWxmIFRyw7hubmVzIHdyb3RlOgo+Pj4KPj4+Cj4+ PiBEZW4gMjQuMTIuMjAxOCAwMC4xMCwgc2tyZXYgUGV0ZXIgV3U6Cj4+Pj4gT24gU3VuLCBEZWMg MjMsIDIwMTggYXQgMDI6NTU6NTJQTSArMDEwMCwgTm9yYWxmIFRyw7hubmVzIHdyb3RlOgo+Pj4+ Pgo+Pj4+Pgo+Pj4+PiBEZW4gMjMuMTIuMjAxOCAwMS41NSwgc2tyZXYgUGV0ZXIgV3U6Cj4+Pj4+ PiBBZnRlciBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwIGNhbGxzIGRybV9mYl9oZWxwZXJfaW5p dCwKPj4+Pj4+ICJkZXYtPmZiX2hlbHBlciIgd2lsbCBiZSBpbml0aWFsaXplZCAoYW5kIHRodXMg ZHJtX2ZiX2hlbHBlcl9maW5pCj4+Pj4+PiB3aWxsCj4+Pj4+PiBoYXZlIHNvbWUgZWZmZWN0KS4g QWZ0ZXIgdGhhdCwgZHJtX2ZiX2hlbHBlcl9pbml0aWFsX2NvbmZpZyBpcyBjYWxsZWQKPj4+Pj4+ IHdoaWNoIG1heSBjYWxsIHRoZSAiZmJfcHJvYmUiIGRyaXZlciBjYWxsYmFjay4KPj4+Pj4+Cj4+ Pj4+PiBUaGlzIGRyaXZlciBjYWxsYmFjayBtYXkgY2FsbCBkcm1fZmJfaGVscGVyX2RlZmlvX2lu aXQgKGFzIGlzIGRvbmUgYnkKPj4+Pj4+IGRybV9mYl9oZWxwZXJfZ2VuZXJpY19wcm9iZSkgb3Ig c2V0IGEgZnJhbWVidWZmZXIgKGFzIGlzIGRvbmUgYnkKPj4+Pj4+IGJvY2hzKQo+Pj4+Pj4gYXMg ZG9jdW1lbnRlZC4gVGhlc2UgYXJlIG5vcm1hbGx5IGNsZWFuZWQgdXAgb24gZXhpdCBieQo+Pj4+ Pj4gZHJtX2ZiX2hlbHBlcl9mYmRldl90ZWFyZG93biB3aGljaCBhbHNvIGNhbGxzIGRybV9mYl9o ZWxwZXJfZmluaS4KPj4+Pj4+Cj4+Pj4+PiBJZiBhbiBlcnJvciBvY2N1cnMgYWZ0ZXIgImZiX3By b2JlIiwgYnV0IGJlZm9yZSBzZXR1cCBpcyBjb21wbGV0ZSwKPj4+Pj4+IHRoZW4KPj4+Pj4+IGNh bGxpbmcganVzdCBkcm1fZmJfaGVscGVyX2Zpbmkgd2lsbCBsZWFrIHJlc291cmNlcy4gVGhpcyB3 YXMKPj4+Pj4+IHRyaWdnZXJlZAo+Pj4+Pj4gYnkgZGYyMDUyY2M5MjIgKCJib2NoczogY29udmVy dCB0bwo+Pj4+Pj4gZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cC90ZWFyZG93biIpOgo+Pj4+Pj4K Pj4+Pj4+IMKgwqDCoMKgwqDCoCBbwqDCoCA1MC4wMDgwMzBdIGJvY2hzZHJtZmI6IGVuYWJsZSBD T05GSUdfRkJfTElUVExFX0VORElBTgo+Pj4+Pj4gdG8gc3VwcG9ydCB0aGlzIGZyYW1lYnVmZmVy Cj4+Pj4+PiDCoMKgwqDCoMKgwqAgW8KgwqAgNTAuMDA5NDM2XSBib2Nocy1kcm0gMDAwMDowMDow Mi4wOgo+Pj4+Pj4gW2RybTpkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwXSAqRVJST1IqIGZiZGV2 OiBGYWlsZWQgdG8gc2V0Cj4+Pj4+PiBjb25maWd1cmF0aW9uIChyZXQ9LTM4KQo+Pj4+Pj4gwqDC oMKgwqDCoMKgIFvCoMKgIDUwLjAxMTQ1Nl0gW2RybV0gSW5pdGlhbGl6ZWQgYm9jaHMtZHJtIDEu MC4wIDIwMTMwOTI1Cj4+Pj4+PiBmb3IgMDAwMDowMDowMi4wIG9uIG1pbm9yIDIKPj4+Pj4+IMKg wqDCoMKgwqDCoCBbwqDCoCA1MC4wMTM2MDRdIFdBUk5JTkc6IENQVTogMSBQSUQ6IDEgYXQKPj4+ Pj4+IGRyaXZlcnMvZ3B1L2RybS9kcm1fbW9kZV9jb25maWcuYzo0NzcKPj4+Pj4+IGRybV9tb2Rl X2NvbmZpZ19jbGVhbnVwKzB4MjgwLzB4MmEwCj4+Pj4+PiDCoMKgwqDCoMKgwqAgW8KgwqAgNTAu MDE2MTc1XSBDUFU6IDEgUElEOiAxIENvbW06IHN3YXBwZXIvMCBUYWludGVkOgo+Pj4+Pj4gR8Kg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBUIDQuMjAuMC1yYzcgIzEKPj4+Pj4+IMKgwqDC oMKgwqDCoCBbwqDCoCA1MC4wMTc3MzJdIEVJUDogZHJtX21vZGVfY29uZmlnX2NsZWFudXArMHgy ODAvMHgyYTAKPj4+Pj4+IMKgwqDCoMKgwqDCoCAuLi4KPj4+Pj4+IMKgwqDCoMKgwqDCoCBbwqDC oCA1MC4wMjMxNTVdIENhbGwgVHJhY2U6Cj4+Pj4+PiDCoMKgwqDCoMKgwqAgW8KgwqAgNTAuMDIz MTU1XcKgID8gYm9jaHNfa21zX2ZpbmkrMHgxZS8weDMwCj4+Pj4+PiDCoMKgwqDCoMKgwqAgW8Kg wqAgNTAuMDIzMTU1XcKgID8gYm9jaHNfdW5sb2FkKzB4MTgvMHg0MAo+Pj4+Pj4KPj4+Pj4+IFRo aXMgY2FuIGJlIHJlcHJvZHVjZWQgd2l0aCBRRU1VIGFuZCBDT05GSUdfRkJfTElUVExFX0VORElB Tj1uLgo+Pj4+Pj4KPj4+Pj4+IExpbms6IGh0dHBzOi8vbGttbC5rZXJuZWwub3JnL3IvMjAxODEy MjEwODMyMjYuR0kyMzMzMkBzaGFvMi1kZWJpYW4KPj4+Pj4+IExpbms6IGh0dHBzOi8vbGttbC5r ZXJuZWwub3JnL3IvMjAxODEyMjMwMDQzMTUuR0ExMTQ1NUBhbAo+Pj4+Pj4gRml4ZXM6IDg3NDEy MTYzOTZiMiAoImRybS9mYi1oZWxwZXI6IEFkZAo+Pj4+Pj4gZHJtX2ZiX2hlbHBlcl9mYmRldl9z ZXR1cC90ZWFyZG93bigpIikKPj4+Pj4+IFJlcG9ydGVkLWJ5OiBrZXJuZWwgdGVzdCByb2JvdCA8 cm9uZy5hLmNoZW5AaW50ZWwuY29tPgo+Pj4+Pj4gQ2M6IE5vcmFsZiBUcsO4bm5lcyA8bm9yYWxm QHRyb25uZXMub3JnPgo+Pj4+Pj4gU2lnbmVkLW9mZi1ieTogUGV0ZXIgV3UgPHBldGVyQGxla2Vu c3RleW4ubmw+Cj4+Pj4+PiAtLS0KPj4+Pj4+IMKgwqDCoCBkcml2ZXJzL2dwdS9kcm0vZHJtX2Zi X2hlbHBlci5jIHwgMiArLQo+Pj4+Pj4gwqDCoMKgIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlv bigrKSwgMSBkZWxldGlvbigtKQo+Pj4+Pj4KPj4+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dw dS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4+Pj4+PiBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVs cGVyLmMKPj4+Pj4+IGluZGV4IDlkNjRmODc0Zjk2NS4uNDMyZTBmM2I5MjY3IDEwMDY0NAo+Pj4+ Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2RybV9mYl9oZWxwZXIuYwo+Pj4+Pj4gKysrIGIvZHJp dmVycy9ncHUvZHJtL2RybV9mYl9oZWxwZXIuYwo+Pj4+Pj4gQEAgLTI4NjAsNyArMjg2MCw3IEBA IGludCBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwKHN0cnVjdAo+Pj4+Pj4gZHJtX2RldmljZSAq ZGV2LAo+Pj4+Pj4gwqDCoMKgwqDCoMKgwqAgcmV0dXJuIDA7Cj4+Pj4+PiDCoMKgwqAgZXJyX2Ry bV9mYl9oZWxwZXJfZmluaToKPj4+Pj4+IC3CoMKgwqAgZHJtX2ZiX2hlbHBlcl9maW5pKGZiX2hl bHBlcik7Cj4+Pj4+PiArwqDCoMKgIGRybV9mYl9oZWxwZXJfZmJkZXZfdGVhcmRvd24oZGV2KTsK Pj4+Pj4KPj4+Pj4gVGhpcyBjaGFuZ2Ugd2lsbCBicmVhayB0aGUgZXJyb3IgcGF0aCBmb3IgZHJt X2ZiZGV2X2dlbmVyaWNfc2V0dXAoKQo+Pj4+PiBiZWNhdXNlIGRybV9mYl9oZWxwZXJfZ2VuZXJp Y19wcm9iZSgpIGNsZWFucyB1cCBvbiBlcnJvciBidXQgZG9lc24ndAo+Pj4+PiBjbGVhciBkcm1f ZmJfaGVscGVyLT5mYiByZXN1bHRpbmcgaW4gYSBkb3VibGUKPj4+Pj4gZHJtX2ZyYW1lYnVmZmVy X3JlbW92ZSgpLgo+Pj4+Cj4+Pj4gVGhpcyBzaG91bGQgcHJvYmFibHkgY29uc2lkZXJlZCBhIGJ1 ZyBvZiBkcm1fZmJfaGVscGVyX2dlbmVyaWNfcHJvYmUuCj4+Pj4gT3duZXJzaGlwIG9mIGZiX2hl bHBlciBzaG91bGQgcmVtYWluIHdpdGggdGhlIGNhbGxlci4gVGhlIGNhbGxlciBjYW4KPj4+PiBk ZXRlY3QgYW4gZXJyb3IgYW5kIGFjdCBhY2NvcmRpbmdseS4KPj4+Pgo+Pj4+PiBNeSBhc3N1bXB0 aW9uIGhhcyBiZWVuIHRoYXQgdGhlIGRybV9mYl9oZWxwZXJfZnVuY3MtPmZiX3Byb2JlIGNhbGxi YWNrCj4+Pj4+IGNsZWFucyB1cCBpdHMgcmVzb3VyY2VzIG9uIGVycm9yLiBDbGVhcmx5IHRoaXMg aXMgbm90IHRoZSBjYXNlIGZvcgo+Pj4+PiBib2Nocywgc28KPj4+Pj4gbXkgdGFrZSBvbiB0aGlz IGlzIHRoYXQgYm9jaHNmYl9jcmVhdGUoKSBuZWVkcyB0byBjbGVhbiB1cCBvbiBlcnJvci4KPj4+ Pgo+Pj4+IFRoYXQgYXNzdW1wdGlvbiBzdGlsbCBob2xkcyBmb3IgYm9jaHMuIFRoZSBwcm9ibGVt IGlzIHRoaXMgc2VxdWVuY2U6Cj4+Pj4gLSBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwIGlzIGNh bGxlZC4KPj4+PiAtIGZiX3Byb2JlIHN1Y2NlZWRzICh0aGlzIGlzIGNydWNpYWwpLgo+Pj4+IC0g cmVnaXN0ZXJfZnJhbWVidWZmZXIgZmFpbHMuCj4+Pj4gLSBlcnJvciBwYXRoIG9mIHNldHVwIGlz IHRyaWdnZXJlZC4KPj4+Pgo+Pj4+IEFzIGZiX2hlbHBlciBpcyBmdWxseSBzZXR1cCBieSBkcml2 ZXJzLCB0aGUgZHJtX2ZiX2hlbHBlciBjb3JlIHNob3VsZAo+Pj4+IGZ1bGx5IGRlYWxsb2NhdGUg aXQgYWdhaW4gb24gdGhlIGVycm9yIHBhdGggb3IgZWxzZSBhIGxlYWsgb2NjdXJzLgo+Pj4+Cj4+ Pj4+IEdlcmQgaGFzIGEgcGF0Y2hzZXQgdGhhdCBzd2l0Y2hlcyBib2NocyBvdmVyIHRvIHRoZSBn ZW5lcmljIGZiZGV2Cj4+Pj4+IGVtdWxhdGlvbiwgYnV0IG9mYyB0aGF0IGRvZXNuJ3QgaGVscCB3 aXRoIDQuMjA6Cj4+Pj4+IGh0dHBzOi8vcGF0Y2h3b3JrLmZyZWVkZXNrdG9wLm9yZy9zZXJpZXMv NTQyNjkvCj4+Pj4KPj4+PiBBbmQgdGhhdCBkb2VzIG5vdCBoZWxwIHdpdGggb3RoZXIgdXNlcnMg b2YgdGhlIGRybV9mYl9oZWxwZXIgd2hvIHVzZQo+Pj4+IGZ1bmN0aW9ucyBsaWtlIGRybV9mYl9o ZWxwZXJfZGVmaW9faW5pdC4gVGhleSB3aWxsIGxpa2VseSBydW4gaW4gdGhlCj4+Pj4gc2FtZSBw cm9ibGVtLgo+Pj4+Cj4+Pj4gSSBkb24ndCBoYXZlIGEgd2F5IHRvIHRlc3QgdGlueWRybSBvciBv dGhlciBkcml2ZXJzLCBidXQgaWYgeW91IGZvcmNlCj4+Pj4gcmVnaXN0ZXJfZnJhbWVidWZmZXIg dG8gZmFpbCwgeW91IHNob3VsZCBiZSBhYmxlIHRvIHJlcHJvZHVjZSB0aGUKPj4+PiBwcm9ibGVt IHdpdGggZHJtX2ZiX2hlbHBlcl9nZW5lcmljX3Byb2JlLgo+Pj4+Cj4+Pgo+Pj4gTm93IEkgdW5k ZXJzdGFuZC4gSSBoYXZlIGxvb2tlZCBhdCB0aGUgZHJpdmVycyB0aGF0IHVzZSBkcm1fZmJfaGVs cGVyCj4+PiBhbmQgbm8gb25lIHNlZW0gdG8gaGFuZGxlIHRoZSBjYXNlIHdoZXJlIHJlZ2lzdGVy X2ZyYW1lYnVmZmVyKCkgaXMKPj4+IGZhaWxpbmcuCj4+Pgo+Pj4gSGVyZSdzIHdoYXQgZHJpdmVy cyBkbyB3aGVuIGRybV9mYl9oZWxwZXJfaW5pdGlhbF9jb25maWcoKSBmYWlsczoKPj4+Cj4+PiBE b2Vzbid0IGNoZWNrOgo+Pj4gYW1kZ3B1Cj4+PiB2aXJ0aW8KPj4+Cj4+PiBDYWxscyBkcm1fZmJf aGVscGVyX2ZpbmkoKToKPj4+IGFybWFkYQo+Pj4gYXN0Cj4+PiBleHlub3MKPj4+IGdtYTUwMAo+ Pj4gaGlzaWxpY29uCj4+PiBtZ2FnMjAwCj4+PiBtc20KPj4+IG5vdXZlYXUKPj4+IG9tYXAKPj4+ IHJhZGVvbgo+Pj4gcm9ja2NoaXAKPj4+IHRlZ3JhCj4+PiB1ZGwKPj4+IGJvY2hzIC0gVXNlcyBk cm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwKCkKPj4+IHF4bCAtIFVzZXMgZHJtX2ZiX2hlbHBlcl9m YmRldl9zZXR1cCgpCj4+PiB2Ym94dmlkZW8gLSBVc2VzIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0 dXAoKQo+Pj4KPj4+IE1pZ2h0IGNsZWFuIHVwLCBub3Qgc3VyZToKPj4+IGNpcnJ1cwo+Pj4KPj4+ IExvb2tzIHN1c3BpY2lvdXM6Cj4+PiBpOTE1Cj4+Pgo+Pj4gSSBsb29rZWQgYXQgYm9jaHMgYmVm b3JlIGl0IHN3aXRjaGVkIHRvIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAoKSBhbmQKPj4+IGl0 IGFsc28ganVzdCBjYWxsZWQgZHJtX2ZiX2hlbHBlcl9maW5pKCkuCj4+Pgo+Pj4gSXQgbG9va3Mg bGlrZSB5b3UndmUgdW5jb3ZlcmVkIHNvbWV0aGluZyBubyBvbmUgaGFzIHRob3VnaCBhYm91dCAo b3IKPj4+IG5vdCBpbXBsZW1lbnRlZCBhdCBsZWFzdCkuCj4+Pgo+Pj4gSXQncyBub3QganVzdCB0 aGUgZnJhbWVidWZmZXIgdGhhdCdzIG5vdCBkZXN0cm95ZWQsIHRoZSBidWZmZXIgb2JqZWN0Cj4+ PiBpcyBhbHNvIGxlYWtlZC4gZHJtX21vZGVfY29uZmlnX2NsZWFudXAoKSB5ZWxscyBhYm91dCB0 aGUgZnJhbWVidWZmZXIKPj4+IChhbmQgZnJlZXMgaXQpLCBidXQgc2F5cyBub3RoaW5nIGFib3V0 IHRoZSBidWZmZXIgb2JqZWN0LiBJdCBtaWdodCBiZQo+Pj4gdGhhdCBpdCBjYW4ndCBldmVuIGJl IG1hZGUgdG8gZGV0ZWN0IHRoYXQgc2luY2Ugc29tZSBkcml2ZXJzIGRvIHNwZWNpYWwKPj4+IHN0 dWZmIGZvciB0aGUgZmJkZXYgYnVmZmVyLgo+Pj4KPj4+IEknbGwgcGljayB1cCBvbiB0aGlzIGFu ZCBkbyBzb21lIHRlc3RpbmcgYWZ0ZXIgdGhlIENocmlzdG1hcyBob2xpZGF5cy4KPj4KPj4gVGhh bmtzLCB0aGUgd2FybmluZyBpcyBiYWQgZm9yIENJICh3aGljaCB1c2VzIFFFTVUpLCBidXQgb3Ro ZXJ3aXNlIGl0Cj4+IHNob3VsZCBub3QgaGF2ZSBhbnkgZWZmZWN0IG9uIHJlZ3VsYXIgdXNlcnMg c28gaXQgY2FuIHdhaXQuCj4+Cj4gCj4gVGhpcyBwYXRjaCBpcyBnb29kIGFzIGxvbmcgYXMgaXQn cyBhcHBsaWVkIGFsb25nIHNpZGUgdGhlIGZpeFsxXSB0byB0aGUKPiBnZW5lcmljIGVtdWxhdGlv bjoKPiAKPiBSZXZpZXdlZC1ieTogTm9yYWxmIFRyw7hubmVzIDxub3JhbGZAdHJvbm5lcy5vcmc+ Cj4gCj4gSSBjYW4gYXBwbHkgdGhlbSBib3RoIHdoZW4gSSBnZXQgYW4gYWNrL3JiIG9uIHRoZSBv dGhlciBwYXRjaC4KPiAKCkFwcGxpZWQgdG8gZHJtLW1pc2MtbmV4dC4KCk5vcmFsZi4KX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============8734289419063953879==" 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: Tue, 08 Jan 2019 18:55:26 +0100 Message-ID: In-Reply-To: <564896a7-e232-70e2-dd01-fec265f731eb@tronnes.org> List-Id: --===============8734289419063953879== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Den 05.01.2019 19.25, skrev Noralf Tr=C3=B8nnes: > = > = > Den 24.12.2018 16.03, skrev Peter Wu: >> 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 >>>>>> 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"): >>>>>> >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.008030] bochsd= rmfb: enable CONFIG_FB_LITTLE_ENDIAN >>>>>> to support this framebuffer >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.009436] bochs-= drm 0000:00:02.0: >>>>>> [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set >>>>>> configuration (ret=3D-38) >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.011456] [drm] = Initialized bochs-drm 1.0.0 20130925 >>>>>> for 0000:00:02.0 on minor 2 >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.013604] WARNIN= G: CPU: 1 PID: 1 at >>>>>> drivers/gpu/drm/drm_mode_config.c:477 >>>>>> drm_mode_config_cleanup+0x280/0x2a0 >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.016175] CPU: 1= PID: 1 Comm: swapper/0 Tainted: >>>>>> G=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 T 4.20.0-rc7 #1 >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.017732] EIP: d= rm_mode_config_cleanup+0x280/0x2a0 >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ... >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.023155] Call T= race: >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.023155]=C2=A0 = ? bochs_kms_fini+0x1e/0x30 >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [=C2=A0=C2=A0 50.023155]=C2=A0 = ? 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/teardown()") >>>>>> Reported-by: kernel test robot >>>>>> Cc: Noralf Tr=C3=B8nnes >>>>>> Signed-off-by: Peter Wu >>>>>> --- >>>>>> =C2=A0=C2=A0=C2=A0 drivers/gpu/drm/drm_fb_helper.c | 2 +- >>>>>> =C2=A0=C2=A0=C2=A0 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, >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; >>>>>> =C2=A0=C2=A0=C2=A0 err_drm_fb_helper_fini: >>>>>> -=C2=A0=C2=A0=C2=A0 drm_fb_helper_fini(fb_helper); >>>>>> +=C2=A0=C2=A0=C2=A0 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=C3=B8nnes > = > I can apply them both when I get an ack/rb on the other patch. > = Applied to drm-misc-next. Noralf. --===============8734289419063953879==--