Message ID | 20230629120526.7184-1-wsa+renesas@sang-engineering.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp9595043vqr; Thu, 29 Jun 2023 05:20:17 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ60KVifbZjl5vJEJESuwFg6b7inOQ5jekwLWZXg7eQ5us6g6s/7icGDvTSNE8jGL4BTZl6m X-Received: by 2002:a05:6a20:8f22:b0:111:ee3b:59a7 with SMTP id b34-20020a056a208f2200b00111ee3b59a7mr50199734pzk.5.1688041216683; Thu, 29 Jun 2023 05:20:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688041216; cv=none; d=google.com; s=arc-20160816; b=iSTFqH8ahQuoByERo3IPTOFxNqcXY8C/jAatojrlVOAV+jiGCbQ7smuYOFe3cGjRTd oDTimFZsH3jJ1egkLjjWDeDUjCGzHPW4qi4kLYflTsRJhyi6Gy+oIQ1Jsl4/hhckzTJt ndQ5I9H2Q7xyZ2pRtmH+199kpUPRCYoThcvPdil8aJhutfTit2f9riy4BBvE9lvnuZYV HT2VLlaJyUK5TT0lqr8qllmKqmgKGikMlMtTiPqFMTz0micwInFH2JKL/C8Nv5i/ssoL PQM5yTOSkHN5voT6Q/oi8xcZOtCyB/5x75XkQd0DKlP48j/pq9T5NzPWPghoIs3tWnGf IWKg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=PqLg8rAp2ECwt8uzRvRRgzi1vbVpdg/PUE+HMKUbCGA=; fh=AarkXCaMi7t+0UkStCDLO8nXlHASW2DcS6naoAtQ+Wk=; b=hYe6wsYd+MPCX1MJ5HaztdMI4ZvzyCXBMv9rLl+3zxv7bHUDkrNdYqI5I3W21G7qeT cWbXCRaCqUt1dhpdcFnumD0uHPbVLJSl3t84cH+hsijf1xoDAVsA0S3fjwCYctSe7UuW UVJ5WEjioDFmpNylTMHR9GaKqaIWvARrbw1E1DnJDanPGCL37BqHoSxuzqZ87gFvzUBx jZb+aHO+cwCcINGaVNI1/niyWCS3VsxZFsXrSPAG2YekcgVVezmMIQ+f+7yggAtFo0t+ U6lgGzcuKQlPd9IfiBAVf46bSu/kcBMgP3iTJNuyHv47L8Ms6ju7CtFqHEzg8m+Artvr Ystw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sang-engineering.com header.s=k1 header.b=kpCXlx4G; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bw9-20020a056a00408900b00666b7377d65si9259442pfb.389.2023.06.29.05.20.01; Thu, 29 Jun 2023 05:20:16 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@sang-engineering.com header.s=k1 header.b=kpCXlx4G; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232172AbjF2MGo (ORCPT <rfc822;ivan.orlov0322@gmail.com> + 99 others); Thu, 29 Jun 2023 08:06:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47564 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232197AbjF2MF7 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 29 Jun 2023 08:05:59 -0400 Received: from mail.zeus03.de (www.zeus03.de [194.117.254.33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A6C5735AF for <linux-kernel@vger.kernel.org>; Thu, 29 Jun 2023 05:05:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= sang-engineering.com; h=from:to:cc:subject:date:message-id :mime-version:content-transfer-encoding; s=k1; bh=PqLg8rAp2ECwt8 uzRvRRgzi1vbVpdg/PUE+HMKUbCGA=; b=kpCXlx4GiLSUJuHUwZCGw8XjAxrVmD 78tpwwmMEWQ0tyq2psZouoIpzB5XMll0WKNG4LZqjGsJS1oBNO1G/YloaK76St0i 6KTpsb7Ba1btJkbsT/gB20TPM2oHAaAYb2i8Xr7u6LE14b42SW0njsBxnvaihtRC o+SkQlSL/0pIPbvTb/N0jgxs2h1xsxl2ct6OaHIgdN8w9EU9bishvmTdyD84HBy0 Bl5thzE2aawSwOvgRyzoab/+bBtv7bnJ3tW0WTiYh7TBtk39uJHgWrTRh4U8FRLa tfDA/WsH3bV1dGwy8p80MWdp4kJRHcywS+0Nvs/6A0HGH2xDBkENWAYg== Received: (qmail 3752080 invoked from network); 29 Jun 2023 14:05:49 +0200 Received: by mail.zeus03.de with ESMTPSA (TLS_AES_256_GCM_SHA384 encrypted, authenticated); 29 Jun 2023 14:05:49 +0200 X-UD-Smtp-Session: l3s3148p1@TgA1hUP/wszBVfKA From: Wolfram Sang <wsa+renesas@sang-engineering.com> To: linux-renesas-soc@vger.kernel.org Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>, weiping zhang <zhangweiping@didichuxing.com>, Cornelia Huck <cohuck@redhat.com>, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Subject: [PATCH] virtio-mmio: don't break lifecycle of vm_dev Date: Thu, 29 Jun 2023 14:05:26 +0200 Message-Id: <20230629120526.7184-1-wsa+renesas@sang-engineering.com> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770039507092351121?= X-GMAIL-MSGID: =?utf-8?q?1770039507092351121?= |
Series |
virtio-mmio: don't break lifecycle of vm_dev
|
|
Commit Message
Wolfram Sang
June 29, 2023, 12:05 p.m. UTC
vm_dev has a separate lifecycle because it has a 'struct device'
embedded. Thus, having a release callback for it is correct.
Allocating the vm_dev struct with devres totally breaks this protection,
though. Instead of waiting for the vm_dev release callback, the memory
is freed when the platform_device is removed. Resulting in a
use-after-free when finally the callback is to be called.
To easily see the problem, compile the kernel with
CONFIG_DEBUG_KOBJECT_RELEASE and unbind with sysfs.
The fix is easy, don't use devres in this case.
Found during my research about object lifetime problems.
Fixes: 7eb781b1bbb7 ("virtio_mmio: add cleanup for virtio_mmio_probe")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/virtio/virtio_mmio.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Comments
On Thu, 29 Jun 2023 14:05:26 +0200, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > vm_dev has a separate lifecycle because it has a 'struct device' > embedded. Thus, having a release callback for it is correct. > > Allocating the vm_dev struct with devres totally breaks this protection, device? or driver? And why? > though. Instead of waiting for the vm_dev release callback, the memory > is freed when the platform_device is removed. Resulting in a > use-after-free when finally the callback is to be called. Can we have the break stack? Thanks. > > To easily see the problem, compile the kernel with > CONFIG_DEBUG_KOBJECT_RELEASE and unbind with sysfs. > > The fix is easy, don't use devres in this case. > > Found during my research about object lifetime problems. > > Fixes: 7eb781b1bbb7 ("virtio_mmio: add cleanup for virtio_mmio_probe") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/virtio/virtio_mmio.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index a46a4a29e929..97760f611295 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -607,9 +607,8 @@ static void virtio_mmio_release_dev(struct device *_d) > struct virtio_device *vdev = > container_of(_d, struct virtio_device, dev); > struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev); > - struct platform_device *pdev = vm_dev->pdev; > > - devm_kfree(&pdev->dev, vm_dev); > + kfree(vm_dev); > } > > /* Platform device */ > @@ -620,7 +619,7 @@ static int virtio_mmio_probe(struct platform_device *pdev) > unsigned long magic; > int rc; > > - vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL); > + vm_dev = kzalloc(sizeof(*vm_dev), GFP_KERNEL); > if (!vm_dev) > return -ENOMEM; > > -- > 2.35.1 >
> > Allocating the vm_dev struct with devres totally breaks this protection, > > device? or driver? devres is the short name for 'managed devices'. That is, all the devm_* functions. > And why? This is written in the below paragraph... > > though. Instead of waiting for the vm_dev release callback, the memory > > is freed when the platform_device is removed. Resulting in a > > use-after-free when finally the callback is to be called. ... right here. Sadly, the video of my talk about device lifetime issues is not online yet. But my slides are [1]. Please check pages 29-31. [1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf > Can we have the break stack? Sure! root@Lager:/sys/bus/platform/drivers/virtio-mmio# echo virtio-mmio.0 > unbind [ 25.258132] kobject: 'virtio0' ((ptrval)): kobject_release, parent 00000000 (delayed 300) [ 25.266592] ------------[ cut here ]------------ [ 25.271219] WARNING: CPU: 0 PID: 606 at lib/debugobjects.c:505 debug_print_object+0xa4/0xcc [ 25.279595] ODEBUG: free active (active state 0) object: (ptrval) object type: timer_list hint: kobject_delayed_cleanup+0x0/0x68 [ 25.291186] Modules linked in: [ 25.294250] CPU: 0 PID: 606 Comm: sh Tainted: G W 6.4.0-00001-g1279c634d910 #1215 [ 25.303134] Hardware name: Generic R-Car Gen2 (Flattened Device Tree) [ 25.309580] Backtrace: [ 25.312032] dump_backtrace from show_stack+0x20/0x24 [ 25.317104] r7:00000009 r6:c0745e4a r5:60000093 r4:c073ddc2 [ 25.322768] show_stack from dump_stack_lvl+0x2c/0x34 [ 25.327836] dump_stack_lvl from dump_stack+0x14/0x1c [ 25.332906] r5:c02fcc34 r4:c2db3880 [ 25.336484] dump_stack from __warn+0x90/0x118 [ 25.340944] __warn from warn_slowpath_fmt+0x88/0xcc [ 25.345921] r10:c2607c78 r9:00000000 r8:c0745e02 r7:00000009 r6:c02fcc34 r5:000001f9 [ 25.353759] r4:c0745e4a [ 25.356293] warn_slowpath_fmt from debug_print_object+0xa4/0xcc [ 25.362314] r8:c0b1a3b4 r7:c06020f8 r6:c0735cc1 r5:c0a0492c r4:c1004a08 [ 25.369019] debug_print_object from debug_check_no_obj_freed+0x184/0x1bc [ 25.375822] r8:c2607c00 r7:c06020f8 r6:c2db3880 r5:c2608000 r4:c2607000 [ 25.382527] debug_check_no_obj_freed from __kmem_cache_free+0x40/0x80 [ 25.389072] r10:00000004 r9:c2db3880 r8:c073257a r7:00000013 r6:c03952b0 r5:c2607c00 [ 25.396909] r4:c1000280 [ 25.399443] __kmem_cache_free from kfree+0x90/0x98 [ 25.404335] r7:f1581dec r6:c201e010 r5:c03952b0 r4:c2607c00 [ 25.409998] kfree from release_nodes+0x58/0x68 [ 25.414548] r7:f1581dec r6:c201e010 r5:f1581dec r4:c2607c00 [ 25.420210] release_nodes from devres_release_all+0xb8/0xe8 [ 25.425891] r9:c2db3880 r8:c0a9e1d0 r7:80000013 r6:c2db3880 r5:00000004 r4:c201e010 [ 25.433640] devres_release_all from device_unbind_cleanup+0x1c/0x70 [ 25.440022] r7:c0a99488 r6:00000000 r5:c0a99290 r4:c201e010 [ 25.445684] device_unbind_cleanup from device_release_driver_internal+0xb4/0x128 [ 25.453187] r5:c0a99290 r4:c201e010 [ 25.456764] device_release_driver_internal from device_driver_detach+0x20/0x24 [ 25.464088] r7:c0a99488 r6:c201e010 r5:c0a9e1d0 r4:0000000e [ 25.469750] device_driver_detach from unbind_store+0x60/0x94 [ 25.475512] unbind_store from drv_attr_store+0x34/0x40 [ 25.480759] r9:c2db3880 r8:f1581f00 r7:c2d43a10 r6:c2d43a00 r5:c2d37e40 r4:c038ecfc [ 25.488508] drv_attr_store from sysfs_kf_write+0x48/0x54 [ 25.493928] sysfs_kf_write from kernfs_fop_write_iter+0x160/0x1ac [ 25.500128] r5:c2d37e40 r4:0000000e [ 25.503705] kernfs_fop_write_iter from vfs_write+0x154/0x1b4 [ 25.509469] r8:00000000 r7:000f3070 r6:f1581f58 r5:c24a36c0 r4:0000000e [ 25.516173] vfs_write from ksys_write+0x84/0xd8 [ 25.520802] r8:0000000e r7:f1581f64 r6:f1581f58 r5:000f3070 r4:c24a36c0 [ 25.527507] ksys_write from sys_write+0x18/0x1c [ 25.532135] r8:c0100264 r7:00000004 r6:0000000e r5:000f3070 r4:00000001 [ 25.538839] sys_write from ret_fast_syscall+0x0/0x54 [ 25.543901] Exception stack(0xf1581fa8 to 0xf1581ff0) [ 25.548960] 1fa0: 00000001 000f3070 00000001 000f3070 0000000e 00000000 [ 25.557147] 1fc0: 00000001 000f3070 0000000e 00000004 000f09f4 00000020 000f1284 000f1418 [ 25.565333] 1fe0: 000f0250 beaca868 00018ef0 b6f40ae8 [ 25.570387] ---[ end trace 0000000000000000 ]---
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index a46a4a29e929..97760f611295 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c @@ -607,9 +607,8 @@ static void virtio_mmio_release_dev(struct device *_d) struct virtio_device *vdev = container_of(_d, struct virtio_device, dev); struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev); - struct platform_device *pdev = vm_dev->pdev; - devm_kfree(&pdev->dev, vm_dev); + kfree(vm_dev); } /* Platform device */ @@ -620,7 +619,7 @@ static int virtio_mmio_probe(struct platform_device *pdev) unsigned long magic; int rc; - vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL); + vm_dev = kzalloc(sizeof(*vm_dev), GFP_KERNEL); if (!vm_dev) return -ENOMEM;