Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754742AbdLOBsa (ORCPT ); Thu, 14 Dec 2017 20:48:30 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:38924 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754538AbdLOBs2 (ORCPT ); Thu, 14 Dec 2017 20:48:28 -0500 X-Google-Smtp-Source: ACJfBosa2tKAXzLoMM1Cmcx3dAyW2iQtQeuJ4vMcuV3eeKMG4F3bffk8ByCN8E8TNo7ayl08O0ZHZ2b3QYVUl26jJw4= MIME-Version: 1.0 In-Reply-To: <20171214204808-mutt-send-email-mst@kernel.org> References: <20171212134550.18378-1-mark.rutland@arm.com> <20171212180223.2dfb3b40.cohuck@redhat.com> <20171213143413.e3efqns53333uf5g@lakrids.cambridge.arm.com> <20171214204808-mutt-send-email-mst@kernel.org> From: weiping zhang Date: Fri, 15 Dec 2017 09:48:26 +0800 Message-ID: Subject: Re: [PATCHv2] virtio_mmio: fix devm cleanup To: "Michael S. Tsirkin" Cc: Mark Rutland , Cornelia Huck , weiping zhang , Linux Kernel Mailing List , virtualization@lists.linux-foundation.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4912 Lines: 111 2017-12-15 2:48 GMT+08:00 Michael S. Tsirkin : > On Wed, Dec 13, 2017 at 02:34:14PM +0000, Mark Rutland wrote: >> On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote: >> > On Tue, 12 Dec 2017 13:45:50 +0000 >> > Mark Rutland wrote: >> > >> > > Recent rework of the virtio_mmio probe/remove paths balanced a >> > > devm_ioremap() with an iounmap() rather than its devm variant. This ends >> > > up corrupting the devm datastructures, and results in the following >> > > boot-time splat on arm64 under QEMU 2.9.0: >> > > >> > > [ 3.450397] ------------[ cut here ]------------ >> > > [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844) >> > > [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220 >> > > [ 3.475898] Kernel panic - not syncing: panic_on_warn set ... >> > > [ 3.475898] >> > > [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1 >> > > [ 3.513109] Hardware name: linux,dummy-virt (DT) >> > > [ 3.525382] Call trace: >> > > [ 3.531683] dump_backtrace+0x0/0x368 >> > > [ 3.543921] show_stack+0x20/0x30 >> > > [ 3.547767] dump_stack+0x108/0x164 >> > > [ 3.559584] panic+0x25c/0x51c >> > > [ 3.569184] __warn+0x29c/0x31c >> > > [ 3.576023] report_bug+0x1d4/0x290 >> > > [ 3.586069] bug_handler.part.2+0x40/0x100 >> > > [ 3.597820] bug_handler+0x4c/0x88 >> > > [ 3.608400] brk_handler+0x11c/0x218 >> > > [ 3.613430] do_debug_exception+0xe8/0x318 >> > > [ 3.627370] el1_dbg+0x18/0x78 >> > > [ 3.634037] __vunmap+0x1b8/0x220 >> > > [ 3.648747] vunmap+0x6c/0xc0 >> > > [ 3.653864] __iounmap+0x44/0x58 >> > > [ 3.659771] devm_ioremap_release+0x34/0x68 >> > > [ 3.672983] release_nodes+0x404/0x880 >> > > [ 3.683543] devres_release_all+0x6c/0xe8 >> > > [ 3.695692] driver_probe_device+0x250/0x828 >> > > [ 3.706187] __driver_attach+0x190/0x210 >> > > [ 3.717645] bus_for_each_dev+0x14c/0x1f0 >> > > [ 3.728633] driver_attach+0x48/0x78 >> > > [ 3.740249] bus_add_driver+0x26c/0x5b8 >> > > [ 3.752248] driver_register+0x16c/0x398 >> > > [ 3.757211] __platform_driver_register+0xd8/0x128 >> > > [ 3.770860] virtio_mmio_init+0x1c/0x24 >> > > [ 3.782671] do_one_initcall+0xe0/0x398 >> > > [ 3.791890] kernel_init_freeable+0x594/0x660 >> > > [ 3.798514] kernel_init+0x18/0x190 >> > > [ 3.810220] ret_from_fork+0x10/0x18 >> > > >> > > To fix this, we can simply rip out the explicit cleanup that the devm >> > > infrastructure will do for us when our probe function returns an error >> > > code, or when our remove function returns. >> > > >> > > We only need to ensure that we call put_device() if a call to >> > > register_virtio_device() fails in the probe path. >> > > >> > > Signed-off-by: Mark Rutland >> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") >> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") >> > > Cc: Cornelia Huck >> > > Cc: Michael S. Tsirkin >> > > Cc: weiping zhang >> > > Cc: virtualization@lists.linux-foundation.org >> > > --- >> > > drivers/virtio/virtio_mmio.c | 43 +++++++++---------------------------------- >> > > 1 file changed, 9 insertions(+), 34 deletions(-) >> > >> > In the hope that I have grokked the devm_* interface by now, >> > >> > Reviewed-by: Cornelia Huck >> >> Thanks! >> >> Michael, could you please queue this as a fix for v4.15? >> >> This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2, >> impacting our automated regression testing, and I'd very much like to >> get back to testing pure mainline rather than mainline + local fixes. >> >> Thanks, >> Mark. > > Yep, plan to. > Thanks! Sorry to bother again, As we know if we call device_register we should keep vdev alive until dev.release be called. As discuss with Cornelia before, > - return register_virtio_device(&vm_dev->vdev); > + rc = register_virtio_device(&vm_dev->vdev); > + if (rc) > + goto put_dev; > + return 0; > +put_dev: > + put_device(&vm_dev->vdev.dev); > Here you give up the extra reference from device_initialize(), which > may or may not be the last reference (since you don't know if > device_add() had already exposed the struct device to other code that > might have acquired a reference). As the device has an empty release > function, touching the device structure after that is not a real > problem, but... cuase devm_ interface will free vdev if probe fail, even dev.ref count not dev to 0. So devm_ interface, may be not very suitable for device_register. ______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization