* [Qemu-devel] [PATCH v2 0/5] crypto threads
@ 2018-12-05 14:46 Vladimir Sementsov-Ogievskiy
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-05 14:46 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, den, vsementsov
Hi all.
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.
Vladimir Sementsov-Ogievskiy (5):
crypto/block-luks: fix memory leak in qcrypto_block_luks_create
crypto/block: refactor qcrypto_block_*crypt_helper functions
crypto/block: rename qcrypto_block_*crypt_helper
crypto/block: introduce qcrypto_block_*crypt_helper functions
crypto: support multiple threads accessing one QCryptoBlock
crypto/blockpriv.h | 42 ++++++--
include/crypto/block.h | 3 +
block/crypto.c | 1 +
block/qcow.c | 2 +-
block/qcow2.c | 4 +-
crypto/block-luks.c | 60 ++++++-----
crypto/block-qcow.c | 26 ++---
crypto/block.c | 221 +++++++++++++++++++++++++++++---------
tests/test-crypto-block.c | 3 +
9 files changed, 257 insertions(+), 105 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
2018-12-05 14:46 [Qemu-devel] [PATCH v2 0/5] crypto threads Vladimir Sementsov-Ogievskiy
@ 2018-12-05 14:46 ` Vladimir Sementsov-Ogievskiy
2018-12-06 10:34 ` Daniel P. Berrangé
2018-12-07 12:27 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-05 14:46 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, den, vsementsov
Free block->cipher and block->ivgen on error path.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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] 19+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
2018-12-05 14:46 [Qemu-devel] [PATCH v2 0/5] crypto threads Vladimir Sementsov-Ogievskiy
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
@ 2018-12-05 14:46 ` Vladimir Sementsov-Ogievskiy
2018-12-06 10:36 ` Daniel P. Berrangé
2018-12-07 12:37 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-05 14:46 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, den, vsementsov
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>
---
crypto/block.c | 81 +++++++++++++++++++-------------------------------
1 file changed, 31 insertions(+), 50 deletions(-)
diff --git a/crypto/block.c b/crypto/block.c
index e59d1140fe..f4101f0841 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 (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
+ const void *in,
+ void *out,
+ size_t len,
+ Error **errp);
+
+static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ QCryptoCipherEncryptFunc 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;
- }
-
- 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;
- }
+ return do_qcrypto_block_encrypt(cipher, niv, ivgen, sectorsize, offset,
+ buf, len, qcrypto_cipher_decrypt, errp);
+}
- 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_encrypt(cipher, niv, ivgen, sectorsize, offset,
+ buf, len, qcrypto_cipher_encrypt, errp);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper
2018-12-05 14:46 [Qemu-devel] [PATCH v2 0/5] crypto threads Vladimir Sementsov-Ogievskiy
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
@ 2018-12-05 14:46 ` Vladimir Sementsov-Ogievskiy
2018-12-06 10:39 ` Daniel P. Berrangé
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
2018-12-05 14:47 ` [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
4 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-05 14:46 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, den, vsementsov
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 | 58 ++++++++++++++++++++++-----------------------
4 files changed, 76 insertions(+), 76 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 41840abcec..347a7c010a 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_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_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..72dd51051d 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_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_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_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_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..536ef4ee98 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_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_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 f4101f0841..540b27e581 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -196,15 +196,15 @@ typedef int (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
size_t len,
Error **errp);
-static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
- size_t niv,
- QCryptoIVGen *ivgen,
- int sectorsize,
- uint64_t offset,
- uint8_t *buf,
- size_t len,
- QCryptoCipherEncryptFunc func,
- Error **errp)
+static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher,
+ size_t niv,
+ QCryptoIVGen *ivgen,
+ int sectorsize,
+ uint64_t offset,
+ uint8_t *buf,
+ size_t len,
+ QCryptoCipherEncryptFunc func,
+ Error **errp)
{
uint8_t *iv;
int ret = -1;
@@ -249,29 +249,29 @@ static int do_qcrypto_block_encrypt(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_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_encrypt(cipher, niv, ivgen, sectorsize, offset,
- buf, len, qcrypto_cipher_decrypt, errp);
+ return do_qcrypto_cipher_encrypt(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_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_encrypt(cipher, niv, ivgen, sectorsize, offset,
- buf, len, qcrypto_cipher_encrypt, errp);
+ return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, sectorsize, offset,
+ buf, len, qcrypto_cipher_encrypt, errp);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions
2018-12-05 14:46 [Qemu-devel] [PATCH v2 0/5] crypto threads Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
@ 2018-12-05 14:46 ` Vladimir Sementsov-Ogievskiy
2018-12-06 10:39 ` Daniel P. Berrangé
2018-12-05 14:47 ` [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
4 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-05 14:46 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, den, vsementsov
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>
---
crypto/blockpriv.h | 14 ++++++++++++++
crypto/block-luks.c | 14 ++++++--------
crypto/block-qcow.c | 14 ++++++--------
crypto/block.c | 26 ++++++++++++++++++++++++++
4 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 347a7c010a..e9fe3e5687 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -96,4 +96,18 @@ int qcrypto_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 72dd51051d..bbffd80fff 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_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_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 536ef4ee98..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_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_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 540b27e581..3edd9ec251 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -275,3 +275,29 @@ int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher,
return do_qcrypto_cipher_encrypt(cipher, niv, ivgen, sectorsize, 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_cipher_encrypt(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_cipher_encrypt(block->cipher, block->niv, block->ivgen,
+ sectorsize, offset, buf, len,
+ qcrypto_cipher_encrypt, errp);
+}
--
2.18.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-05 14:46 [Qemu-devel] [PATCH v2 0/5] crypto threads Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
@ 2018-12-05 14:47 ` Vladimir Sementsov-Ogievskiy
2018-12-06 10:54 ` Daniel P. Berrangé
4 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-05 14:47 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: berrange, mreitz, kwolf, den, vsementsov
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 | 3 +
block/crypto.c | 1 +
block/qcow.c | 2 +-
block/qcow2.c | 4 +-
crypto/block-luks.c | 25 +++----
crypto/block-qcow.c | 20 +++---
crypto/block.c | 146 +++++++++++++++++++++++++++++++++-----
tests/test-crypto-block.c | 3 +
9 files changed, 176 insertions(+), 44 deletions(-)
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index e9fe3e5687..86dae49210 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;
+ int n_ciphers;
+ int 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,
+ int 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,
+ int 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..866a89b06a 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -75,6 +75,8 @@ typedef enum {
* @readfunc: callback for reading data from the volume
* @opaque: data to pass to @readfunc
* @flags: bitmask of QCryptoBlockOpenFlags values
+ * @n_threads: prepare block to multi tasking with up to
+ * @n_threads threads
* @errp: pointer to a NULL-initialized error object
*
* Create a new block encryption object for an existing
@@ -107,6 +109,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ int 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 bbffd80fff..41fb0aa2e7 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,
+ int 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);
@@ -888,6 +888,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
void *opaque,
Error **errp)
{
+ int ret;
QCryptoBlockLUKS *luks;
QCryptoBlockCreateOptionsLUKS luks_opts;
Error *local_err = NULL;
@@ -1030,11 +1031,11 @@ 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) {
+ ret = qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
+ luks_opts.cipher_mode,
+ masterkey, luks->header.key_bytes,
+ 1, errp);
+ if (ret < 0) {
goto error;
}
@@ -1341,7 +1342,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..e668145561 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,
+ int 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,
+ int 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 3edd9ec251..43426268e6 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -52,10 +52,13 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
QCryptoBlockReadFunc readfunc,
void *opaque,
unsigned int flags,
+ int n_threads,
Error **errp)
{
QCryptoBlock *block = g_new0(QCryptoBlock, 1);
+ qemu_mutex_init(&block->mutex);
+
block->format = options->format;
if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -69,7 +72,8 @@ 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;
}
@@ -87,6 +91,8 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
{
QCryptoBlock *block = g_new0(QCryptoBlock, 1);
+ qemu_mutex_init(&block->mutex);
+
block->format = options->format;
if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -148,12 +154,94 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
{
- return block->cipher;
+ /* 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->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,
+ int n_threads, Error **errp)
+{
+ int 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)
+{
+ int 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,7 +272,7 @@ 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);
g_free(block);
}
@@ -199,6 +287,7 @@ typedef int (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
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);
+ }
+
+ 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);
}
-
int qcrypto_block_decrypt_helper(QCryptoBlock *block,
int sectorsize,
uint64_t offset,
@@ -284,11 +379,17 @@ int qcrypto_block_decrypt_helper(QCryptoBlock *block,
size_t len,
Error **errp)
{
- return do_qcrypto_cipher_encrypt(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_cipher_encrypt(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,
@@ -297,7 +398,14 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
size_t len,
Error **errp)
{
- return do_qcrypto_cipher_encrypt(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_cipher_encrypt(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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
@ 2018-12-06 10:34 ` Daniel P. Berrangé
2018-12-07 12:27 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
1 sibling, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-06 10:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz, kwolf, den
On Wed, Dec 05, 2018 at 05:46:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Free block->cipher and block->ivgen on error path.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> crypto/block-luks.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
@ 2018-12-06 10:36 ` Daniel P. Berrangé
2018-12-06 17:36 ` Vladimir Sementsov-Ogievskiy
2018-12-07 12:37 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
1 sibling, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-06 10:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz, kwolf, den
On Wed, Dec 05, 2018 at 05:46:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
> crypto/block.c | 81 +++++++++++++++++++-------------------------------
> 1 file changed, 31 insertions(+), 50 deletions(-)
>
> diff --git a/crypto/block.c b/crypto/block.c
> index e59d1140fe..f4101f0841 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 (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
> + const void *in,
> + void *out,
> + size_t len,
> + Error **errp);
> +
> +static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
Can we call this functuon 'encdec', since it is misleading to call
it just 'encrypt' when its used for decrypt too.
> + size_t niv,
> + QCryptoIVGen *ivgen,
> + int sectorsize,
> + uint64_t offset,
> + uint8_t *buf,
> + size_t len,
> + QCryptoCipherEncryptFunc func,
And call this 'EncDecFunc' too
> + 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;
> - }
> -
> - 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;
> - }
> + return do_qcrypto_block_encrypt(cipher, niv, ivgen, sectorsize, offset,
> + buf, len, qcrypto_cipher_decrypt, errp);
> +}
>
> - 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_encrypt(cipher, niv, ivgen, sectorsize, offset,
> + buf, len, qcrypto_cipher_encrypt, errp);
> }
> --
> 2.18.0
>
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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
@ 2018-12-06 10:39 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-06 10:39 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz, kwolf, den
On Wed, Dec 05, 2018 at 05:46:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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.
The naming convention I'm following is that methods defined in a file
should all use the name of the file as their prefix. Hence everything
in crypto/block.* should be named with a qcrypto_block_ prefix
>
> 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 | 58 ++++++++++++++++++++++-----------------------
> 4 files changed, 76 insertions(+), 76 deletions(-)
>
> diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
> index 41840abcec..347a7c010a 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_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_cipher_encrypt_helper(QCryptoCipher *cipher,
> + size_t niv,
> + QCryptoIVGen *ivgen,
> + int sectorsize,
> + uint64_t offset,
> + uint8_t *buf,
> + size_t len,
> + Error **errp);
Rather than dropping 'block' from the method name, just add
'cipher'. eg
qcrypto_block_cipher_decrypt_helper
qcrypto_block_cipher_encrypt_helper
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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
@ 2018-12-06 10:39 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-06 10:39 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz, kwolf, den
On Wed, Dec 05, 2018 at 05:46:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
> crypto/blockpriv.h | 14 ++++++++++++++
> crypto/block-luks.c | 14 ++++++--------
> crypto/block-qcow.c | 14 ++++++--------
> crypto/block.c | 26 ++++++++++++++++++++++++++
> 4 files changed, 52 insertions(+), 16 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-05 14:47 ` [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
@ 2018-12-06 10:54 ` Daniel P. Berrangé
2018-12-06 17:42 ` Vladimir Sementsov-Ogievskiy
2018-12-07 14:44 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-06 10:54 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz, kwolf, den
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>
> ---
> crypto/blockpriv.h | 16 ++++-
> include/crypto/block.h | 3 +
> block/crypto.c | 1 +
> block/qcow.c | 2 +-
> block/qcow2.c | 4 +-
> crypto/block-luks.c | 25 +++----
> crypto/block-qcow.c | 20 +++---
> crypto/block.c | 146 +++++++++++++++++++++++++++++++++-----
> tests/test-crypto-block.c | 3 +
> 9 files changed, 176 insertions(+), 44 deletions(-)
>
> diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
> index e9fe3e5687..86dae49210 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;
> + int n_ciphers;
> + int n_free_ciphers;
size_t for both of these since they're effectively array indexes.
> 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,
> + int n_threads,
unsigned int, and more below which I won't repeat...
> diff --git a/include/crypto/block.h b/include/crypto/block.h
> index cd18f46d56..866a89b06a 100644
> --- a/include/crypto/block.h
> +++ b/include/crypto/block.h
> @@ -75,6 +75,8 @@ typedef enum {
> * @readfunc: callback for reading data from the volume
> * @opaque: data to pass to @readfunc
> * @flags: bitmask of QCryptoBlockOpenFlags values
> + * @n_threads: prepare block to multi tasking with up to
> + * @n_threads threads
The description is a little awkward, Lets say
@n_threads: allow concurrent I/O from up to @n_threads threads
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index bbffd80fff..41fb0aa2e7 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -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);
> @@ -888,6 +888,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> void *opaque,
> Error **errp)
> {
> + int ret;
> QCryptoBlockLUKS *luks;
> QCryptoBlockCreateOptionsLUKS luks_opts;
> Error *local_err = NULL;
> @@ -1030,11 +1031,11 @@ 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) {
> + ret = qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
> + luks_opts.cipher_mode,
> + masterkey, luks->header.key_bytes,
> + 1, errp);
> + if (ret < 0) {
No need for ret, since we're only using it for a failure check. Just
put the method call in the if ().
> diff --git a/crypto/block.c b/crypto/block.c
> index 3edd9ec251..43426268e6 100644
> --- a/crypto/block.c
> +++ b/crypto/block.c
> @@ -52,10 +52,13 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
> QCryptoBlockReadFunc readfunc,
> void *opaque,
> unsigned int flags,
> + int n_threads,
> Error **errp)
> {
> QCryptoBlock *block = g_new0(QCryptoBlock, 1);
>
> + qemu_mutex_init(&block->mutex);
We need a corresponding qemu_mutex_destroy() when free'ing the QCryptoBlock.
> @@ -148,12 +154,94 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
>
> QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
> {
> - return block->cipher;
> + /* 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:
> + */
Comment is wrong since this is about ciphers, not ivgen.
> + 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,
> + int n_threads, Error **errp)
> +{
> + int i;
size_t for loop indexes
> +
> + 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;
> +}
> @@ -199,6 +287,7 @@ typedef int (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
> 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.
>
> -
> int qcrypto_block_decrypt_helper(QCryptoBlock *block,
> int sectorsize,
> uint64_t offset,
> @@ -284,11 +379,17 @@ int qcrypto_block_decrypt_helper(QCryptoBlock *block,
> size_t len,
> Error **errp)
> {
> - return do_qcrypto_cipher_encrypt(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_cipher_encrypt(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,
> @@ -297,7 +398,14 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
> size_t len,
> Error **errp)
> {
> - return do_qcrypto_cipher_encrypt(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_cipher_encrypt(cipher, block->niv, block->ivgen,
> + &block->mutex, sectorsize, offset, buf, len,
> + qcrypto_cipher_encrypt, errp);
> +
> + qcrypto_block_push_cipher(block, cipher);
> +
> + return ret;
> }
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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
2018-12-06 10:36 ` Daniel P. Berrangé
@ 2018-12-06 17:36 ` Vladimir Sementsov-Ogievskiy
2018-12-07 9:45 ` Daniel P. Berrangé
0 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-06 17:36 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, qemu-block, mreitz, kwolf, Denis Lunev
06.12.2018 13:36, Daniel P. Berrangé wrote:
> On Wed, Dec 05, 2018 at 05:46:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 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>
>> ---
>> crypto/block.c | 81 +++++++++++++++++++-------------------------------
>> 1 file changed, 31 insertions(+), 50 deletions(-)
>>
>> diff --git a/crypto/block.c b/crypto/block.c
>> index e59d1140fe..f4101f0841 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 (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
>> + const void *in,
>> + void *out,
>> + size_t len,
>> + Error **errp);
>> +
>> +static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
>
> Can we call this functuon 'encdec', since it is misleading to call
> it just 'encrypt' when its used for decrypt too.
Maybe just _crypt ?)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-06 10:54 ` Daniel P. Berrangé
@ 2018-12-06 17:42 ` Vladimir Sementsov-Ogievskiy
2018-12-07 9:45 ` Daniel P. Berrangé
2018-12-07 14:44 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-06 17:42 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, qemu-block, mreitz, kwolf, Denis Lunev
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
2018-12-06 17:36 ` Vladimir Sementsov-Ogievskiy
@ 2018-12-07 9:45 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-07 9:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-devel, qemu-block, mreitz, kwolf, Denis Lunev
On Thu, Dec 06, 2018 at 05:36:29PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 06.12.2018 13:36, Daniel P. Berrangé wrote:
> > On Wed, Dec 05, 2018 at 05:46:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> 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>
> >> ---
> >> crypto/block.c | 81 +++++++++++++++++++-------------------------------
> >> 1 file changed, 31 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/crypto/block.c b/crypto/block.c
> >> index e59d1140fe..f4101f0841 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 (*QCryptoCipherEncryptFunc)(QCryptoCipher *cipher,
> >> + const void *in,
> >> + void *out,
> >> + size_t len,
> >> + Error **errp);
> >> +
> >> +static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
> >
> > Can we call this functuon 'encdec', since it is misleading to call
> > it just 'encrypt' when its used for decrypt too.
>
> Maybe just _crypt ?)
Sure thats fine
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] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-06 17:42 ` Vladimir Sementsov-Ogievskiy
@ 2018-12-07 9:45 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-07 9:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-devel, qemu-block, mreitz, kwolf, Denis Lunev
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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
2018-12-06 10:34 ` Daniel P. Berrangé
@ 2018-12-07 12:27 ` Alberto Garcia
1 sibling, 0 replies; 19+ messages in thread
From: Alberto Garcia @ 2018-12-07 12:27 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: kwolf, den, berrange, mreitz
On Wed 05 Dec 2018 03:46:56 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Free block->cipher and block->ivgen on error path.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
2018-12-06 10:36 ` Daniel P. Berrangé
@ 2018-12-07 12:37 ` Alberto Garcia
1 sibling, 0 replies; 19+ messages in thread
From: Alberto Garcia @ 2018-12-07 12:37 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: kwolf, den, berrange, mreitz
On Wed 05 Dec 2018 03:46:57 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> +static int do_qcrypto_block_encrypt(QCryptoCipher *cipher,
I also prefer encdec (which is the naming already used in crypto/xts.c),
but I'm also fine with crypt.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-06 10:54 ` Daniel P. Berrangé
2018-12-06 17:42 ` Vladimir Sementsov-Ogievskiy
@ 2018-12-07 14:44 ` Vladimir Sementsov-Ogievskiy
2018-12-07 14:45 ` Daniel P. Berrangé
1 sibling, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-07 14:44 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, qemu-block, mreitz, kwolf, Denis Lunev
06.12.2018 13:54, Daniel P. Berrangé wrote:
>> @@ -31,8 +32,12 @@ struct QCryptoBlock {
>> const QCryptoBlockDriver *driver;
>> void *opaque;
>>
>> - QCryptoCipher *cipher;
>> + QCryptoCipher **ciphers;
>> + int n_ciphers;
>> + int n_free_ciphers;
> size_t for both of these since they're effectively array indexes.
>
>> 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,
>> + int n_threads,
> unsigned int, and more below which I won't repeat...
>
Why not size_t here, then? this parameter directly maps into n_ciphers
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
2018-12-07 14:44 ` Vladimir Sementsov-Ogievskiy
@ 2018-12-07 14:45 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2018-12-07 14:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-devel, qemu-block, mreitz, kwolf, Denis Lunev
On Fri, Dec 07, 2018 at 02:44:12PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 06.12.2018 13:54, Daniel P. Berrangé wrote:
> >> @@ -31,8 +32,12 @@ struct QCryptoBlock {
> >> const QCryptoBlockDriver *driver;
> >> void *opaque;
> >>
> >> - QCryptoCipher *cipher;
> >> + QCryptoCipher **ciphers;
> >> + int n_ciphers;
> >> + int n_free_ciphers;
> > size_t for both of these since they're effectively array indexes.
> >
> >> 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,
> >> + int n_threads,
> > unsigned int, and more below which I won't repeat...
> >
>
> Why not size_t here, then? this parameter directly maps into n_ciphers
Yes, I'm fine with size_t 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 :|
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-12-07 14:45 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-05 14:46 [Qemu-devel] [PATCH v2 0/5] crypto threads Vladimir Sementsov-Ogievskiy
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create Vladimir Sementsov-Ogievskiy
2018-12-06 10:34 ` Daniel P. Berrangé
2018-12-07 12:27 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
2018-12-06 10:36 ` Daniel P. Berrangé
2018-12-06 17:36 ` Vladimir Sementsov-Ogievskiy
2018-12-07 9:45 ` Daniel P. Berrangé
2018-12-07 12:37 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper Vladimir Sementsov-Ogievskiy
2018-12-06 10:39 ` Daniel P. Berrangé
2018-12-05 14:46 ` [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions Vladimir Sementsov-Ogievskiy
2018-12-06 10:39 ` Daniel P. Berrangé
2018-12-05 14:47 ` [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock Vladimir Sementsov-Ogievskiy
2018-12-06 10:54 ` Daniel P. Berrangé
2018-12-06 17:42 ` Vladimir Sementsov-Ogievskiy
2018-12-07 9:45 ` Daniel P. Berrangé
2018-12-07 14:44 ` Vladimir Sementsov-Ogievskiy
2018-12-07 14:45 ` Daniel P. Berrangé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.