migration: Fix race on qemu_file_shutdown()
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>
This commit is contained in:
parent
4934a5dd7c
commit
f5816b5c86
|
@ -79,6 +79,30 @@ int qemu_file_shutdown(QEMUFile *f)
|
||||||
int ret = 0;
|
int ret = 0;
|
||||||
|
|
||||||
f->shutdown = true;
|
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,
|
if (!qio_channel_has_feature(f->ioc,
|
||||||
QIO_CHANNEL_FEATURE_SHUTDOWN)) {
|
QIO_CHANNEL_FEATURE_SHUTDOWN)) {
|
||||||
return -ENOSYS;
|
return -ENOSYS;
|
||||||
|
@ -88,9 +112,6 @@ int qemu_file_shutdown(QEMUFile *f)
|
||||||
ret = -EIO;
|
ret = -EIO;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!f->last_error) {
|
|
||||||
qemu_file_set_error(f, -EIO);
|
|
||||||
}
|
|
||||||
return ret;
|
return ret;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue