* [PATCHv2] virtio_mmio: fix devm cleanup
@ 2017-12-12 13:45 Mark Rutland
2017-12-12 14:26 ` weiping zhang
2017-12-12 17:02 ` Cornelia Huck
0 siblings, 2 replies; 8+ messages in thread
From: Mark Rutland @ 2017-12-12 13:45 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, Cornelia Huck, Michael S . Tsirkin, weiping zhang,
virtualization
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 13:45 [PATCHv2] virtio_mmio: fix devm cleanup Mark Rutland
@ 2017-12-12 14:26 ` weiping zhang
2017-12-12 14:45 ` Mark Rutland
2017-12-12 17:02 ` Cornelia Huck
1 sibling, 1 reply; 8+ messages in thread
From: weiping zhang @ 2017-12-12 14:26 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Cornelia Huck, weiping zhang, virtualization,
Michael S . Tsirkin
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 14:26 ` weiping zhang
@ 2017-12-12 14:45 ` Mark Rutland
2017-12-12 15:04 ` weiping zhang
0 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2017-12-12 14:45 UTC (permalink / raw)
To: weiping zhang
Cc: linux-kernel, Cornelia Huck, weiping zhang, virtualization,
Michael S . Tsirkin
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 14:45 ` Mark Rutland
@ 2017-12-12 15:04 ` weiping zhang
0 siblings, 0 replies; 8+ messages in thread
From: weiping zhang @ 2017-12-12 15:04 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Cornelia Huck, weiping zhang, virtualization,
Michael S . Tsirkin
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 13:45 [PATCHv2] virtio_mmio: fix devm cleanup Mark Rutland
2017-12-12 14:26 ` weiping zhang
@ 2017-12-12 17:02 ` Cornelia Huck
2017-12-13 14:34 ` Mark Rutland
1 sibling, 1 reply; 8+ messages in thread
From: Cornelia Huck @ 2017-12-12 17:02 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Michael S . Tsirkin, weiping zhang, virtualization
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>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 17:02 ` Cornelia Huck
@ 2017-12-13 14:34 ` Mark Rutland
2017-12-14 18:48 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2017-12-13 14:34 UTC (permalink / raw)
To: Cornelia Huck, Michael S . Tsirkin
Cc: linux-kernel, weiping zhang, virtualization
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-13 14:34 ` Mark Rutland
@ 2017-12-14 18:48 ` Michael S. Tsirkin
2017-12-15 1:48 ` weiping zhang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-12-14 18:48 UTC (permalink / raw)
To: Mark Rutland; +Cc: Cornelia Huck, linux-kernel, weiping zhang, virtualization
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!
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-14 18:48 ` Michael S. Tsirkin
@ 2017-12-15 1:48 ` weiping zhang
0 siblings, 0 replies; 8+ messages in thread
From: weiping zhang @ 2017-12-15 1:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mark Rutland, Cornelia Huck, weiping zhang,
Linux Kernel Mailing List, virtualization
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-15 1:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-12 13:45 [PATCHv2] virtio_mmio: fix devm cleanup Mark Rutland
2017-12-12 14:26 ` weiping zhang
2017-12-12 14:45 ` Mark Rutland
2017-12-12 15:04 ` weiping zhang
2017-12-12 17:02 ` Cornelia Huck
2017-12-13 14:34 ` Mark Rutland
2017-12-14 18:48 ` Michael S. Tsirkin
2017-12-15 1:48 ` weiping zhang
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).