Re: [PATCH] migrate/multifd: fix coredump when the multifd thread cleanup

[Fabiano Rosas <farosas@suse.de>]

"chenyuhui (A)" via <qemu-devel@nongnu.org> writes:

> On 2023/7/26 0:53, Peter Xu wrote:
>> On Tue, Jul 25, 2023 at 04:43:28PM +0800, chenyuhui (A) wrote:
>>> @Peter Xu @Fabiano Rosas
>>> Kindly ping on this.
>> 
>> Ah I see what's missing - please copy maintainer (Juan) for any migration
>> patches, especially multifd ones..  I'm doing that for this one, but I'd
>> suggest you repost with a whole patch and information put into commit msg.
>> 
>> Thanks.
>> 
> Hi, Juan
> This is a patch for migration，please take a look.
>
> From: Yuhui Chen <chenyuhui5@huawei.com>
> Date: Tue, 25 Jul 2023 10:45:48 +0800
> Subject: [PATCH] migrate/multifd: fix coredump when the multifd thread cleanup
>
> There is a coredump while trying to destroy mutex when
> p->running is false but p->mutex is not unlock.
> Make sure all mutexes has been released before destroy them.
>
> Signed-off-by: Yuhui Chen <chenyuhui5@huawei.com>
> ---
>  migration/multifd.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 3387d8277f..3a085bf3ec 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -521,9 +521,7 @@ void multifd_save_cleanup(void)
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>
> -        if (p->running) {
> -            qemu_thread_join(&p->thread);
> -        }
> +        qemu_thread_join(&p->thread);
>      }
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];

Hi,

I took another look at this and noticed that we're not even holding the
lock in the other threads when accessing p->running.

Also, the save_cleanup() function can be called before
qemu_thread_create(), which means p->thread.thread would have a bogus
value and qemu_thread_join() would abort the QEMU process.

What we need here is to stop setting p->running from inside the thread
altogether. The flag is only effectively protecting the
qemu_thread_join() from being called before the thread's creation. We
can post to sem_sync regardless of the thread state because we only
destroy it a few lines down in the same function. And there's already
p->quit which is being accessed properly and sends the thread out of the
loop.

So I suggest this pattern:

qemu_sem_post(&p->sem_sync);
if (p->running) {
    qemu_thread_join(&p->thread);
    p->running = false;
}

It would also be nice to move the p->running = true closer to the
qemu_thread_create() call at multifd_channel_connect().