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 <[email protected]>
Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
Cc: Cornelia Huck <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: weiping zhang <[email protected]>
Cc: [email protected]
---
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/[email protected]
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
2017-12-12 21:45 GMT+08:00 Mark Rutland <[email protected]>:
> 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 <[email protected]>
> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> Cc: Cornelia Huck <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: weiping zhang <[email protected]>
> Cc: [email protected]
> ---
> 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/[email protected]
>
> 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
> [email protected]
> 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 <[email protected]>:
> 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 <[email protected]>:
> On Tue, Dec 12, 2017 at 10:26:24PM +0800, weiping zhang wrote:
>> 2017-12-12 21:45 GMT+08:00 Mark Rutland <[email protected]>:
>> 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 <[email protected]> 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 <[email protected]>
> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> Cc: Cornelia Huck <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: weiping zhang <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 13:45:50 +0000
> Mark Rutland <[email protected]> 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 <[email protected]>
> > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> > Cc: Cornelia Huck <[email protected]>
> > Cc: Michael S. Tsirkin <[email protected]>
> > Cc: weiping zhang <[email protected]>
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> > > Cc: Cornelia Huck <[email protected]>
> > > Cc: Michael S. Tsirkin <[email protected]>
> > > Cc: weiping zhang <[email protected]>
> > > Cc: [email protected]
> > > ---
> > > 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 <[email protected]>
>
> 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 <[email protected]>:
> 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 <[email protected]> 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 <[email protected]>
>> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
>> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
>> > > Cc: Cornelia Huck <[email protected]>
>> > > Cc: Michael S. Tsirkin <[email protected]>
>> > > Cc: weiping zhang <[email protected]>
>> > > Cc: [email protected]
>> > > ---
>> > > 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 <[email protected]>
>>
>> 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization