From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751612AbdLLO02 (ORCPT ); Tue, 12 Dec 2017 09:26:28 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:44055 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750731AbdLLO00 (ORCPT ); Tue, 12 Dec 2017 09:26:26 -0500 X-Google-Smtp-Source: ACJfBoujy+j0HbFSI8sqzxI7hwQBgcUJ0U1RzUjXwxIlhVYf34ipqdeAxA3PCHVzuQg0FpZT463TIz5AgunHEMX6dpc= MIME-Version: 1.0 In-Reply-To: <20171212134550.18378-1-mark.rutland@arm.com> References: <20171212134550.18378-1-mark.rutland@arm.com> From: weiping zhang Date: Tue, 12 Dec 2017 22:26:24 +0800 Message-ID: Subject: Re: [PATCHv2] virtio_mmio: fix devm cleanup To: Mark Rutland Cc: linux-kernel@vger.kernel.org, Cornelia Huck , weiping zhang , virtualization@lists.linux-foundation.org, "Michael S . Tsirkin" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-12-12 21:45 GMT+08:00 Mark Rutland : > 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(-) > > Since v1 [1]: > * Fix cleanup in virtio_mmio_remove > > Mark. > > [1] https://lkml.kernel.org/r/20171212125302.6846-1-mark.rutland@arm.com > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index a9192fe4f345..c92131edfaba 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -522,10 +522,8 @@ static int virtio_mmio_probe(struct platform_device *pdev) > return -EBUSY; > > vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL); > - if (!vm_dev) { > - rc = -ENOMEM; > - goto free_mem; > - } > + if (!vm_dev) > + return -ENOMEM; > > vm_dev->vdev.dev.parent = &pdev->dev; > vm_dev->vdev.dev.release = virtio_mmio_release_dev; > @@ -535,17 +533,14 @@ static int virtio_mmio_probe(struct platform_device *pdev) > spin_lock_init(&vm_dev->lock); > > vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem)); > - if (vm_dev->base == NULL) { > - rc = -EFAULT; > - goto free_vmdev; > - } > + if (vm_dev->base == NULL) > + return -EFAULT; > > /* Check magic value */ > magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE); > if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) { > dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic); > - rc = -ENODEV; > - goto unmap; > + return -ENODEV; > } > > /* Check device version */ > @@ -553,8 +548,7 @@ static int virtio_mmio_probe(struct platform_device *pdev) > if (vm_dev->version < 1 || vm_dev->version > 2) { > dev_err(&pdev->dev, "Version %ld not supported!\n", > vm_dev->version); > - rc = -ENXIO; > - goto unmap; > + return -ENXIO; > } > > vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID); > @@ -563,8 +557,7 @@ static int virtio_mmio_probe(struct platform_device *pdev) > * virtio-mmio device with an ID 0 is a (dummy) placeholder > * with no function. End probing now with no error reported. > */ > - rc = -ENODEV; > - goto unmap; > + return -ENODEV; > } > vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID); > > @@ -590,33 +583,15 @@ static int virtio_mmio_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, vm_dev); > > rc = register_virtio_device(&vm_dev->vdev); > - if (rc) { > - iounmap(vm_dev->base); > - devm_release_mem_region(&pdev->dev, mem->start, > - resource_size(mem)); > + if (rc) > put_device(&vm_dev->vdev.dev); > - } > - return rc; > -unmap: > - iounmap(vm_dev->base); > -free_mem: > - devm_release_mem_region(&pdev->dev, mem->start, > - resource_size(mem)); > -free_vmdev: > - devm_kfree(&pdev->dev, vm_dev); > + > return rc; > } > > static int virtio_mmio_remove(struct platform_device *pdev) > { > struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev); > - struct resource *mem; > - > - iounmap(vm_dev->base); > - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (mem) > - devm_release_mem_region(&pdev->dev, mem->start, > - resource_size(mem)); > unregister_virtio_device(&vm_dev->vdev); > > return 0; > -- > 2.11.0 > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization Hi Mark, thanks your patch, I dig into these three devm_xxx funciton, all of them represented by a struct devres as following, struct devres_node { struct list_head entry; dr_release_t release; #ifdef CONFIG_DEBUG_DEVRES const char *name; size_t size; #endif }; struct devres { struct devres_node node; /* -- 3 pointers */ unsigned long long data[]; /* guarantee ull alignment */ }; 1) devm_request_mem_region -> __devm_request_region dr = devres_alloc(devm_region_release, sizeof(struct region_devres), "devm_region_release" will call __release_region to release resource 2) devm_kzalloc -> devm_kmalloc dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); "devm_kmalloc_release" is noop, do nothing. 3) devm_ioremap -> ... -> __devres_alloc_node ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL); devm_ioremap_release do iounmap so for case 2) above, we need a devm_kfree() before call register_virtio_device