Skip to content
Snippets Groups Projects
Select Git revision
  • feature/libtcg-rebase default protected
1 result
Search by author
  • Any Author
  • Anton Anton anjo
1 author
  1. Sep 29, 2023
  3. Sep 27, 2023
    • Fabiano Rosas's avatar
      migration: Move return path cleanup to main migration thread · 36e9aab3
      Fabiano Rosas authored 
      
Now that the return path thread is allowed to finish during a paused
migration, we can move the cleanup of the QEMUFiles to the main
migration thread.

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-9-farosas@suse.de>
      36e9aab3
    • Fabiano Rosas's avatar
      migration: Replace the return path retry logic · ef796ee9
      Fabiano Rosas authored 
      
Replace the return path retry logic with finishing and restarting the
thread. This fixes a race when resuming the migration that leads to a
segfault.

Currently when doing postcopy we consider that an IO error on the
return path file could be due to a network intermittency. We then keep
the thread alive but have it do cleanup of the 'from_dst_file' and
wait on the 'postcopy_pause_rp' semaphore. When the user issues a
migrate resume, a new return path is opened and the thread is allowed
to continue.

There's a race condition in the above mechanism. It is possible for
the new return path file to be setup *before* the cleanup code in the
return path thread has had a chance to run, leading to the *new* file
being closed and the pointer set to NULL. When the thread is released
after the resume, it tries to dereference 'from_dst_file' and crashes:

Thread 7 "return path" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffd1dbf700 (LWP 9611)]
0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
154         return f->last_error;

(gdb) bt
 #0  0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
 #1  0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206
 #2  0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876
 #3  0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541
 #4  0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477
 #5  0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Here's the race (important bit is open_return_path happening before
migration_release_dst_files):

migration                 | qmp                         | return path
--------------------------+-----------------------------+---------------------------------
			    qmp_migrate_pause()
			     shutdown(ms->to_dst_file)
			      f->last_error = -EIO
migrate_detect_error()
 postcopy_pause()
  set_state(PAUSED)
  wait(postcopy_pause_sem)
			    qmp_migrate(resume)
			    migrate_fd_connect()
			     resume = state == PAUSED
			     open_return_path <-- TOO SOON!
			     set_state(RECOVER)
			     post(postcopy_pause_sem)
							(incoming closes to_src_file)
							res = qemu_file_get_error(rp)
							migration_release_dst_files()
							ms->rp_state.from_dst_file = NULL
  post(postcopy_pause_rp_sem)
							postcopy_pause_return_path_thread()
							  wait(postcopy_pause_rp_sem)
							rp = ms->rp_state.from_dst_file
							goto retry
							qemu_file_get_error(rp)
							SIGSEGV
-------------------------------------------------------------------------------------------

We can keep the retry logic without having the thread alive and
waiting. The only piece of data used by it is the 'from_dst_file' and
it is only allowed to proceed after a migrate resume is issued and the
semaphore released at migrate_fd_connect().

Move the retry logic to outside the thread by waiting for the thread
to finish before pausing the migration.

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-8-farosas@suse.de>
      ef796ee9
    • Fabiano Rosas's avatar
      migration: Consolidate return path closing code · d50f5dc0
      Fabiano Rosas authored 
      
We'll start calling the await_return_path_close_on_source() function
from other parts of the code, so move all of the related checks and
tracepoints into it.

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-7-farosas@suse.de>
      d50f5dc0
    • Fabiano Rosas's avatar
      migration: Remove redundant cleanup of postcopy_qemufile_src · b3b10115
      Fabiano Rosas authored 
      
This file is owned by the return path thread which is already doing
cleanup.

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-6-farosas@suse.de>
      b3b10115
    • Fabiano Rosas's avatar
      migration: Fix possible race when shutting down to_dst_file · 7478fb0d
      Fabiano Rosas authored 
      
It's not safe to call qemu_file_shutdown() on the to_dst_file without
first checking for the file's presence under the lock. The cleanup of
this file happens at postcopy_pause() and migrate_fd_cleanup() which
are not necessarily running in the same thread as migrate_fd_cancel().

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-5-farosas@suse.de>
      7478fb0d
    • Fabiano Rosas's avatar
      migration: Fix possible races when shutting down the return path · 639decf5
      Fabiano Rosas authored 
      
We cannot call qemu_file_shutdown() on the return path file without
taking the file lock. The return path thread could be running it's
cleanup code and have just cleared the from_dst_file pointer.

Checking ms->to_dst_file for errors could also race with
migrate_fd_cleanup() which clears the to_dst_file pointer.

Protect both accesses by taking the file lock.

This was caught by inspection, it should be rare, but the next patches
will start calling this code from other places, so let's do the
correct thing.

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-4-farosas@suse.de>
      639decf5
    • Fabiano Rosas's avatar
      migration: Fix possible race when setting rp_state.error · 28a83472
      Fabiano Rosas authored 
      
We don't need to set the rp_state.error right after a shutdown because
qemu_file_shutdown() always sets the QEMUFile error, so the return
path thread would have seen it and set the rp error itself.

Setting the error outside of the thread is also racy because the
thread could clear it after we set it.

Reviewed-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-3-farosas@suse.de>
      28a83472
    • Peter Xu's avatar
      migration: Fix race that dest preempt thread close too early · cf02f29e
      Peter Xu authored 
      We hit intermit CI issue on failing at migration-test over the unit test
preempt/plain:

qemu-system-x86_64: Unable to read from socket: Connection reset by peer
Memory content inconsistency at 5b43000 first_byte = bd last_byte = bc current = 4f hit_edge = 1
**
ERROR:../tests/qtest/migration-test.c:300:check_guests_ram: assertion failed: (bad == 0)
(test program exited with status code -6)

Fabiano debugged into it and found that the preempt thread can quit even
without receiving all the pages, which can cause guest not receiving all
the pages and corrupt the guest memory.

To make sure preempt thread finished receiving all the pages, we can rely
on the page_requested_count being zero because preempt channel will only
receive requested page faults. Note, not all the faulted pages are required
to be sent via the preempt channel/thread; imagine the case when a
requested page is just queued into the background main channel for
migration, the src qemu will just still send it via the background channel.

Here instead of spinning over reading the count, we add a condvar so the
main thread can wait on it if that unusual case happened, without burning
the cpu for no good reason, even if the duration is short; so even if we
spin in this rare case is probably fine.  It's just better to not do so.

The condvar is only used when that special case is triggered.  Some memory
ordering trick is needed to guarantee it from happening (against the
preempt thread status field), so the main thread will always get a kick
when that triggers correctly.

Closes: https://gitlab.com/qemu-project/qemu/-/issues/1886


Debugged-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarPeter Xu <peterx@redhat.com>
Signed-off-by: default avatarFabiano Rosas <farosas@suse.de>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230918172822.19052-2-farosas@suse.de>
      cf02f29e
    • Stefan Hajnoczi's avatar
      Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging · 5dfd80e3
      Stefan Hajnoczi authored 
      * new round of audio cleanups
* various shadowed local variable fixes in vl, mptsas, pm_smbus, target/i386
* remove deprecated pc-i440fx-1.4 up to pc-i440fx-1.7
* remove PCI drivers from 128K bios.bin
* remove unused variable in user-exec-stub.c
* small fixes for ui/vnc
* scsi-disk: Disallow block sizes smaller than 512 [CVE-2023-42467]

# -----BEGIN PGP SIGNATURE-----
#
# iQFIBAABCAAyFiEE8TM4V0tmI4mGbHaCv/vSX3jHroMFAmUTDaoUHHBib256aW5p
# QHJlZGhhdC5jb20ACgkQv/vSX3jHroMvEgf+NrSaP4pmHrYcVtm43fnKXoLHFrCx
# KYfoK9Lke/DDkTff6rrcfW/Wyqid6Pp9Ch4Rrpr/X71X5gi+c6xb5klC8cpSfLg4
# gtuGctj7WL7KR/067EsLqHvzBob/iebFhZwhtsBrI+z65X+J9pOK78efBTdhezq4
# EEHTWohMAg1I/MWBK5VnOk2fI4+9z9K9zP5AtWmJzwwJkQUoEyl+YDkVmIhMYoGn
# CapRO7i2wIvtoF4wuQUCGsOLmrcWTvRIOcV13k3b6PYCPC40/N9AOpiiyg3XqNah
# UKKM9CcgVnCzCc4Jar2QD+MzkTDxhmQSyLFJgtzrW7CQSE5YB3sUHj3CXg==
# =8nvs
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue 26 Sep 2023 12:58:18 EDT
# gpg:                using RSA key F13338574B662389866C7682BFFBD25F78C7AE83
# gpg:                issuer "pbonzini@redhat.com"
# gpg: Good signature from "Paolo Bonzini <bonzini@gnu.org>" [full]
# gpg:                 aka "Paolo Bonzini <pbonzini@redhat.com>" [full]
# Primary key fingerprint: 46F5 9FBD 57D6 12E7 BFD4  E2F7 7E15 100C CD36 69B1
#      Subkey fingerprint: F133 3857 4B66 2389 866C  7682 BFFB D25F 78C7 AE83

* tag 'for-upstream' of https://gitlab.com/bonzini/qemu

:
  audio: remove shadowed locals
  compiler: introduce QEMU_ANNOTATE
  block: mark mixed functions that can suspend
  target/i386/svm_helper: eliminate duplicate local variable
  target/i386/seg_helper: remove shadowed variable
  target/i386/seg_helper: introduce tss_set_busy
  target/i386/translate: avoid shadowed local variables
  target/i386/cpu: avoid shadowed local variables
  target/i386/kvm: eliminate shadowed local variables
  m48t59-test: avoid possible overflow on ABS
  pm_smbus: rename variable to avoid shadowing
  mptsas: avoid shadowed local variables
  ui/vnc: fix handling of VNC_FEATURE_XVP
  ui/vnc: fix debug output for invalid audio message
  vl: remove shadowed local variables
  hw/scsi/scsi-disk: Disallow block sizes smaller than 512 [CVE-2023-42467]
  user-exec-stub: remove unused variable
  seabios: remove PCI drivers from bios.bin
  pc_piix: remove pc-i440fx-1.4 up to pc-i440fx-1.7

Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
      5dfd80e3
  5. Sep 26, 2023