* [Qemu-devel] [PATCH v3 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
2018-12-07 16:13 [Qemu-devel] [PATCH v3 0/5] crypto threads Vladimir Sementsov-Ogievskiy
@ 2018-12-07 16:13 ` Vladimir Sementsov-Ogievskiy
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-07 16:13 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, berto, vsementsov, den
Free block->cipher and block->ivgen on error path.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
crypto/block-luks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 5738124773..51e24d23ca 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1341,6 +1341,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
qcrypto_ivgen_free(ivgen);
qcrypto_cipher_free(cipher);
+ qcrypto_cipher_free(block->cipher);
+ qcrypto_ivgen_free(block->ivgen);
+
g_free(luks);
return -1;
}
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v3 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
2018-12-07 16:13 [Qemu-devel] [PATCH v3 0/5] crypto threads Vladimir Sementsov-Ogievskiy
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
@ 2018-12-07 16:13 ` Vladimir Sementsov-Ogievskiy
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-07 16:13 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, berto, vsementsov, den
qcrypto_block_encrypt_helper and qcrypto_block_decrypt_helper are
almost identical, let's reduce code duplication and simplify further
improvements.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
crypto/block.c | 81 +++++++++++++++++++-------------------------------
1 file changed, 31 insertions(+), 50 deletions(-)
diff --git a/crypto/block.c b/crypto/block.c
index e59d1140fe..8d0e4bdbb2 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -190,14 +190,21 @@ void qcrypto_block_free(QCryptoBlock *block)
}
-int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp)
+typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
+ const void *in,
+ void *out,
+ size_t len,
+ Error **errp);
+
+static int do_qcrypto_block_encdec(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ QCryptoCipherEncDecFunc func,
+ Error **errp)
{
uint8_t *iv;
int ret = -1;
@@ -226,8 +233,7 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
}
nbytes = len > sectorsize ? sectorsize : len;
- if (qcrypto_cipher_decrypt(cipher, buf, buf,
- nbytes, errp) < 0) {
+ if (func(cipher, buf, buf, nbytes, errp) < 0) {
goto cleanup;
}
@@ -243,7 +249,7 @@ int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
}
-int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
+int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
size_t niv,
QCryptoIVGen *ivgen,
int sectorsize,
@@ -252,45 +258,20 @@ int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
size_t len,
Error **errp)
{
- uint8_t *iv;
- int ret = -1;
- uint64_t startsector = offset / sectorsize;
-
- assert(QEMU_IS_ALIGNED(offset, sectorsize));
- assert(QEMU_IS_ALIGNED(len, sectorsize));
-
- iv = niv ? g_new0(uint8_t, niv) : NULL;
-
- while (len > 0) {
- size_t nbytes;
- if (niv) {
- if (qcrypto_ivgen_calculate(ivgen,
- startsector,
- iv, niv,
- errp) < 0) {
- goto cleanup;
- }
+ return do_qcrypto_block_encdec(cipher, niv, ivgen, sectorsize, offset,
+ buf, len, qcrypto_cipher_decrypt, errp);
+}
- if (qcrypto_cipher_setiv(cipher,
- iv, niv,
- errp) < 0) {
- goto cleanup;
- }
- }
- nbytes = len > sectorsize ? sectorsize : len;
- if (qcrypto_cipher_encrypt(cipher, buf, buf,
- nbytes, errp) < 0) {
- goto cleanup;
- }
-
- startsector++;
- buf += nbytes;
- len -= nbytes;
- }
-
- ret = 0;
- cleanup:
- g_free(iv);
- return ret;
+int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
+{
+ return do_qcrypto_block_encdec(cipher, niv, ivgen, sectorsize, offset,
+ buf, len, qcrypto_cipher_encrypt, errp);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
2018-12-07 16:13 [Qemu-devel] [PATCH v3 0/5] crypto threads Vladimir Sementsov-Ogievskiy
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
@ 2018-12-07 16:13 ` Vladimir Sementsov-Ogievskiy
2018-12-10 10:44 ` Alberto Garcia
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-07 16:13 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, berto, vsementsov, den
Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as
it's not about QCryptoBlock. This is needed to introduce
qcrypto_block_*crypt_helper in the next commit, which will have
QCryptoBlock pointer and than will be able to use additional fields of
it, which in turn will be used to implement thread-safe QCryptoBlock
operations.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
crypto/blockpriv.h | 34 +++++++++++-----------
crypto/block-luks.c | 44 ++++++++++++++--------------
crypto/block-qcow.c | 16 +++++------
crypto/block.c | 70 +++++++++++++++++++++++----------------------
4 files changed, 83 insertions(+), 81 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 41840abcec..54bbe1eb6a 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -78,22 +78,22 @@ struct QCryptoBlockDriver {
};
-int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp);
-
-int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp);
+int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
+
+int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
#endif /* QCRYPTO_BLOCKPRIV_H */
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 51e24d23ca..353a400595 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -504,14 +504,14 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* to reset the encryption cipher every time the master
* key crosses a sector boundary.
*/
- if (qcrypto_block_decrypt_helper(cipher,
- niv,
- ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- 0,
- splitkey,
- splitkeylen,
- errp) < 0) {
+ if (qcrypto_block_cipher_decrypt_helper(cipher,
+ niv,
+ ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ 0,
+ splitkey,
+ splitkeylen,
+ errp) < 0) {
goto cleanup;
}
@@ -1219,12 +1219,12 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Now we encrypt the split master key with the key generated
* from the user's password, before storing it */
- if (qcrypto_block_encrypt_helper(cipher, block->niv, ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- 0,
- splitkey,
- splitkeylen,
- errp) < 0) {
+ if (qcrypto_block_cipher_encrypt_helper(cipher, block->niv, ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ 0,
+ splitkey,
+ splitkeylen,
+ errp) < 0) {
goto error;
}
@@ -1409,10 +1409,10 @@ qcrypto_block_luks_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
- return qcrypto_block_decrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_cipher_decrypt_helper(block->cipher,
+ block->niv, block->ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ offset, buf, len, errp);
}
@@ -1425,10 +1425,10 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
- return qcrypto_block_encrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_cipher_encrypt_helper(block->cipher,
+ block->niv, block->ivgen,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ offset, buf, len, errp);
}
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 7606231e79..3b6722deb0 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -152,10 +152,10 @@ qcrypto_block_qcow_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
- return qcrypto_block_decrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_cipher_decrypt_helper(block->cipher,
+ block->niv, block->ivgen,
+ QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
+ offset, buf, len, errp);
}
@@ -168,10 +168,10 @@ qcrypto_block_qcow_encrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
- return qcrypto_block_encrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_cipher_encrypt_helper(block->cipher,
+ block->niv, block->ivgen,
+ QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
+ offset, buf, len, errp);
}
diff --git a/crypto/block.c b/crypto/block.c
index 8d0e4bdbb2..d43139dbc7 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -191,20 +191,20 @@ void qcrypto_block_free(QCryptoBlock *block)
typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
- const void *in,
- void *out,
- size_t len,
- Error **errp);
-
-static int do_qcrypto_block_encdec(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- QCryptoCipherEncDecFunc func,
- Error **errp)
+ const void *in,
+ void *out,
+ size_t len,
+ Error **errp);
+
+static int do_qcrypto_block_cipher_encdec(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ QCryptoCipherEncDecFunc func,
+ Error **errp)
{
uint8_t *iv;
int ret = -1;
@@ -249,29 +249,31 @@ static int do_qcrypto_block_encdec(QCryptoCipher *cipher,
}
-int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp)
+int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
{
- return do_qcrypto_block_encdec(cipher, niv, ivgen, sectorsize, offset,
- buf, len, qcrypto_cipher_decrypt, errp);
+ return do_qcrypto_block_cipher_encdec(cipher, niv, ivgen, sectorsize,
+ offset, buf, len,
+ qcrypto_cipher_decrypt, errp);
}
-int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- Error **errp)
+int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
{
- return do_qcrypto_block_encdec(cipher, niv, ivgen, sectorsize, offset,
- buf, len, qcrypto_cipher_encrypt, errp);
+ return do_qcrypto_block_cipher_encdec(cipher, niv, ivgen, sectorsize,
+ offset, buf, len,
+ qcrypto_cipher_encrypt, errp);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
@ 2018-12-10 10:44 ` Alberto Garcia
2018-12-10 13:21 ` Daniel P. Berrangé
0 siblings, 1 reply; 16+ messages in thread
From: Alberto Garcia @ 2018-12-10 10:44 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: berrange, mreitz, kwolf, den
On Fri 07 Dec 2018 05:13:49 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as
You forgot to update the new function names in the commit message.
> -int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
> -int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
> +int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher,
> +int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
Berto
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
2018-12-10 10:44 ` Alberto Garcia
@ 2018-12-10 13:21 ` Daniel P. Berrangé
2018-12-10 13:24 ` Alberto Garcia
0 siblings, 1 reply; 16+ messages in thread
From: Daniel P. Berrangé @ 2018-12-10 13:21 UTC (permalink / raw)
To: Alberto Garcia
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, mreitz,
kwolf, den
On Mon, Dec 10, 2018 at 11:44:22AM +0100, Alberto Garcia wrote:
> On Fri 07 Dec 2018 05:13:49 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> > Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as
>
> You forgot to update the new function names in the commit message.
>
> > -int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
> > -int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
> > +int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher,
> > +int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
I'll take care of that when queuing these patches
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 :|
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
2018-12-10 13:21 ` Daniel P. Berrangé
@ 2018-12-10 13:24 ` Alberto Garcia
0 siblings, 0 replies; 16+ messages in thread
From: Alberto Garcia @ 2018-12-10 13:24 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, mreitz,
kwolf, den
On Mon 10 Dec 2018 02:21:56 PM CET, Daniel P. Berrangé wrote:
> On Mon, Dec 10, 2018 at 11:44:22AM +0100, Alberto Garcia wrote:
>> On Fri 07 Dec 2018 05:13:49 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> > Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as
>>
>> You forgot to update the new function names in the commit message.
>>
>> > -int qcrypto_block_decrypt_helper(QCryptoCipher *cipher,
>> > -int qcrypto_block_encrypt_helper(QCryptoCipher *cipher,
>> > +int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher,
>> > +int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
>
> I'll take care of that when queuing these patches
Ok then
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v3 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions
2018-12-07 16:13 [Qemu-devel] [PATCH v3 0/5] crypto threads Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
@ 2018-12-07 16:13 ` Vladimir Sementsov-Ogievskiy
2018-12-10 10:45 ` Alberto Garcia
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
2018-12-10 15:19 ` [Qemu-devel] [PATCH v3 0/5] crypto threads Daniel P. Berrangé
5 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-07 16:13 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, berto, vsementsov, den
Introduce QCryptoBlock-based functions and use them where possible.
This is needed to implement thread-safe encrypt/decrypt operations.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/blockpriv.h | 14 ++++++++++++++
crypto/block-luks.c | 14 ++++++--------
crypto/block-qcow.c | 14 ++++++--------
crypto/block.c | 28 ++++++++++++++++++++++++++++
4 files changed, 54 insertions(+), 16 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 54bbe1eb6a..438c08bec2 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -96,4 +96,18 @@ int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
size_t len,
Error **errp);
+int qcrypto_block_decrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
+
+int qcrypto_block_encrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp);
+
#endif /* QCRYPTO_BLOCKPRIV_H */
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 353a400595..e486e7ee94 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1409,10 +1409,9 @@ qcrypto_block_luks_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
- return qcrypto_block_cipher_decrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_decrypt_helper(block,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ offset, buf, len, errp);
}
@@ -1425,10 +1424,9 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
- return qcrypto_block_cipher_encrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_encrypt_helper(block,
+ QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
+ offset, buf, len, errp);
}
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 3b6722deb0..36bf5f09b7 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -152,10 +152,9 @@ qcrypto_block_qcow_decrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
- return qcrypto_block_cipher_decrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_decrypt_helper(block,
+ QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
+ offset, buf, len, errp);
}
@@ -168,10 +167,9 @@ qcrypto_block_qcow_encrypt(QCryptoBlock *block,
{
assert(QEMU_IS_ALIGNED(offset, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
assert(QEMU_IS_ALIGNED(len, QCRYPTO_BLOCK_QCOW_SECTOR_SIZE));
- return qcrypto_block_cipher_encrypt_helper(block->cipher,
- block->niv, block->ivgen,
- QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
- offset, buf, len, errp);
+ return qcrypto_block_encrypt_helper(block,
+ QCRYPTO_BLOCK_QCOW_SECTOR_SIZE,
+ offset, buf, len, errp);
}
diff --git a/crypto/block.c b/crypto/block.c
index d43139dbc7..3fe3de2ef8 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -277,3 +277,31 @@ int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
offset, buf, len,
qcrypto_cipher_encrypt, errp);
}
+
+
+int qcrypto_block_decrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
+{
+ return do_qcrypto_block_cipher_encdec(block->cipher, block->niv,
+ block->ivgen,
+ sectorsize, offset, buf, len,
+ qcrypto_cipher_decrypt, errp);
+}
+
+
+int qcrypto_block_encrypt_helper(QCryptoBlock *block,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ Error **errp)
+{
+ return do_qcrypto_block_cipher_encdec(block->cipher, block->niv,
+ block->ivgen,
+ sectorsize, offset, buf, len,
+ qcrypto_cipher_encrypt, errp);
+}
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-07 16:13 [Qemu-devel] [PATCH v3 0/5] crypto threads Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
@ 2018-12-07 16:13 ` Vladimir Sementsov-Ogievskiy
2018-12-10 14:06 ` Alberto Garcia
2018-12-10 15:19 ` [Qemu-devel] [PATCH v3 0/5] crypto threads Daniel P. Berrangé
5 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-07 16:13 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, berto, vsementsov, den
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>
---
crypto/blockpriv.h | 16 ++++-
include/crypto/block.h | 2 +
block/crypto.c | 1 +
block/qcow.c | 2 +-
block/qcow2.c | 4 +-
crypto/block-luks.c | 22 +++---
crypto/block-qcow.c | 20 +++---
crypto/block.c | 146 +++++++++++++++++++++++++++++++++-----
tests/test-crypto-block.c | 3 +
9 files changed, 172 insertions(+), 44 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 438c08bec2..5438e822fd 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -22,6 +22,7 @@
#define QCRYPTO_BLOCKPRIV_H
#include "crypto/block.h"
+#include "qemu/thread.h"
typedef struct QCryptoBlockDriver QCryptoBlockDriver;
@@ -31,8 +32,12 @@ struct QCryptoBlock {
const QCryptoBlockDriver *driver;
void *opaque;
- QCryptoCipher *cipher;
+ QCryptoCipher **ciphers;
+ size_t n_ciphers;
+ size_t n_free_ciphers;
QCryptoIVGen *ivgen;
+ QemuMutex mutex;
+
QCryptoHashAlgorithm kdfhash;
size_t niv;
uint64_t payload_offset; /* In bytes */
@@ -46,6 +51,7 @@ struct QCryptoBlockDriver {
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ size_t n_threads,
Error **errp);
int (*create)(QCryptoBlock *block,
@@ -110,4 +116,12 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
size_t len,
Error **errp);
+int qcrypto_block_init_cipher(QCryptoBlock *block,
+ QCryptoCipherAlgorithm alg,
+ QCryptoCipherMode mode,
+ const uint8_t *key, size_t nkey,
+ size_t n_threads, Error **errp);
+
+void qcrypto_block_free_cipher(QCryptoBlock *block);
+
#endif /* QCRYPTO_BLOCKPRIV_H */
diff --git a/include/crypto/block.h b/include/crypto/block.h
index cd18f46d56..e729d5bd66 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -75,6 +75,7 @@ typedef enum {
* @readfunc: callback for reading data from the volume
* @opaque: data to pass to @readfunc
* @flags: bitmask of QCryptoBlockOpenFlags values
+ * @n_threads: allow concurrent I/O from up to @n_threads threads
* @errp: pointer to a NULL-initialized error object
*
* Create a new block encryption object for an existing
@@ -107,6 +108,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ size_t n_threads,
Error **errp);
/**
diff --git a/block/crypto.c b/block/crypto.c
index 33ee01bebd..f0a5f6b987 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -229,6 +229,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
block_crypto_read_func,
bs,
cflags,
+ 1,
errp);
if (!crypto->block) {
diff --git a/block/qcow.c b/block/qcow.c
index 4518cb4c35..0a235bf393 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -213,7 +213,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
}
s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
- NULL, NULL, cflags, errp);
+ NULL, NULL, cflags, 1, errp);
if (!s->crypto) {
ret = -EINVAL;
goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 991d6ac91b..bc8868c36a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -294,7 +294,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
}
s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
qcow2_crypto_hdr_read_func,
- bs, cflags, errp);
+ bs, cflags, 1, errp);
if (!s->crypto) {
return -EINVAL;
}
@@ -1445,7 +1445,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
}
s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
- NULL, NULL, cflags, errp);
+ NULL, NULL, cflags, 1, errp);
if (!s->crypto) {
ret = -EINVAL;
goto fail;
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index e486e7ee94..6bac79c3ab 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -636,6 +636,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ size_t n_threads,
Error **errp)
{
QCryptoBlockLUKS *luks;
@@ -836,11 +837,10 @@ qcrypto_block_luks_open(QCryptoBlock *block,
goto fail;
}
- block->cipher = qcrypto_cipher_new(cipheralg,
- ciphermode,
- masterkey, masterkeylen,
- errp);
- if (!block->cipher) {
+ ret = qcrypto_block_init_cipher(block, cipheralg, ciphermode,
+ masterkey, masterkeylen, n_threads,
+ errp);
+ if (ret < 0) {
ret = -ENOTSUP;
goto fail;
}
@@ -863,7 +863,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
fail:
g_free(masterkey);
- qcrypto_cipher_free(block->cipher);
+ qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
g_free(luks);
g_free(password);
@@ -1030,11 +1030,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
/* Setup the block device payload encryption objects */
- block->cipher = qcrypto_cipher_new(luks_opts.cipher_alg,
- luks_opts.cipher_mode,
- masterkey, luks->header.key_bytes,
- errp);
- if (!block->cipher) {
+ if (qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
+ luks_opts.cipher_mode, masterkey,
+ luks->header.key_bytes, 1, errp) < 0) {
goto error;
}
@@ -1341,7 +1339,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
qcrypto_ivgen_free(ivgen);
qcrypto_cipher_free(cipher);
- qcrypto_cipher_free(block->cipher);
+ qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
g_free(luks);
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 36bf5f09b7..cefb3b2a7b 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -44,6 +44,7 @@ qcrypto_block_qcow_has_format(const uint8_t *buf G_GNUC_UNUSED,
static int
qcrypto_block_qcow_init(QCryptoBlock *block,
const char *keysecret,
+ size_t n_threads,
Error **errp)
{
char *password;
@@ -71,11 +72,11 @@ qcrypto_block_qcow_init(QCryptoBlock *block,
goto fail;
}
- block->cipher = qcrypto_cipher_new(QCRYPTO_CIPHER_ALG_AES_128,
- QCRYPTO_CIPHER_MODE_CBC,
- keybuf, G_N_ELEMENTS(keybuf),
- errp);
- if (!block->cipher) {
+ ret = qcrypto_block_init_cipher(block, QCRYPTO_CIPHER_ALG_AES_128,
+ QCRYPTO_CIPHER_MODE_CBC,
+ keybuf, G_N_ELEMENTS(keybuf),
+ n_threads, errp);
+ if (ret < 0) {
ret = -ENOTSUP;
goto fail;
}
@@ -86,7 +87,7 @@ qcrypto_block_qcow_init(QCryptoBlock *block,
return 0;
fail:
- qcrypto_cipher_free(block->cipher);
+ qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
return ret;
}
@@ -99,6 +100,7 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
QCryptoBlockReadFunc readfunc G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED,
unsigned int flags,
+ size_t n_threads,
Error **errp)
{
if (flags & QCRYPTO_BLOCK_OPEN_NO_IO) {
@@ -112,8 +114,8 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
optprefix ? optprefix : "");
return -1;
}
- return qcrypto_block_qcow_init(block,
- options->u.qcow.key_secret, errp);
+ return qcrypto_block_qcow_init(block, options->u.qcow.key_secret,
+ n_threads, errp);
}
}
@@ -133,7 +135,7 @@ qcrypto_block_qcow_create(QCryptoBlock *block,
return -1;
}
/* QCow2 has no special header, since everything is hardwired */
- return qcrypto_block_qcow_init(block, options->u.qcow.key_secret, errp);
+ return qcrypto_block_qcow_init(block, options->u.qcow.key_secret, 1, errp);
}
diff --git a/crypto/block.c b/crypto/block.c
index 3fe3de2ef8..d70d401f87 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -52,6 +52,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ size_t n_threads,
Error **errp)
{
QCryptoBlock *block = g_new0(QCryptoBlock, 1);
@@ -69,11 +70,14 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
block->driver = qcrypto_block_drivers[options->format];
if (block->driver->open(block, options, optprefix,
- readfunc, opaque, flags, errp) < 0) {
+ readfunc, opaque, flags, n_threads, errp) < 0)
+ {
g_free(block);
return NULL;
}
+ qemu_mutex_init(&block->mutex);
+
return block;
}
@@ -105,6 +109,8 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
return NULL;
}
+ qemu_mutex_init(&block->mutex);
+
return block;
}
@@ -148,12 +154,97 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
{
- return block->cipher;
+ /* Ciphers should be accessed through pop/push method to be thread-safe.
+ * Better, they should not be accessed externally at all (note, that
+ * pop/push are static functions)
+ * This function is used only in test with one thread (it's safe to skip
+ * pop/push interface), so it's enough to assert it here:
+ */
+ assert(block->n_ciphers <= 1);
+ return block->ciphers ? block->ciphers[0] : NULL;
+}
+
+
+static QCryptoCipher *qcrypto_block_pop_cipher(QCryptoBlock *block)
+{
+ QCryptoCipher *cipher;
+
+ qemu_mutex_lock(&block->mutex);
+
+ assert(block->n_free_ciphers > 0);
+ block->n_free_ciphers--;
+ cipher = block->ciphers[block->n_free_ciphers];
+
+ qemu_mutex_unlock(&block->mutex);
+
+ return cipher;
+}
+
+
+static void qcrypto_block_push_cipher(QCryptoBlock *block,
+ QCryptoCipher *cipher)
+{
+ qemu_mutex_lock(&block->mutex);
+
+ assert(block->n_free_ciphers < block->n_ciphers);
+ block->ciphers[block->n_free_ciphers] = cipher;
+ block->n_free_ciphers++;
+
+ qemu_mutex_unlock(&block->mutex);
+}
+
+
+int qcrypto_block_init_cipher(QCryptoBlock *block,
+ QCryptoCipherAlgorithm alg,
+ QCryptoCipherMode mode,
+ const uint8_t *key, size_t nkey,
+ size_t n_threads, Error **errp)
+{
+ size_t i;
+
+ assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
+
+ block->ciphers = g_new0(QCryptoCipher *, n_threads);
+
+ for (i = 0; i < n_threads; i++) {
+ block->ciphers[i] = qcrypto_cipher_new(alg, mode, key, nkey, errp);
+ if (!block->ciphers[i]) {
+ qcrypto_block_free_cipher(block);
+ return -1;
+ }
+ block->n_ciphers++;
+ block->n_free_ciphers++;
+ }
+
+ return 0;
}
+void qcrypto_block_free_cipher(QCryptoBlock *block)
+{
+ size_t i;
+
+ if (!block->ciphers) {
+ return;
+ }
+
+ assert(block->n_ciphers == block->n_free_ciphers);
+
+ for (i = 0; i < block->n_ciphers; i++) {
+ qcrypto_cipher_free(block->ciphers[i]);
+ }
+
+ g_free(block->ciphers);
+ block->ciphers = NULL;
+ block->n_ciphers = block->n_free_ciphers = 0;
+}
+
QCryptoIVGen *qcrypto_block_get_ivgen(QCryptoBlock *block)
{
+ /* ivgen should be accessed under mutex. However, this function is used only
+ * in test with one thread, so it's enough to assert it here:
+ */
+ assert(block->n_ciphers <= 1);
return block->ivgen;
}
@@ -184,8 +275,9 @@ void qcrypto_block_free(QCryptoBlock *block)
block->driver->cleanup(block);
- qcrypto_cipher_free(block->cipher);
+ qcrypto_block_free_cipher(block);
qcrypto_ivgen_free(block->ivgen);
+ qemu_mutex_destroy(&block->mutex);
g_free(block);
}
@@ -199,6 +291,7 @@ typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
static int do_qcrypto_block_cipher_encdec(QCryptoCipher *cipher,
size_t niv,
QCryptoIVGen *ivgen,
+ QemuMutex *ivgen_mutex,
int sectorsize,
uint64_t offset,
uint8_t *buf,
@@ -218,10 +311,15 @@ static int do_qcrypto_block_cipher_encdec(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);
+ }
+
+ if (ret < 0) {
goto cleanup;
}
@@ -258,7 +356,7 @@ int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher,
size_t len,
Error **errp)
{
- return do_qcrypto_block_cipher_encdec(cipher, niv, ivgen, sectorsize,
+ return do_qcrypto_block_cipher_encdec(cipher, niv, ivgen, NULL, sectorsize,
offset, buf, len,
qcrypto_cipher_decrypt, errp);
}
@@ -273,12 +371,11 @@ int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher,
size_t len,
Error **errp)
{
- return do_qcrypto_block_cipher_encdec(cipher, niv, ivgen, sectorsize,
+ return do_qcrypto_block_cipher_encdec(cipher, niv, ivgen, NULL, sectorsize,
offset, buf, len,
qcrypto_cipher_encrypt, errp);
}
-
int qcrypto_block_decrypt_helper(QCryptoBlock *block,
int sectorsize,
uint64_t offset,
@@ -286,12 +383,17 @@ int qcrypto_block_decrypt_helper(QCryptoBlock *block,
size_t len,
Error **errp)
{
- return do_qcrypto_block_cipher_encdec(block->cipher, block->niv,
- block->ivgen,
- sectorsize, offset, buf, len,
- qcrypto_cipher_decrypt, errp);
-}
+ int ret;
+ QCryptoCipher *cipher = qcrypto_block_pop_cipher(block);
+ ret = do_qcrypto_block_cipher_encdec(cipher, block->niv, block->ivgen,
+ &block->mutex, sectorsize, offset, buf,
+ len, qcrypto_cipher_decrypt, errp);
+
+ qcrypto_block_push_cipher(block, cipher);
+
+ return ret;
+}
int qcrypto_block_encrypt_helper(QCryptoBlock *block,
int sectorsize,
@@ -300,8 +402,14 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
size_t len,
Error **errp)
{
- return do_qcrypto_block_cipher_encdec(block->cipher, block->niv,
- block->ivgen,
- sectorsize, offset, buf, len,
- qcrypto_cipher_encrypt, errp);
+ int ret;
+ QCryptoCipher *cipher = qcrypto_block_pop_cipher(block);
+
+ ret = do_qcrypto_block_cipher_encdec(cipher, block->niv, block->ivgen,
+ &block->mutex, sectorsize, offset, buf,
+ len, qcrypto_cipher_encrypt, errp);
+
+ qcrypto_block_push_cipher(block, cipher);
+
+ return ret;
}
diff --git a/tests/test-crypto-block.c b/tests/test-crypto-block.c
index fae4ffc453..d309d044ef 100644
--- a/tests/test-crypto-block.c
+++ b/tests/test-crypto-block.c
@@ -305,6 +305,7 @@ static void test_block(gconstpointer opaque)
test_block_read_func,
&header,
0,
+ 1,
NULL);
g_assert(blk == NULL);
@@ -313,6 +314,7 @@ static void test_block(gconstpointer opaque)
test_block_read_func,
&header,
QCRYPTO_BLOCK_OPEN_NO_IO,
+ 1,
&error_abort);
g_assert(qcrypto_block_get_cipher(blk) == NULL);
@@ -327,6 +329,7 @@ static void test_block(gconstpointer opaque)
test_block_read_func,
&header,
0,
+ 1,
&error_abort);
g_assert(blk);
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
@ 2018-12-10 14:06 ` Alberto Garcia
2018-12-10 14:52 ` Daniel P. Berrangé
0 siblings, 1 reply; 16+ messages in thread
From: Alberto Garcia @ 2018-12-10 14:06 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: berrange, mreitz, kwolf, den
On Fri 07 Dec 2018 05:13:51 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> @@ -148,12 +154,97 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
>
> QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
> {
> - return block->cipher;
> + /* Ciphers should be accessed through pop/push method to be thread-safe.
> + * Better, they should not be accessed externally at all (note, that
> + * pop/push are static functions)
> + * This function is used only in test with one thread (it's safe to skip
> + * pop/push interface), so it's enough to assert it here:
> + */
> + assert(block->n_ciphers <= 1);
> + return block->ciphers ? block->ciphers[0] : NULL;
If this is only supposed to be called in test mode I think you can also
assert that g_test_initialized() is true.
And the same with qcrypto_block_get_ivgen() later in this patch.
> +int qcrypto_block_init_cipher(QCryptoBlock *block,
> + QCryptoCipherAlgorithm alg,
> + QCryptoCipherMode mode,
> + const uint8_t *key, size_t nkey,
> + size_t n_threads, Error **errp)
> +{
> + size_t i;
> +
> + assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> +
> + block->ciphers = g_new0(QCryptoCipher *, n_threads);
You can use g_new() instead of g_new0() because you're anyway
overwriting all elements of the array.
But these are minor nits, the patchs looks good to me else.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-10 14:06 ` Alberto Garcia
@ 2018-12-10 14:52 ` Daniel P. Berrangé
2018-12-10 15:09 ` Alberto Garcia
0 siblings, 1 reply; 16+ messages in thread
From: Daniel P. Berrangé @ 2018-12-10 14:52 UTC (permalink / raw)
To: Alberto Garcia
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, mreitz,
kwolf, den
On Mon, Dec 10, 2018 at 03:06:59PM +0100, Alberto Garcia wrote:
> On Fri 07 Dec 2018 05:13:51 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> > @@ -148,12 +154,97 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
> >
> > QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
> > {
> > - return block->cipher;
> > + /* Ciphers should be accessed through pop/push method to be thread-safe.
> > + * Better, they should not be accessed externally at all (note, that
> > + * pop/push are static functions)
> > + * This function is used only in test with one thread (it's safe to skip
> > + * pop/push interface), so it's enough to assert it here:
> > + */
> > + assert(block->n_ciphers <= 1);
> > + return block->ciphers ? block->ciphers[0] : NULL;
>
> If this is only supposed to be called in test mode I think you can also
> assert that g_test_initialized() is true.
>
> And the same with qcrypto_block_get_ivgen() later in this patch.
I consider these APIs as being valid for use anywhere - it just
happens it is only used in the tests right now. So I think it is
ok to assert on n_cipers here.
> > +int qcrypto_block_init_cipher(QCryptoBlock *block,
> > + QCryptoCipherAlgorithm alg,
> > + QCryptoCipherMode mode,
> > + const uint8_t *key, size_t nkey,
> > + size_t n_threads, Error **errp)
> > +{
> > + size_t i;
> > +
> > + assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> > +
> > + block->ciphers = g_new0(QCryptoCipher *, n_threads);
>
> You can use g_new() instead of g_new0() because you're anyway
> overwriting all elements of the array.
I'd rather have it initialized to zero upfront, so if creating any
cipher in the array fails, we don't have uninitialized array elements
during later cleanup code.
> But these are minor nits, the patchs looks good to me else.
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
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 :|
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-10 14:52 ` Daniel P. Berrangé
@ 2018-12-10 15:09 ` Alberto Garcia
2018-12-10 15:16 ` Daniel P. Berrangé
0 siblings, 1 reply; 16+ messages in thread
From: Alberto Garcia @ 2018-12-10 15:09 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, mreitz,
kwolf, den
On Mon 10 Dec 2018 03:52:03 PM CET, Daniel P. Berrangé wrote:
>> > +int qcrypto_block_init_cipher(QCryptoBlock *block,
>> > + QCryptoCipherAlgorithm alg,
>> > + QCryptoCipherMode mode,
>> > + const uint8_t *key, size_t nkey,
>> > + size_t n_threads, Error **errp)
>> > +{
>> > + size_t i;
>> > +
>> > + assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
>> > +
>> > + block->ciphers = g_new0(QCryptoCipher *, n_threads);
>>
>> You can use g_new() instead of g_new0() because you're anyway
>> overwriting all elements of the array.
>
> I'd rather have it initialized to zero upfront, so if creating any
> cipher in the array fails, we don't have uninitialized array elements
> during later cleanup code.
But it is the value of block->n_ciphers that determines the size of the
array, and that is only incremented after each successful iteration:
for (i = 0; i < n_threads; i++) {
block->ciphers[i] = qcrypto_cipher_new(alg, mode, key, nkey, errp);
/* ... error handling ... */
block->n_ciphers++;
block->n_free_ciphers++;
}
In other words, the cleanup code won't touch uninitialized elements
because it cannot even tell the difference between an index that points
to an uninitialized element of the array and an index that points beyond
the allocated memory.
Berto
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-10 15:09 ` Alberto Garcia
@ 2018-12-10 15:16 ` Daniel P. Berrangé
2018-12-10 15:24 ` Alberto Garcia
0 siblings, 1 reply; 16+ messages in thread
From: Daniel P. Berrangé @ 2018-12-10 15:16 UTC (permalink / raw)
To: Alberto Garcia
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, mreitz,
kwolf, den
On Mon, Dec 10, 2018 at 04:09:31PM +0100, Alberto Garcia wrote:
> On Mon 10 Dec 2018 03:52:03 PM CET, Daniel P. Berrangé wrote:
> >> > +int qcrypto_block_init_cipher(QCryptoBlock *block,
> >> > + QCryptoCipherAlgorithm alg,
> >> > + QCryptoCipherMode mode,
> >> > + const uint8_t *key, size_t nkey,
> >> > + size_t n_threads, Error **errp)
> >> > +{
> >> > + size_t i;
> >> > +
> >> > + assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> >> > +
> >> > + block->ciphers = g_new0(QCryptoCipher *, n_threads);
> >>
> >> You can use g_new() instead of g_new0() because you're anyway
> >> overwriting all elements of the array.
> >
> > I'd rather have it initialized to zero upfront, so if creating any
> > cipher in the array fails, we don't have uninitialized array elements
> > during later cleanup code.
>
> But it is the value of block->n_ciphers that determines the size of the
> array, and that is only incremented after each successful iteration:
>
> for (i = 0; i < n_threads; i++) {
> block->ciphers[i] = qcrypto_cipher_new(alg, mode, key, nkey, errp);
> /* ... error handling ... */
> block->n_ciphers++;
> block->n_free_ciphers++;
> }
>
> In other words, the cleanup code won't touch uninitialized elements
> because it cannot even tell the difference between an index that points
> to an uninitialized element of the array and an index that points beyond
> the allocated memory.
.../correctly written/ cleanup code won't touch uninitialized elements...
I prefer to see the code be hardened against mistakes and so would always
zero-initialize everything unless there's compelling performance reason
not to in certain specific places.
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 :|
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-10 15:16 ` Daniel P. Berrangé
@ 2018-12-10 15:24 ` Alberto Garcia
0 siblings, 0 replies; 16+ messages in thread
From: Alberto Garcia @ 2018-12-10 15:24 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, mreitz,
kwolf, den
On Mon 10 Dec 2018 04:16:48 PM CET, Daniel P. Berrangé wrote:
>> In other words, the cleanup code won't touch uninitialized elements
>> because it cannot even tell the difference between an index that
>> points to an uninitialized element of the array and an index that
>> points beyond the allocated memory.
>
> .../correctly written/ cleanup code won't touch uninitialized elements...
>
> I prefer to see the code be hardened against mistakes and so would
> always zero-initialize everything unless there's compelling
> performance reason not to in certain specific places.
Ok, sounds reasonable.
Berto
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/5] crypto threads
2018-12-07 16:13 [Qemu-devel] [PATCH v3 0/5] crypto threads Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2018-12-07 16:13 ` [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
@ 2018-12-10 15:19 ` Daniel P. Berrangé
5 siblings, 0 replies; 16+ messages in thread
From: Daniel P. Berrangé @ 2018-12-10 15:19 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-devel, qemu-block, mreitz, kwolf, berto, den
On Fri, Dec 07, 2018 at 07:13:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
>
> v3:
> 01: add r-b by Daniel and Alberto
> 02: Alberto r-b
> encrypt->encdec for generic
> 03: qcrypto_cipher_*crypt_helper->
> qcrypto_block_cipher_*crypt_helper
> 04: rebased on previous renamings
> Daniel r-b
> 05: use size_t
> improve comment about n_threads
> don't use ret in qcrypto_block_luks_create
> do qemu_mutex_init at the end of open/create
> do qemu_mutex_destroy in free
> fix comment for qcrypto_block_get_cipher
>
> v2:
> 05: fix compilation of test-crypto-block (I am ashamed:(
> and change QCryptoBlock interface to be a bit more safe.
>
> These series are preliminary step before moving encryption code in
> qcow2 to the threads. The first attempt of doing it is on list
> ([PATCH 00/11] qcow2: encryption threads :
> https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00729.html)
> But it's approach with multiplying the whole QCryptoBlock per thread
> was rejected. So, here is a solution to maintain multitasking inside
> QCryptoBlock.
Thanks, I've queued this series for when 4.0 opens up.
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 :|
^ permalink raw reply [flat|nested] 16+ messages in thread