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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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.

-- 
Best regards,
Vladimir