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

[Daniel P . Berrangé]

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.


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