crypto threads
Subsystems (2)
v2 cover crypto threads
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(-)
Signature · 1 line
2.18.0
PATCH v2 1/5 crypto/block-luks: fix memory leak in qcrypto_block_luks_create
Free block->cipher and block->ivgen on error path.
--- crypto/block-luks.c | 3 +++ 1 file changed, 3 insertions(+)
crypto/block-luks.c +3 -0
@@ -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 ; }
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
Free
block->cipherandblock->ivgenon error path.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- crypto/block-luks.c | 3 +++ 1 file changed, 3 insertions(+)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/5] crypto/block-luks: fix memory leak in qcrypto_block_luks_create
In reply to Vladimir Sementsov-OgievskiyQuoted 3 lines · Vladimir Sementsov-Ogievskiy
Free
block->cipherandblock->ivgenon error path.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
Berto
PATCH v2 2/5 crypto/block: refactor qcrypto_block_*crypt_helper functions
In reply to Vladimir Sementsov-Ogievskiyqcrypto_block_encrypt_helper and qcrypto_block_decrypt_helper are almost identical, let's reduce code duplication and simplify further improvements.
--- crypto/block.c | 81 +++++++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 50 deletions(-)
crypto/block.c +31 -50
@@ -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); }
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
Quoted 22 lines · Vladimir Sementsov-Ogievskiy
qcrypto_block_encrypt_helperandqcrypto_block_decrypt_helperare almost identical, let's reduce code duplication and simplify further improvements.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- crypto/block.c | 81 +++++++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 50 deletions(-)crypto/block.c +0
-5@@ -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,
crypto/block.c void qcrypto_block_free(QCryptoBlock *block) · line 199
Quoted 10 lines
crypto/block.c +7
-3-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.
crypto/block.c void qcrypto_block_free(QCryptoBlock *block) · line 206
Quoted 7 lines
crypto/block.c +7
-0+size_t niv, + QCryptoIVGen *ivgen, + int sectorsize, + uint64_t offset, + uint8_t *buf, + size_t len, + QCryptoCipherEncryptFunc func,
And call this 'EncDecFunc' too
Quoted 86 lines
crypto/block.c +17
-42+ 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); }Signature · 1 line
2.18.0
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
Quoted 28 lines · Daniel P. Berrangé
Quoted 27 lines · Vladimir Sementsov-Ogievskiy
qcrypto_block_encrypt_helperandqcrypto_block_decrypt_helperare almost identical, let's reduce code duplication and simplify further improvements.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- crypto/block.c | 81 +++++++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 50 deletions(-)crypto/block.c +2
-8@@ -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,
Quoted 8 lines
crypto/block.c void qcrypto_block_free(QCryptoBlock *block) · line 199
Quoted 5 lines
crypto/block.c +5
-0+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 ?)
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
Quoted 38 lines · Vladimir Sementsov-Ogievskiy
Quoted 28 lines · Daniel P. Berrangé
Quoted 27 lines · Vladimir Sementsov-Ogievskiy
qcrypto_block_encrypt_helperandqcrypto_block_decrypt_helperare almost identical, let's reduce code duplication and simplify further improvements.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- crypto/block.c | 81 +++++++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 50 deletions(-)crypto/block.c +2
-8@@ -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,Quoted 8 lines
crypto/block.c void qcrypto_block_free(QCryptoBlock *block) · line 199
Quoted 5 lines
crypto/block.c +5
-0+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.
Quoted 1 line
Maybe just
_crypt?)
Sure thats fine
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/5] crypto/block: refactor qcrypto_block_*crypt_helper functions
In reply to Vladimir Sementsov-OgievskiyQuoted 8 lines · Vladimir Sementsov-Ogievskiy
qcrypto_block_encrypt_helperandqcrypto_block_decrypt_helperare almost identical, let's reduce code duplication and simplify further improvements.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>+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.
Berto
PATCH v2 3/5 crypto/block: rename qcrypto_block_*crypt_helper
In reply to Vladimir Sementsov-OgievskiyRename 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.
--- 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(-)
crypto/blockpriv.h +17 -17
@@ -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 */
crypto/block-luks.c +22 -22
68 lines not on this page · raw message
crypto/block-qcow.c +8 -8
@@ -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); }
crypto/block.c +29 -29
73 lines not on this page · raw message
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v2 3/5] crypto/block: rename qcrypto_block_*crypt_helper
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
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
Quoted 41 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- 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(-)crypto/blockpriv.h +7
-17@@ -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,
crypto/blockpriv.h struct QCryptoBlockDriver { · line 97
Quoted 10 lines
crypto/blockpriv.h +10
-0+ 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
Signature · 3 lines
|: 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 :|
PATCH v2 4/5 crypto/block: introduce qcrypto_block_*crypt_helper functions
In reply to Vladimir Sementsov-OgievskiyIntroduce QCryptoBlock-based functions and use them where possible. This is needed to implement thread-safe encrypt/decrypt operations.
--- 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(-)
crypto/blockpriv.h +14 -0
@@ -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 */
crypto/block-luks.c +6 -8
@@ -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); }
crypto/block-qcow.c +6 -8
@@ -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); }
crypto/block.c +26 -0
@@ -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); +}
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v2 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions
Quoted 10 lines · Vladimir Sementsov-Ogievskiy
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 <[email protected]>--- 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(-)
Regards,
Daniel
Signature · 3 lines
|: 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 :|
PATCH v2 5/5 crypto: support multiple threads accessing one QCryptoBlock
In reply to Vladimir Sementsov-OgievskiyThe 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.
--- 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(-)
crypto/blockpriv.h +15 -1
@@ -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 */
include/crypto/block.h +3 -0
@@ -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);/**
block/crypto.c +1 -0
@@ -229,6 +229,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, block_crypto_read_func, bs, cflags, +1 , errp);if (!crypto->block) {
block/qcow.c +1 -1
@@ -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;
block/qcow2.c +2 -2
@@ -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;
crypto/block-luks.c +13 -12
@@ -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);
crypto/block-qcow.c +11 -9
@@ -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); }
crypto/block.c +127 -19
222 lines not on this page · raw message
tests/test-crypto-block.c +3 -0
@@ -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);
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
Quoted 31 lines · Vladimir Sementsov-Ogievskiy
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 <[email protected]>--- 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(-)crypto/blockpriv.h +1
-0@@ -22,6 +22,7 @@#define QCRYPTO_BLOCKPRIV_H #include "crypto/block.h" +#include "qemu/thread.h" typedef struct QCryptoBlockDriver QCryptoBlockDriver ;
crypto/blockpriv.h struct QCryptoBlock { · line 37
Quoted 7 lines
crypto/blockpriv.h +3
-1@@ -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.
crypto/blockpriv.h struct QCryptoBlockDriver { · line 54
Quoted 11 lines
crypto/blockpriv.h +3
-0QCryptoIVGen *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...
include/crypto/block.h typedef enum { · line 79
Quoted 10 lines
include/crypto/block.h +2
-0@@ -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
Quoted 25 lines
crypto/block-luks.c +2
-1@@ -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 */
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1038
Quoted 10 lines
crypto/block-luks.c +5
-5- 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 ().
Quoted 5 lines
crypto/block.c +0
-0
crypto/block.c QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, · line 60
Quoted 9 lines
crypto/block.c +2
-0@@ -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.
crypto/block.c int qcrypto_block_encrypt(QCryptoBlock *block, · line 159
Quoted 8 lines
crypto/block.c +3
-1@@ -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.
Quoted 31 lines
crypto/block.c +28
-0+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);
crypto/block.c int qcrypto_block_encrypt(QCryptoBlock *block, · line 200
Quoted 10 lines
crypto/block.c +10
-0+} + + +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
Quoted 32 lines
crypto/block.c +18
-1+ +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,
crypto/block.c static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher, · line 316
Quoted 10 lines
crypto/block.c +7
-3- 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....
Quoted 18 lines
crypto/block.c +5
-2+ +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 ); }
crypto/block.c int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher, · line 373
Quoted 9 lines
crypto/block.c +3
-2@@ -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.
Quoted 35 lines
crypto/block.c +11
-8- 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;
crypto/block.c int qcrypto_block_encrypt_helper(QCryptoBlock *block, · line 411
Quoted 10 lines
crypto/block.c +9
-0+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
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
Quoted 8 lines · Daniel P. Berrangé
Quoted 7 lines · Vladimir Sementsov-Ogievskiy
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 <[email protected]>---
[..]
Quoted 2 lines
Quoted 2 lines
crypto/block.c +0
-0
[..]
Quoted 42 lines
Quoted 12 lines
static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher,
size_tniv, QCryptoIVGen *ivgen,crypto/block.c +1
-1+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,crypto/block.c static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher, · line 316
Quoted 10 lines
crypto/block.c +7
-3- 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_mutexcompulsory....Quoted 17 lines
crypto/block.c +5
-2+ +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 ); }
Quoted 14 lines
crypto/block.c int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher, · line 373
Quoted 10 lines
crypto/block.c +3
-2@@ -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.
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
Quoted 74 lines · Vladimir Sementsov-Ogievskiy
Quoted 8 lines · Daniel P. Berrangé
Quoted 7 lines · Vladimir Sementsov-Ogievskiy
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 <[email protected]>---
[..]
Quoted 2 lines
Quoted 2 lines
crypto/block.c +0
-0[..]
Quoted 42 lines
Quoted 12 lines
static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher,
size_tniv, QCryptoIVGen *ivgen,crypto/block.c +1
-1+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,crypto/block.c static int do_qcrypto_cipher_encrypt(QCryptoCipher *cipher, · line 316
Quoted 10 lines
crypto/block.c +7
-3- 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_mutexcompulsory....Quoted 17 lines
crypto/block.c +5
-2+ +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 ); }Quoted 14 lines
crypto/block.c int qcrypto_cipher_encrypt_helper(QCryptoCipher *cipher, · line 373
Quoted 10 lines
crypto/block.c +3
-2@@ -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.
Quoted 2 lines
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
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
In reply to Daniel P. BerrangéQuoted 10 lines · Daniel P. Berrangé
crypto/blockpriv.h struct QCryptoBlock { · line 37
Quoted 8 lines
crypto/blockpriv.h +3
-1@@ -31,8 +32,12 @@ struct QCryptoBlock {const QCryptoBlockDriver *driver;void *opaque; -QCryptoCipher *cipher; +QCryptoCipher **ciphers; +int n_ciphers; +int n_free_ciphers;
size_tfor both of these since they're effectively array indexes.
Quoted 13 lines
crypto/blockpriv.h struct QCryptoBlockDriver { · line 54
Quoted 11 lines
crypto/blockpriv.h +3
-0QCryptoIVGen *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
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 5/5] crypto: support multiple threads accessing one QCryptoBlock
Quoted 25 lines · Vladimir Sementsov-Ogievskiy
Quoted 10 lines · Daniel P. Berrangé
crypto/blockpriv.h struct QCryptoBlock { · line 37
Quoted 8 lines
crypto/blockpriv.h +3
-1@@ -31,8 +32,12 @@ struct QCryptoBlock {const QCryptoBlockDriver *driver;void *opaque; -QCryptoCipher *cipher; +QCryptoCipher **ciphers; +int n_ciphers; +int n_free_ciphers;
size_tfor both of these since they're effectively array indexes.Quoted 13 lines
crypto/blockpriv.h struct QCryptoBlockDriver { · line 54
Quoted 11 lines
crypto/blockpriv.h +3
-0QCryptoIVGen *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...
Quoted 1 line
Why not
size_there, then? this parameter directly maps inton_ciphers
Yes, I'm fine with size_t too
Regards,
Daniel
Signature · 3 lines
|: 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 :|