Re: [Qemu-devel] [PATCH] migration: introduce decompress-error-check

[Xiao Guangrong]

On 04/26/2018 10:01 PM, Eric Blake wrote:
> On 04/26/2018 04:15 AM, address@hidden wrote:
> > From: Xiao Guangrong <address@hidden>
> >
> > QEMU 2.13 enables strict check for compression & decompression to
> > make the migration more robuster, that depends on the source to fix
>
> s/robuster/robust/

Will fix, thank you for pointing it out.

> > the internal design which triggers the unexpected error conditions
>
> 2.13 hasn't been released yet.  Why do we need a knob to explicitly turn
> off strict checking?  Can we not instead make 2.13 automatically smart
> enough to tell if the incoming stream is coming from an older qemu
> (which might fail if the strict checks are enabled) vs. a newer qemu
> (the sender gave us what we need to ensure the strict checks are
> worthwhile)?

Really smart.

How about introduce a new command, MIG_CMD_DECOMPRESS_ERR_CHECK,
the destination will do strict check if got this command (i.e, new
QEMU is running on the source), otherwise, turn the check off.

> > To make it work for migrating old version QEMU to 2.13 QEMU, we
> > introduce this parameter to disable the error check on the
> > destination
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
>
> > +++ b/qapi/migration.json
> > @@ -455,6 +455,17 @@
> >   #          compression, so set the decompress-threads to the number about 1/4
> >   #          of compress-threads is adequate.
> >   #
> > +# @decompress-error-check: check decompression errors. When false, the errors
> > +#                          triggered by memory decompression are ignored.
>
> What are the consequences of such an error?  Is it a security hole to
> leave this at false, when a malicious migration stream can cause us to
> misbehave by ignoring the errors?

The issue fixed by strict error check is avoiding VM corruption if zlib failed
to compress & decompress the memory, i.e, catch error conditions returned by
zlib API.

> > +#                          When true, migration is aborted if the errors are
> > +#                          detected. For the old QEMU versions (< 2.13) the
> > +#                          internal design will cause decompression to fail
> > +#                          so the destination should completely ignore the
> > +#                          error conditions, i.e, make it be false if these
> > +#                          QEMUs are going to be migrated. Since 2.13, this
> > +#                          design is fixed, make it be true to avoid corrupting
> > +#                          the VM silently (Since 2.13)
>
> Rather wordy; I'd suggest:
>
> @decompress-error-check: Set to true to abort the migration if
>          decompression errors are detected at the destination. Should be
>          left at false (default) for qemu older than 2.13, since only
>          newer qemu sends streams that do not trigger spurious
>          decompression errors. (Since 2.13)

Yup, much better.

> But that's if we even need it (it SHOULD be possible to design something
> into the migration stream so that you can detect this property
> automatically instead of relying on the user to set the property).

Yes, can not agree with you more. :)