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

[Daniel P . Berrangé]

On Tue, Sep 20, 2022 at 06:37:57PM -0400, Peter Xu 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.

The ultimate answer really is to stop using QEMUFile entirely and just
do migration with the QIOChannel objects directly. The work I did in the
last cycle to remove the QEMUFileOps callbacks was another piece of the
puzzle in moving in that direction, by ensuring that every QEMUFile is
now backed by a QIOChannel. All QEMUFile is doing now is adding the I/O
caching layer and some convenience APIs for I/O operations.

So the next step would be to add a  QIOChannelCached class that can wrap
over another QIOChannel, to add the I/O buffering functionality that
currently exists in QEMUFile. Once that's done, it'd be at the stage
where we could look at how to use QIOChannel APIs for VMstate. It would
likely involve wiring up an Error **errp  parameter into a great many
places so we get synchronous error propagation instead of out-of-band
error checking, so a phased transition would need to be figured out.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|