[PATCHv2] virtio_mmio: fix devm cleanup

[PATCHv2] virtio_mmio: fix devm cleanup

[Mark Rutland]

Recent rework of the virtio_mmio probe/remove paths balanced a
devm_ioremap() with an iounmap() rather than its devm variant. This ends
up corrupting the devm datastructures, and results in the following
boot-time splat on arm64 under QEMU 2.9.0:

[    3.450397] ------------[ cut here ]------------
[    3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
[    3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
[    3.475898] Kernel panic - not syncing: panic_on_warn set ...
[    3.475898]
[    3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
[    3.513109] Hardware name: linux,dummy-virt (DT)
[    3.525382] Call trace:
[    3.531683]  dump_backtrace+0x0/0x368
[    3.543921]  show_stack+0x20/0x30
[    3.547767]  dump_stack+0x108/0x164
[    3.559584]  panic+0x25c/0x51c
[    3.569184]  __warn+0x29c/0x31c
[    3.576023]  report_bug+0x1d4/0x290
[    3.586069]  bug_handler.part.2+0x40/0x100
[    3.597820]  bug_handler+0x4c/0x88
[    3.608400]  brk_handler+0x11c/0x218
[    3.613430]  do_debug_exception+0xe8/0x318
[    3.627370]  el1_dbg+0x18/0x78
[    3.634037]  __vunmap+0x1b8/0x220
[    3.648747]  vunmap+0x6c/0xc0
[    3.653864]  __iounmap+0x44/0x58
[    3.659771]  devm_ioremap_release+0x34/0x68
[    3.672983]  release_nodes+0x404/0x880
[    3.683543]  devres_release_all+0x6c/0xe8
[    3.695692]  driver_probe_device+0x250/0x828
[    3.706187]  __driver_attach+0x190/0x210
[    3.717645]  bus_for_each_dev+0x14c/0x1f0
[    3.728633]  driver_attach+0x48/0x78
[    3.740249]  bus_add_driver+0x26c/0x5b8
[    3.752248]  driver_register+0x16c/0x398
[    3.757211]  __platform_driver_register+0xd8/0x128
[    3.770860]  virtio_mmio_init+0x1c/0x24
[    3.782671]  do_one_initcall+0xe0/0x398
[    3.791890]  kernel_init_freeable+0x594/0x660
[    3.798514]  kernel_init+0x18/0x190
[    3.810220]  ret_from_fork+0x10/0x18

To fix this, we can simply rip out the explicit cleanup that the devm
infrastructure will do for us when our probe function returns an error
code, or when our remove function returns.

We only need to ensure that we call put_device() if a call to
register_virtio_device() fails in the probe path.

Signed-off-by: Mark Rutland <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

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[weiping zhang]

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

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[Mark Rutland]

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.

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[weiping zhang]

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.

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[Cornelia Huck]

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>

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[Mark Rutland]

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.

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[Michael S. Tsirkin]

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!

Re: [PATCHv2] virtio_mmio: fix devm cleanup

[weiping zhang]

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