Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Zheng Chuan]

On 2020/8/21 20:39, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Fri, Aug 21, 2020 at 08:22:06PM +0800, Zheng Chuan wrote:
>>>
>>>
>>> On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
>>>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>>>> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
>>>>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>>>>>>> Record hash results for each sampled page.
>>>>>>>
>>>>>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>>>>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>>>>>>> ---
>>>>>>>  migration/dirtyrate.c | 144 
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  migration/dirtyrate.h |   7 +++
>>>>>>>  2 files changed, 151 insertions(+)
>>>>>>>
>>>>>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>>>>>> index c4304ef..62b6f69 100644
>>>>>>> --- a/migration/dirtyrate.c
>>>>>>> +++ b/migration/dirtyrate.c
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>  #include "dirtyrate.h"
>>>>>>>  
>>>>>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>>>>>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
>>>>>>
>>>>>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
>>>>>> It's never going to change is it?
>>>>>> (and anyway it's just a MD5 len?)
>>>>>
>>>>> I wouldn't want to bet on that given that this is use of MD5. We might
>>>>> claim this isn't security critical, but surprises happen, and we will
>>>>> certainly be dinged on security audits for introducing new use of MD5
>>>>> no matter what.
>>>>>
>>>>> If a cryptographic hash is required, then sha256 should be the choice
>>>>> for any new code that doesn't have back compat requirements.
>>>>>
>>>>> If a cryptographic hash is not required then how about crc32 
>>>>
>>>> It doesn't need to be cryptographic; is crc32 the fastest reasonable hash 
>>>> for use
>>>> in large areas?
>>>>
>>>> Dave
>>>>
>>>>> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
>>>>> hash, but then pick the most insecure one.
>>>>>
>>>>> sha256 is slower than md5, but it is conceivable that in future we might
>>>>> gain support for something like Blake2b which is similar security level
>>>>> to SHA3, while being faster than MD5.
>>>>>
>>>>> Overall I'm pretty unethusiastic about use of MD5 being introduced and
>>>>> worse, being hardcoded as the only option.
>>>>>
>>>>> 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 :|
>>>
>>> Hi, Daniel, Dave.
>>>
>>> I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.
>>>
>>> 1. Calculation speed
>>> 1） MD5 takes about 500ms to sample and hash all pages by 
>>> record_ramblock_hash_info().
>>> 2)  SHA256 takes about 750ms to sample all pages by 
>>> record_ramblock_hash_info().
>>>
>>> 2. CPU Consumption
>>> 1)  MD5 may have instant rise up to 48% for dirtyrate thread
>>> 2)  SHA256 may have instant rise up to 75% for dirtyrate thread
>>>
>>> 3. Memory Consumption
>>> SHA256 may need twice memory than MD5 due to its HASH_LEN.
>>>
>>> I am trying to consider if crc32 is more faster and takes less memory and 
>>> is more safer than MD5?
>>
>> No, crc32 is absolutely *weaker* than MD5. It is NOT a cryptographic
>> hash so does not try to guarantee collision resistance. It only has
>> 2^32 possible outputs.
>>
>> MD5 does try to guarantee collision resistance, but MD5 is considered
>> broken these days, so a malicious attacker can cause collisions if they
>> are motivated enough.
>>
>> IOW if you need collision resistance that SHA256 should be used.
> 
> There's no need to guard against malicious behaviour here - this is just
> a stat to guide migration.
> If CRC32 is likely to be faster than md5 I suspect it's enough.
> 
> Dave
> 
Hi，Dave, Daniel.

I did test by crc32，it is much faster than MD5 and SHA256:)

As for 128G vm it takes only about 50ms to sample and hash all pages by 
record_ramblock_hash_info().
And the dirtyrate calculation is still good enough:)
++++++++++++++++++++++++++++++++++++++++++
|                      |    dirtyrate    |
++++++++++++++++++++++++++++++++++++++++++
| no mempress          |     4MB/s       |
------------------------------------------
| mempress 4096 1024   |    1248MB/s     |
++++++++++++++++++++++++++++++++++++++++++
| mempress 4096 4096   |    4060MB/s     |
++++++++++++++++++++++++++++++++++++++++++

I will take crc32 in PatchV4, is that OK from the perspective of safety?

In my opinion, it should be safe.
The crc32 is only for compare and the recorder will be free after calculation 
is over.
The output is just dirtyrate for user to guide migration.

What's more, i consider increase DIRTYRATE_DEFAULT_SAMPLE_PAGES from 256 to 512
which may takes about 75ms to sample and hash all pages by 
record_ramblock_hash_info().

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