* Peter Xu (pet...@redhat.com) wrote:
> 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>
> Signed-off-by: Peter Xu <pet...@redhat.com>
Oh that's kind of fun,
Reviewed-by: Dr. David Alan Gilbert <dgilb...@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.32.0
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
