[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue |
Date: |
Mon, 09 Feb 2015 10:09:43 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 09/02/2015 07:50, Bin Wu wrote:
> 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.
>
> 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. Only in this context, when the current
> coroutine gets execution control again(after the execution of
> qemu_coroutine_switch), we can restart the target coutine because the
> target coutine has yielded back to the current coroutine or it has
> terminated.
qemu_coroutine_yield can be executed for other reasons than locks. In
those cases, it is correct to call qemu_co_queue_run_restart. I think
it's an NBD bug.
Paolo
> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
> but we find we can not access the target coroutine if it terminates.
>
> Signed-off-by: Bin Wu <address@hidden>
> ---
> qemu-coroutine.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..cc0bdfa 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
> qemu_coroutine_delete(co);
> }
>
> -static void coroutine_swap(Coroutine *from, Coroutine *to)
> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
> {
> CoroutineAction ret;
>
> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>
> - qemu_co_queue_run_restart(to);
> -
> switch (ret) {
> case COROUTINE_YIELD:
> - return;
> + break;
> case COROUTINE_TERMINATE:
> trace_qemu_coroutine_terminate(to);
> + qemu_co_queue_run_restart(to);
> coroutine_delete(to);
> - return;
> + break;
> default:
> abort();
> }
> +
> + return ret;
> }
>
> void qemu_coroutine_enter(Coroutine *co, void *opaque)
> {
> Coroutine *self = qemu_coroutine_self();
> + CoroutineAction ret;
>
> trace_qemu_coroutine_enter(self, co, opaque);
>
> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>
> co->caller = self;
> co->entry_arg = opaque;
> - coroutine_swap(self, co);
> + ret = coroutine_swap(self, co);
> + if (ret == COROUTINE_YIELD)
> + qemu_co_queue_run_restart(co);
> }
>
> void coroutine_fn qemu_coroutine_yield(void)
>