[Qemu-devel] encrypt in threads

[Qemu-devel] encrypt in threads

[Vladimir Sementsov-Ogievskiy]

Hi Daniel!

After moving compression to threads in Qcow2 it's an obvious next step to
"threadyfy" encryption in Qcow2 too.

But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
in parallel threads with the same first argument (block), it just produce wrong
things (pattern verification fails in iotests)..

So, can you advise the way to parallelize encryption/decryption?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] encrypt in threads

[Vladimir Sementsov-Ogievskiy]

21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> Hi Daniel!
> 
> After moving compression to threads in Qcow2 it's an obvious next step to
> "threadyfy" encryption in Qcow2 too.
> 
> But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
> in parallel threads with the same first argument (block), it just produce wrong
> things (pattern verification fails in iotests)..
> 
> So, can you advise the way to parallelize encryption/decryption?
> 

Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing to do?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] encrypt in threads

[Daniel P. Berrangé]

On Thu, Nov 22, 2018 at 01:01:20PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> > Hi Daniel!
> > 
> > After moving compression to threads in Qcow2 it's an obvious next step to
> > "threadyfy" encryption in Qcow2 too.
> > 
> > But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
> > in parallel threads with the same first argument (block), it just produce wrong
> > things (pattern verification fails in iotests)..
> > 
> > So, can you advise the way to parallelize encryption/decryption?
> > 
> 
> Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing to do?

That's rather a heavy weight approach and would cause pain when we want
to support future options such as keyslot updates, and in the future,
LUKSv2 with master key changes.

Probably what's needed is change to QCryptoBlock struct so that there
can be multiple QCryptoCipher instances allocated - one per thread.

We might also need to introduce some locking around usage of the
QCryptoIVGen object, since that has a QCryptoCipher handle too.

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

Re: [Qemu-devel] encrypt in threads

[Vladimir Sementsov-Ogievskiy]

On 27.11.2018 16:08, Daniel P. Berrangé wrote:
> On Thu, Nov 22, 2018 at 01:01:20PM +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi Daniel!
>>>
>>> After moving compression to threads in Qcow2 it's an obvious next step to
>>> "threadyfy" encryption in Qcow2 too.
>>>
>>> But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
>>> in parallel threads with the same first argument (block), it just produce wrong
>>> things (pattern verification fails in iotests)..
>>>
>>> So, can you advise the way to parallelize encryption/decryption?
>>>
>> Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing to do?
> That's rather a heavy weight approach and would cause pain when we want
> to support future options such as keyslot updates, and in the future,
> LUKSv2 with master key changes.
>
> Probably what's needed is change to QCryptoBlock struct so that there
> can be multiple QCryptoCipher instances allocated - one per thread.
>
> We might also need to introduce some locking around usage of the
> QCryptoIVGen object, since that has a QCryptoCipher handle too.


Can we also create QCryptoIVGen per thread, as QCryptoCipher? Or it 
should be one? If one, why my implementation with QCryptoBlock per 
thread works (at least it passes iotests, hmm)


>
> Regards,
> Daniel

Re: [Qemu-devel] encrypt in threads

[Daniel P. Berrangé]

On Thu, Nov 29, 2018 at 06:28:44PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 
> On 27.11.2018 16:08, Daniel P. Berrangé wrote:
> > On Thu, Nov 22, 2018 at 01:01:20PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> >> 21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> >>> Hi Daniel!
> >>>
> >>> After moving compression to threads in Qcow2 it's an obvious next step to
> >>> "threadyfy" encryption in Qcow2 too.
> >>>
> >>> But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
> >>> in parallel threads with the same first argument (block), it just produce wrong
> >>> things (pattern verification fails in iotests)..
> >>>
> >>> So, can you advise the way to parallelize encryption/decryption?
> >>>
> >> Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing to do?
> > That's rather a heavy weight approach and would cause pain when we want
> > to support future options such as keyslot updates, and in the future,
> > LUKSv2 with master key changes.
> >
> > Probably what's needed is change to QCryptoBlock struct so that there
> > can be multiple QCryptoCipher instances allocated - one per thread.
> >
> > We might also need to introduce some locking around usage of the
> > QCryptoIVGen object, since that has a QCryptoCipher handle too.
> 
> 
> Can we also create QCryptoIVGen per thread, as QCryptoCipher? Or it 
> should be one? If one, why my implementation with QCryptoBlock per 
> thread works (at least it passes iotests, hmm)

The only IV generator that uses ciphers is the "essiv" one. Even that
one uses the cipher in ECB mode, so there is no initialization vector
required for its internal cipher. This means there's no critical
shared state that would be overwritten by threads. Thus using a
separate QCryptoCipher per thread for the essiv IV gen is overkill.
None the less I think we should protect the IV generator calls with
a mutex just to be safe. I don't think the mutex would have a notable
impact on performance of the iv generator.

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

Re: [Qemu-devel] encrypt in threads

[Vladimir Sementsov-Ogievskiy]

30.11.2018 12:48, Daniel P. Berrangé wrote:
> On Thu, Nov 29, 2018 at 06:28:44PM +0000, Vladimir Sementsov-Ogievskiy wrote:
>>
>> On 27.11.2018 16:08, Daniel P. Berrangé wrote:
>>> On Thu, Nov 22, 2018 at 01:01:20PM +0000, Vladimir Sementsov-Ogievskiy wrote:
>>>> 21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi Daniel!
>>>>>
>>>>> After moving compression to threads in Qcow2 it's an obvious next step to
>>>>> "threadyfy" encryption in Qcow2 too.
>>>>>
>>>>> But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
>>>>> in parallel threads with the same first argument (block), it just produce wrong
>>>>> things (pattern verification fails in iotests)..
>>>>>
>>>>> So, can you advise the way to parallelize encryption/decryption?
>>>>>
>>>> Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing to do?
>>> That's rather a heavy weight approach and would cause pain when we want
>>> to support future options such as keyslot updates, and in the future,
>>> LUKSv2 with master key changes.
>>>
>>> Probably what's needed is change to QCryptoBlock struct so that there
>>> can be multiple QCryptoCipher instances allocated - one per thread.
>>>
>>> We might also need to introduce some locking around usage of the
>>> QCryptoIVGen object, since that has a QCryptoCipher handle too.
>>
>>
>> Can we also create QCryptoIVGen per thread, as QCryptoCipher? Or it
>> should be one? If one, why my implementation with QCryptoBlock per
>> thread works (at least it passes iotests, hmm)
> 
> The only IV generator that uses ciphers is the "essiv" one. Even that
> one uses the cipher in ECB mode, so there is no initialization vector
> required for its internal cipher. This means there's no critical
> shared state that would be overwritten by threads. Thus using a
> separate QCryptoCipher per thread for the essiv IV gen is overkill.
> None the less I think we should protect the IV generator calls with
> a mutex just to be safe. I don't think the mutex would have a notable
> impact on performance of the iv generator.

But if we use mutex, it means that we can't generate two ivs in parallel,
and then, it becomes better to have separate iv-gen per thread, instead
of only one, protected by mutex. (or I don't follow)

> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] encrypt in threads

[Daniel P. Berrangé]

On Fri, Nov 30, 2018 at 10:04:57AM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 30.11.2018 12:48, Daniel P. Berrangé wrote:
> > On Thu, Nov 29, 2018 at 06:28:44PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> >>
> >> On 27.11.2018 16:08, Daniel P. Berrangé wrote:
> >>> On Thu, Nov 22, 2018 at 01:01:20PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> >>>> 21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> >>>>> Hi Daniel!
> >>>>>
> >>>>> After moving compression to threads in Qcow2 it's an obvious next step to
> >>>>> "threadyfy" encryption in Qcow2 too.
> >>>>>
> >>>>> But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
> >>>>> in parallel threads with the same first argument (block), it just produce wrong
> >>>>> things (pattern verification fails in iotests)..
> >>>>>
> >>>>> So, can you advise the way to parallelize encryption/decryption?
> >>>>>
> >>>> Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing to do?
> >>> That's rather a heavy weight approach and would cause pain when we want
> >>> to support future options such as keyslot updates, and in the future,
> >>> LUKSv2 with master key changes.
> >>>
> >>> Probably what's needed is change to QCryptoBlock struct so that there
> >>> can be multiple QCryptoCipher instances allocated - one per thread.
> >>>
> >>> We might also need to introduce some locking around usage of the
> >>> QCryptoIVGen object, since that has a QCryptoCipher handle too.
> >>
> >>
> >> Can we also create QCryptoIVGen per thread, as QCryptoCipher? Or it
> >> should be one? If one, why my implementation with QCryptoBlock per
> >> thread works (at least it passes iotests, hmm)
> > 
> > The only IV generator that uses ciphers is the "essiv" one. Even that
> > one uses the cipher in ECB mode, so there is no initialization vector
> > required for its internal cipher. This means there's no critical
> > shared state that would be overwritten by threads. Thus using a
> > separate QCryptoCipher per thread for the essiv IV gen is overkill.
> > None the less I think we should protect the IV generator calls with
> > a mutex just to be safe. I don't think the mutex would have a notable
> > impact on performance of the iv generator.
> 
> But if we use mutex, it means that we can't generate two ivs in parallel,
> and then, it becomes better to have separate iv-gen per thread, instead
> of only one, protected by mutex. (or I don't follow)

I'm questioning whether parallelism of the iv-gen is actually needed
though. It has very short execution time, only encrypting 64 bits of
data, per 512 byte sector with essiv.

If you have show performance numbers that prove a per-thread iv generator
is needed for ESSIV I'd accept that, otherwise I'd just go for a mutex.

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

Re: [Qemu-devel] encrypt in threads

[Daniel P. Berrangé]

On Wed, Nov 21, 2018 at 05:30:53PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> Hi Daniel!
> 
> After moving compression to threads in Qcow2 it's an obvious next step to
> "threadyfy" encryption in Qcow2 too.
> 
> But it turned out to be not as simple as I assumed. If I call qcrypto_block_encrypt
> in parallel threads with the same first argument (block), it just produce wrong
> things (pattern verification fails in iotests)..

Yes, this makes sense, because the underlying crypto impls all require
that their state is only used from a single thread at any time. What's
likely happening is that the IV is being scrambled so we're encrypting
with the wrong tweak.

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