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

[Dr. David Alan Gilbert]

* 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

> 
> 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 :|
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK