Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue

[Kevin Wolf]

Am 09.02.2015 um 07:50 hat Bin Wu geschrieben:
> From: Bin Wu <address@hidden>
> 
> We tested VMs migration with their disk images by drive_mirror. With
> migration, two VMs copyed large files between each other. During the
> test, a segfault occured. The stack was as follow:
> 
> (gdb) bt
> qemu-coroutine-lock.c:66
> to=0x7fa5a1798648) at qemu-coroutine.c:97
> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
> block/nbd-client.c:165
> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
> block/nbd-client.c:262
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
> block/nbd-client.c:296
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) 
> at
> block.c:3471
> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) 
> at
> block.c:3471
> coroutine-ucontext.c:121
> 
> After analyzing the stack and reviewing the code, we find the
> qemu_co_queue_run_restart should not be put in the coroutine_swap function 
> which
> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
> qemu_coroutine_enter needs to restart the co_queue.
> 
> The error scenario is as follow: coroutine C1 enters C2, C2 yields
> back to C1, then C1 ternimates and the related coroutine memory
> becomes invalid. After a while, the C2 coroutine is entered again.
> At this point, C1 is used as a parameter passed to
> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
> accesses an invalid memory and a segfault error ocurrs.

Why would C1 be used for the qemu_co_queue_run_restart() call? There is
only one caller, coroutine_swap(). It calls the function for the
coroutine that has just yielded or terminated. In the case of
termination, the coroutine is deleted only afterwards, in the switch
block in coroutine_swap().

> The qemu_co_queue_run_restart function re-enters coroutines waiting
> in the co_queue. However, this function should be only used int the
> qemu_coroutine_enter context.

It shouldn't make a difference. Any coroutine waiting in a CoQueue
should be ready to be restarted at either point.

Kevin