| Message ID | 20171212134550.18378-1-mark.rutland@arm.com |
|---|---|
| State | Accepted |
| Commit | c2e90800aef22e7ea14ea7560ba99993f11d3616 |
| Headers | show |
| Series | [PATCHv2] virtio_mmio: fix devm cleanup | expand |
2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > 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 <mark.rutland@arm.com> > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") > Cc: Cornelia Huck <cohuck@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: weiping zhang <zhangweiping@didichuxing.com> > 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
On Tue, Dec 12, 2017 at 10:26:24PM +0800, weiping zhang wrote: > 2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > Hi Mark, Hi, > 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 */ > }; > 2) devm_kzalloc -> devm_kmalloc > > dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); > "devm_kmalloc_release" is noop, do nothing. Please note that the release function is there to perform cleanup prior to the devm infrastructure releasing the memory. The devm_kmalloc_release function is a no-op since nothing has to be done prior to memory being freed, but the memory itself is still freed. In alloc_dr(), the struct devres is allocated together with the memory, since alloc_dr() does: size_t tot_size = sizeof(struct devres) + size; struct devres *dr; dr = kmalloc_node_track_caller(tot_size, gfp, nid); return dr->data; ... where dr->data points at the memory after the struct devres. Later, in release_nodes() we do: list_for_each_entry_safe_reverse(dr, tmp, &todo, node.entry) { devres_log(dev, &dr->node, "REL"); dr->node.release(dev, dr->data); kfree(dr); } ... which will invoke the no-op devm_kmalloc_release, then free the devres allocation, including the dr->data memory the user requested. > so for case 2) above, we need a devm_kfree() before call > register_virtio_device As above, I do not believe that is the case. Thanks, Mark.
2017-12-12 22:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > On Tue, Dec 12, 2017 at 10:26:24PM +0800, weiping zhang wrote: >> 2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: >> Hi Mark, > > Hi, > >> 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 */ >> }; > >> 2) devm_kzalloc -> devm_kmalloc >> >> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); >> "devm_kmalloc_release" is noop, do nothing. > > Please note that the release function is there to perform cleanup prior > to the devm infrastructure releasing the memory. > > The devm_kmalloc_release function is a no-op since nothing has to be > done prior to memory being freed, but the memory itself is still freed. > > In alloc_dr(), the struct devres is allocated together with the memory, > since alloc_dr() does: > > size_t tot_size = sizeof(struct devres) + size; > struct devres *dr; > > dr = kmalloc_node_track_caller(tot_size, gfp, nid); > > return dr->data; > > ... where dr->data points at the memory after the struct devres. > > Later, in release_nodes() we do: > > list_for_each_entry_safe_reverse(dr, tmp, &todo, node.entry) { > devres_log(dev, &dr->node, "REL"); > dr->node.release(dev, dr->data); > kfree(dr); > } > > ... which will invoke the no-op devm_kmalloc_release, then free the > devres allocation, including the dr->data memory the user requested. > >> so for case 2) above, we need a devm_kfree() before call >> register_virtio_device > > As above, I do not believe that is the case. > Oh I see, thanks your detail explanation. Thanks a lot. > Thanks, > Mark.
On Tue, 12 Dec 2017 13:45:50 +0000 Mark Rutland <mark.rutland@arm.com> 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 <mark.rutland@arm.com> > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") > Cc: Cornelia Huck <cohuck@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: weiping zhang <zhangweiping@didichuxing.com> > 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 <cohuck@redhat.com>
On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote: > On Tue, 12 Dec 2017 13:45:50 +0000 > Mark Rutland <mark.rutland@arm.com> 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 <mark.rutland@arm.com> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") > > Cc: Cornelia Huck <cohuck@redhat.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: weiping zhang <zhangweiping@didichuxing.com> > > 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 <cohuck@redhat.com> 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.
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 <mark.rutland@arm.com> 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 <mark.rutland@arm.com> > > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") > > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") > > > Cc: Cornelia Huck <cohuck@redhat.com> > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Cc: weiping zhang <zhangweiping@didichuxing.com> > > > 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 <cohuck@redhat.com> > > 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!
2017-12-15 2:48 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>: > 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 <mark.rutland@arm.com> 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 <mark.rutland@arm.com> >> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") >> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") >> > > Cc: Cornelia Huck <cohuck@redhat.com> >> > > Cc: Michael S. Tsirkin <mst@redhat.com> >> > > Cc: weiping zhang <zhangweiping@didichuxing.com> >> > > 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 <cohuck@redhat.com> >> >> 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
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;
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 <mark.rutland@arm.com> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe") Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove") Cc: Cornelia Huck <cohuck@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: weiping zhang <zhangweiping@didichuxing.com> 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 -- 2.11.0