* [PATCH] virtio_mmio: fix devm cleanup
@ 2017-12-12 12:53 Mark Rutland
2017-12-12 13:09 ` Cornelia Huck
0 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2017-12-12 12:53 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, Cornelia Huck, Michael S . Tsirkin, weiping zhang,
virtualization
Recent rework of the virtio_mmio probe path 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 failure 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. We only need to ensure that we call put_device() if a call to
register_virtio_device() fails.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
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 | 36 +++++++++---------------------------
1 file changed, 9 insertions(+), 27 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index a9192fe4f345..793b1085967f 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,20 +583,9 @@ 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;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_mmio: fix devm cleanup
2017-12-12 12:53 [PATCH] virtio_mmio: fix devm cleanup Mark Rutland
@ 2017-12-12 13:09 ` Cornelia Huck
2017-12-12 13:29 ` Mark Rutland
0 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2017-12-12 13:09 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Michael S . Tsirkin, weiping zhang, virtualization
On Tue, 12 Dec 2017 12:53:02 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> Recent rework of the virtio_mmio probe path 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 failure 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
>
Oops.
> 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. We only need to ensure that we call put_device() if a call to
> register_virtio_device() fails.
OK, that was the subtility I obviously missed. Reading through the
code, this seems correct (although I find the infrastructure a bit
unintuitive).
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> 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 | 36 +++++++++---------------------------
> 1 file changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index a9192fe4f345..793b1085967f 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,20 +583,9 @@ 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;
> }
>
Shouldn't the cleanup in _remove() then be removed as well?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_mmio: fix devm cleanup
2017-12-12 13:09 ` Cornelia Huck
@ 2017-12-12 13:29 ` Mark Rutland
2017-12-12 13:31 ` Cornelia Huck
0 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2017-12-12 13:29 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-kernel, Michael S . Tsirkin, weiping zhang, virtualization
On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 12:53:02 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > Recent rework of the virtio_mmio probe path 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 failure 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
>
> Oops.
>
> > 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. We only need to ensure that we call put_device() if a call to
> > register_virtio_device() fails.
>
> OK, that was the subtility I obviously missed. Reading through the
> code, this seems correct (although I find the infrastructure a bit
> unintuitive).
No worries.
> Shouldn't the cleanup in _remove() then be removed as well?
Ah, I'd missed that.
I believe that can be removed, yes.
Should I spin a v2 with that folded in?
Thanks,
Mark.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_mmio: fix devm cleanup
2017-12-12 13:29 ` Mark Rutland
@ 2017-12-12 13:31 ` Cornelia Huck
2017-12-12 13:34 ` Mark Rutland
0 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2017-12-12 13:31 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Michael S . Tsirkin, weiping zhang, virtualization
On Tue, 12 Dec 2017 13:29:02 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
> > Shouldn't the cleanup in _remove() then be removed as well?
>
> Ah, I'd missed that.
>
> I believe that can be removed, yes.
>
> Should I spin a v2 with that folded in?
I think it has the same devm_ioremap vs. iounmap discrepancy, so it
should probably be folded in.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_mmio: fix devm cleanup
2017-12-12 13:31 ` Cornelia Huck
@ 2017-12-12 13:34 ` Mark Rutland
0 siblings, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2017-12-12 13:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-kernel, Michael S . Tsirkin, weiping zhang, virtualization
On Tue, Dec 12, 2017 at 02:31:48PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 13:29:02 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
>
> > > Shouldn't the cleanup in _remove() then be removed as well?
> >
> > Ah, I'd missed that.
> >
> > I believe that can be removed, yes.
> >
> > Should I spin a v2 with that folded in?
>
> I think it has the same devm_ioremap vs. iounmap discrepancy, so it
> should probably be folded in.
Ah, yes. I'll spin a v2 now.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-12 13:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-12 12:53 [PATCH] virtio_mmio: fix devm cleanup Mark Rutland
2017-12-12 13:09 ` Cornelia Huck
2017-12-12 13:29 ` Mark Rutland
2017-12-12 13:31 ` Cornelia Huck
2017-12-12 13:34 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).