Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Dec 06, 2018 at 05:42:54PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 06.12.2018 13:54, Daniel P. Berrangé wrote:
> > On Wed, Dec 05, 2018 at 05:47:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> The two thing that should be handled are cipher and ivgen. For ivgen
> >> the solution is just mutex, as iv calculations should not be long in
> >> comparison with encryption/decryption. And for cipher let's just keep
> >> per-thread ciphers.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> 
> [..]
> 
> >> --- a/crypto/block.c
> >> +++ b/crypto/block.c
> 
> [..]
> 
> >>   static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher,
> >>                                        size_t niv,
> >>                                        QCryptoIVGen *ivgen,
> >> +                                     QemuMutex *ivgen_mutex,
> >>                                        int sectorsize,
> >>                                        uint64_t offset,
> >>                                        uint8_t *buf,
> >> @@ -218,10 +307,15 @@ static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher,
> >>       while (len > 0) {
> >>           size_t nbytes;
> >>           if (niv) {
> >> -            if (qcrypto_ivgen_calculate(ivgen,
> >> -                                        startsector,
> >> -                                        iv, niv,
> >> -                                        errp) < 0) {
> >> +            if (ivgen_mutex) {
> >> +                qemu_mutex_lock(ivgen_mutex);
> >> +            }
> >> +            ret = qcrypto_ivgen_calculate(ivgen, startsector, iv, niv, errp);
> >> +            if (ivgen_mutex) {
> >> +                qemu_mutex_unlock(ivgen_mutex);
> >> +            }
> > 
> > I think we should just make  ivgen_mutex compulsory....
> > 
> >> +
> >> +            if (ret < 0) {
> >>                   goto cleanup;
> >>               }
> >>   
> >> @@ -258,8 +352,9 @@ int qcrypto_cipher_decrypt_helper(QCryptoCipher *cipher,
> >>                                     size_t len,
> >>                                     Error **errp)
> >>   {
> >> -    return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, sectorsize, offset,
> >> -                                     buf, len, qcrypto_cipher_decrypt, errp);
> >> +    return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, NULL, sectorsize,
> >> +                                     offset, buf, len, qcrypto_cipher_decrypt,
> >> +                                     errp);
> >>   }
> >>   
> >>   
> >> @@ -272,11 +367,11 @@ int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher,
> >>                                     size_t len,
> >>                                     Error **errp)
> >>   {
> >> -    return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, sectorsize, offset,
> >> -                                     buf, len, qcrypto_cipher_encrypt, errp);
> >> +    return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, NULL, sectorsize,
> >> +                                     offset, buf, len, qcrypto_cipher_encrypt,
> >> +                                     errp);
> >>   }
> > 
> > ...and get the mutex passed into these functions, as its easier to just
> > know the ivgen is always protected, and not have to trace back the callpath
> > to see if the usage is safe.
> 
> but there places, where these helpers called without any QCryptoBlock, when we just
> have local cipher and ivgen, so there is no mutex and it's not needed.

Ah, ok.


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