[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 4/8] migration: Fix race on qemu_file_shutdown()
From: |
Juan Quintela |
Subject: |
[PULL 4/8] migration: Fix race on qemu_file_shutdown() |
Date: |
Mon, 21 Nov 2022 13:59:03 +0100 |
From: Peter Xu <peterx@redhat.com>
In qemu_file_shutdown(), there's a possible race if with current order of
operation. There're two major things to do:
(1) Do real shutdown() (e.g. shutdown() syscall on socket)
(2) Update qemufile's last_error
We must do (2) before (1) otherwise there can be a race condition like:
page receiver other thread
------------- ------------
qemu_get_buffer()
do shutdown()
returns 0 (buffer all zero)
(meanwhile we didn't check this retcode)
try to detect IO error
last_error==NULL, IO okay
install ALL-ZERO page
set last_error
--> guest crash!
To fix this, we can also check retval of qemu_get_buffer(), but not all
APIs can be properly checked and ultimately we still need to go back to
qemu_file_get_error(). E.g. qemu_get_byte() doesn't return error.
Maybe some day a rework of qemufile API is really needed, but for now keep
using qemu_file_get_error() and fix it by not allowing that race condition
to happen. Here shutdown() is indeed special because the last_error was
emulated. For real -EIO errors it'll always be set when e.g. sendmsg()
error triggers so we won't miss those ones, only shutdown() is a bit tricky
here.
Cc: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/qemu-file.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 4f400c2e52..2d5f74ffc2 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -79,6 +79,30 @@ int qemu_file_shutdown(QEMUFile *f)
int ret = 0;
f->shutdown = true;
+
+ /*
+ * We must set qemufile error before the real shutdown(), otherwise
+ * there can be a race window where we thought IO all went though
+ * (because last_error==NULL) but actually IO has already stopped.
+ *
+ * If without correct ordering, the race can happen like this:
+ *
+ * page receiver other thread
+ * ------------- ------------
+ * qemu_get_buffer()
+ * do shutdown()
+ * returns 0 (buffer all zero)
+ * (we didn't check this retcode)
+ * try to detect IO error
+ * last_error==NULL, IO okay
+ * install ALL-ZERO page
+ * set last_error
+ * --> guest crash!
+ */
+ if (!f->last_error) {
+ qemu_file_set_error(f, -EIO);
+ }
+
if (!qio_channel_has_feature(f->ioc,
QIO_CHANNEL_FEATURE_SHUTDOWN)) {
return -ENOSYS;
@@ -88,9 +112,6 @@ int qemu_file_shutdown(QEMUFile *f)
ret = -EIO;
}
- if (!f->last_error) {
- qemu_file_set_error(f, -EIO);
- }
return ret;
}
--
2.38.1
- [PULL 0/8] Next patches, Juan Quintela, 2022/11/21
- [PULL 1/8] migration/channel-block: fix return value for qio_channel_block_{readv, writev}, Juan Quintela, 2022/11/21
- [PULL 2/8] migration/multifd/zero-copy: Create helper function for flushing, Juan Quintela, 2022/11/21
- [PULL 3/8] migration: Fix possible infinite loop of ram save process, Juan Quintela, 2022/11/21
- [PULL 4/8] migration: Fix race on qemu_file_shutdown(),
Juan Quintela <=
- [PULL 5/8] migration: Disallow postcopy preempt to be used with compress, Juan Quintela, 2022/11/21
- [PULL 8/8] migration: Block migration comment or code is wrong, Juan Quintela, 2022/11/21
- [PULL 7/8] migration: Disable multifd explicitly with compression, Juan Quintela, 2022/11/21
- [PULL 6/8] migration: Use non-atomic ops for clear log bitmap, Juan Quintela, 2022/11/21
- Re: [PULL 0/8] Next patches, Stefan Hajnoczi, 2022/11/21