| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAA70yB7qN_50xFFmx5Vxs61teSjBjm2jCzZY=XqwNiLU_dWMgw@mail.gmail.com>
Date: Tue, 12 Dec 2017 22:26:24 +0800
From: weiping zhang <zwp10758@...il.com>
To: Mark Rutland <mark.rutland@....com>
Cc: linux-kernel@...r.kernel.org, Cornelia Huck <cohuck@...hat.com>,
weiping zhang <zhangweiping@...ichuxing.com>,
virtualization@...ts.linux-foundation.org,
"Michael S . Tsirkin" <mst@...hat.com>
Subject: Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@....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@....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@...hat.com>
> Cc: Michael S. Tsirkin <mst@...hat.com>
> Cc: weiping zhang <zhangweiping@...ichuxing.com>
> Cc: virtualization@...ts.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@...ts.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
Powered by blists - more mailing lists