Re: [PATCH 2/5] migration: Fix race on qemu_file_shutdown()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@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 <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Oh that's kind of fun,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@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 / dgilbert@redhat.com / Manchester, UK