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

Xiao Guangrong
2018-04-27T03:15:37Z

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. :)