2017-12-15 2:48 GMT+08:00 Michael S. Tsirkin <mst@xxxxxxxxxx>:
> 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@xxxxxxx> 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@xxxxxxx>
>> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
>> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
>> > > Cc: Cornelia Huck <cohuck@xxxxxxxxxx>
>> > > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
>> > > Cc: weiping zhang <zhangweiping@xxxxxxxxxxxxxxx>
>> > > Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
>> > > ---
>> > > 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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
