From: Peter Xu <pet...@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 <berra...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Signed-off-by: Peter Xu <pet...@redhat.com>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Signed-off-by: Juan Quintela <quint...@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
