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

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 04/26/2018 10:01 PM, Eric Blake wrote:
> On 04/26/2018 04:15 AM, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> 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 <xiaoguangrong@tencent.com>
>> ---
> 
>> +++ 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. :)