| Message ID | 20200518122041.10694-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | LUKS: encryption slot management using amend interface | expand |
Patchew URL: https://patchew.org/QEMU/20200518122041.10694-1-mlevitsk@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200518122041.10694-1-mlevitsk@redhat.com
Subject: [PATCH v7 00/14] LUKS: encryption slot management using amend interface
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/20200512064900.28554-1-pauldzim@gmail.com -> patchew/20200512064900.28554-1-pauldzim@gmail.com
- [tag update] patchew/20200518083704.52646-1-david@redhat.com -> patchew/20200518083704.52646-1-david@redhat.com
* [new tag] patchew/20200518151255.10785-1-kraxel@redhat.com -> patchew/20200518151255.10785-1-kraxel@redhat.com
* [new tag] patchew/20200518153025.13576-1-alex.bennee@linaro.org -> patchew/20200518153025.13576-1-alex.bennee@linaro.org
Switched to a new branch 'test'
93adc63 iotests: add tests for blockdev-amend
4dd3bcb block/qcow2: implement blockdev-amend
3924c27 block/crypto: implement blockdev-amend
1fa4dd9 block/core: add generic infrastructure for x-blockdev-amend qmp command
386b94f iotests: qemu-img tests for luks key management
e231475 iotests: filter few more luks specific create options
35ad578 block/qcow2: extend qemu-img amend interface with crypto options
e7a1398 block/crypto: implement the encryption key management
aabc8dc block/crypto: rename two functions
b49aedc block/amend: refactor qcow2 amend options
7faebb5 block/amend: separate amend and create options for qemu-img
7fd48d8 block/amend: add 'force' option
03a65ce qcrypto/luks: implement encryption key management
d5426f8 qcrypto/core: add generic infrastructure for crypto options amendment
=== OUTPUT BEGIN ===
1/14 Checking commit d5426f85b5cf (qcrypto/core: add generic infrastructure for crypto options amendment)
2/14 Checking commit 03a65ce69afd (qcrypto/luks: implement encryption key management)
3/14 Checking commit 7fd48d81c1ff (block/amend: add 'force' option)
4/14 Checking commit 7faebb5b9d94 (block/amend: separate amend and create options for qemu-img)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#32: FILE: block/qcow2.c:5637:
+#define QCOW_COMMON_OPTIONS \
+ { \
+ .name = BLOCK_OPT_SIZE, \
+ .type = QEMU_OPT_SIZE, \
+ .help = "Virtual disk size" \
+ }, \
+ { \
+ .name = BLOCK_OPT_COMPAT_LEVEL, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Compatibility level (v2 [0.10] or v3 [1.1])" \
+ }, \
+ { \
+ .name = BLOCK_OPT_BACKING_FILE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "File name of a base image" \
+ }, \
+ { \
+ .name = BLOCK_OPT_BACKING_FMT, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Image format of the base image" \
+ }, \
+ { \
+ .name = BLOCK_OPT_DATA_FILE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "File name of an external data file" \
+ }, \
+ { \
+ .name = BLOCK_OPT_DATA_FILE_RAW, \
+ .type = QEMU_OPT_BOOL, \
+ .help = "The external data file must stay valid " \
+ "as a raw image" \
+ }, \
+ { \
+ .name = BLOCK_OPT_ENCRYPT, \
+ .type = QEMU_OPT_BOOL, \
+ .help = "Encrypt the image with format 'aes'. (Deprecated " \
+ "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)", \
+ }, \
+ { \
+ .name = BLOCK_OPT_ENCRYPT_FORMAT, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Encrypt the image, format choices: 'aes', 'luks'", \
+ }, \
+ BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.", \
+ "ID of secret providing qcow AES key or LUKS passphrase"), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."), \
+ { \
+ .name = BLOCK_OPT_CLUSTER_SIZE, \
+ .type = QEMU_OPT_SIZE, \
+ .help = "qcow2 cluster size", \
+ .def_value_str = stringify(DEFAULT_CLUSTER_SIZE) \
+ }, \
+ { \
+ .name = BLOCK_OPT_PREALLOC, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Preallocation mode (allowed values: off, " \
+ "metadata, falloc, full)" \
+ }, \
+ { \
+ .name = BLOCK_OPT_LAZY_REFCOUNTS, \
+ .type = QEMU_OPT_BOOL, \
+ .help = "Postpone refcount updates", \
+ .def_value_str = "off" \
+ }, \
+ { \
+ .name = BLOCK_OPT_REFCOUNT_BITS, \
+ .type = QEMU_OPT_NUMBER, \
+ .help = "Width of a reference count entry in bits", \
+ .def_value_str = "16" \
+ }, \
+ { \
+ .name = BLOCK_OPT_COMPRESSION_TYPE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Compression method used for image cluster " \
+ "compression", \
+ .def_value_str = "zlib" \
+ }
total: 1 errors, 0 warnings, 244 lines checked
Patch 4/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/14 Checking commit b49aedccab95 (block/amend: refactor qcow2 amend options)
6/14 Checking commit aabc8dc7b6c0 (block/crypto: rename two functions)
7/14 Checking commit e7a1398191aa (block/crypto: implement the encryption key management)
8/14 Checking commit 35ad578c65eb (block/qcow2: extend qemu-img amend interface with crypto options)
9/14 Checking commit e231475b1b24 (iotests: filter few more luks specific create options)
10/14 Checking commit 386b94fced14 (iotests: qemu-img tests for luks key management)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100755
total: 0 errors, 1 warnings, 431 lines checked
Patch 10/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/14 Checking commit 1fa4dd9c14e7 (block/core: add generic infrastructure for x-blockdev-amend qmp command)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31:
new file mode 100644
total: 0 errors, 1 warnings, 221 lines checked
Patch 11/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/14 Checking commit 3924c27718fc (block/crypto: implement blockdev-amend)
13/14 Checking commit 4dd3bcbb826b (block/qcow2: implement blockdev-amend)
14/14 Checking commit 93adc6369ead (iotests: add tests for blockdev-amend)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100755
total: 0 errors, 1 warnings, 591 lines checked
Patch 14/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200518122041.10694-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200518122041.10694-1-mlevitsk@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200518122041.10694-1-mlevitsk@redhat.com
Subject: [PATCH v7 00/14] LUKS: encryption slot management using amend interface
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
a170aa5 iotests: add tests for blockdev-amend
ccf4ad8 block/qcow2: implement blockdev-amend
f669823 block/crypto: implement blockdev-amend
d14e032 block/core: add generic infrastructure for x-blockdev-amend qmp command
cd60243 iotests: qemu-img tests for luks key management
669924e iotests: filter few more luks specific create options
9b2a290 block/qcow2: extend qemu-img amend interface with crypto options
fe70cac block/crypto: implement the encryption key management
05e4264 block/crypto: rename two functions
50ec885 block/amend: refactor qcow2 amend options
a824f7d block/amend: separate amend and create options for qemu-img
cee3ceb block/amend: add 'force' option
0d15e95 qcrypto/luks: implement encryption key management
fc17979 qcrypto/core: add generic infrastructure for crypto options amendment
=== OUTPUT BEGIN ===
1/14 Checking commit fc17979ac832 (qcrypto/core: add generic infrastructure for crypto options amendment)
2/14 Checking commit 0d15e95c3354 (qcrypto/luks: implement encryption key management)
3/14 Checking commit cee3cebe91c6 (block/amend: add 'force' option)
4/14 Checking commit a824f7d24eec (block/amend: separate amend and create options for qemu-img)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#32: FILE: block/qcow2.c:5637:
+#define QCOW_COMMON_OPTIONS \
+ { \
+ .name = BLOCK_OPT_SIZE, \
+ .type = QEMU_OPT_SIZE, \
+ .help = "Virtual disk size" \
+ }, \
+ { \
+ .name = BLOCK_OPT_COMPAT_LEVEL, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Compatibility level (v2 [0.10] or v3 [1.1])" \
+ }, \
+ { \
+ .name = BLOCK_OPT_BACKING_FILE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "File name of a base image" \
+ }, \
+ { \
+ .name = BLOCK_OPT_BACKING_FMT, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Image format of the base image" \
+ }, \
+ { \
+ .name = BLOCK_OPT_DATA_FILE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "File name of an external data file" \
+ }, \
+ { \
+ .name = BLOCK_OPT_DATA_FILE_RAW, \
+ .type = QEMU_OPT_BOOL, \
+ .help = "The external data file must stay valid " \
+ "as a raw image" \
+ }, \
+ { \
+ .name = BLOCK_OPT_ENCRYPT, \
+ .type = QEMU_OPT_BOOL, \
+ .help = "Encrypt the image with format 'aes'. (Deprecated " \
+ "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)", \
+ }, \
+ { \
+ .name = BLOCK_OPT_ENCRYPT_FORMAT, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Encrypt the image, format choices: 'aes', 'luks'", \
+ }, \
+ BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.", \
+ "ID of secret providing qcow AES key or LUKS passphrase"), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."), \
+ BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."), \
+ { \
+ .name = BLOCK_OPT_CLUSTER_SIZE, \
+ .type = QEMU_OPT_SIZE, \
+ .help = "qcow2 cluster size", \
+ .def_value_str = stringify(DEFAULT_CLUSTER_SIZE) \
+ }, \
+ { \
+ .name = BLOCK_OPT_PREALLOC, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Preallocation mode (allowed values: off, " \
+ "metadata, falloc, full)" \
+ }, \
+ { \
+ .name = BLOCK_OPT_LAZY_REFCOUNTS, \
+ .type = QEMU_OPT_BOOL, \
+ .help = "Postpone refcount updates", \
+ .def_value_str = "off" \
+ }, \
+ { \
+ .name = BLOCK_OPT_REFCOUNT_BITS, \
+ .type = QEMU_OPT_NUMBER, \
+ .help = "Width of a reference count entry in bits", \
+ .def_value_str = "16" \
+ }, \
+ { \
+ .name = BLOCK_OPT_COMPRESSION_TYPE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "Compression method used for image cluster " \
+ "compression", \
+ .def_value_str = "zlib" \
+ }
total: 1 errors, 0 warnings, 244 lines checked
Patch 4/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/14 Checking commit 50ec88577410 (block/amend: refactor qcow2 amend options)
6/14 Checking commit 05e42645cec3 (block/crypto: rename two functions)
7/14 Checking commit fe70cacc684d (block/crypto: implement the encryption key management)
8/14 Checking commit 9b2a2904a525 (block/qcow2: extend qemu-img amend interface with crypto options)
9/14 Checking commit 669924e2c486 (iotests: filter few more luks specific create options)
10/14 Checking commit cd602439c6ed (iotests: qemu-img tests for luks key management)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100755
total: 0 errors, 1 warnings, 431 lines checked
Patch 10/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/14 Checking commit d14e03201d9d (block/core: add generic infrastructure for x-blockdev-amend qmp command)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31:
new file mode 100644
total: 0 errors, 1 warnings, 221 lines checked
Patch 11/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/14 Checking commit f66982365f52 (block/crypto: implement blockdev-amend)
13/14 Checking commit ccf4ad8eb24a (block/qcow2: implement blockdev-amend)
14/14 Checking commit a170aa5b9080 (iotests: add tests for blockdev-amend)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100755
total: 0 errors, 1 warnings, 591 lines checked
Patch 14/14 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200518122041.10694-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Mon, May 18, 2020 at 03:20:27PM +0300, Maxim Levitsky wrote: > Hi! > Here is the updated series of my patches, incorporating all the feedback I received. > > This implements the API interface that we agreed upon except that I merged the > LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise > I need nested unions which are not supported currently by QAPI parser. > This didn't change the API and thus once support for nested unions is there, > it can always be implemented in backward compatible way. > > I hope that this series will finally be considered for merging, since I am somewhat running > out of time to finish this task. > > Patches are strictly divided by topic to 3 groups, and each group depends on former groups. > > * Patches 1,2 implement qcrypto generic amend interface, including definition > of structs used in crypto.json and implement this in luks crypto driver > Nothing is exposed to the user at this stage > > * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management > for luks and for qcow2, and add a bunch of iotests to cover that. > > * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it > to luks and qcow2 driver to implement qmp based encryption slot management also using > the code from patches 1,2, and also add a bunch of iotests to cover this. > > Tested with -raw,-qcow2,-nbd and -luks iotests and 'make check' > > Changes from V4: > * Addresed feedback on patch 2 from Daniel (thanks!) > * Gave real numbers to the tests > * Updated the copyright in amend.c to RedHat > * Rebased and adjusted the python iotests to latest changes in iotest infrastructure > > Changes from V5: > * Updated all QMP docs to state that all added QMP features are since 5.1 > * Rebased to latest master > > Changes from V6: > * Addressed most of the review feedback from Max Reitz > * Rebased on latest qemu master It needs another rebase as there's some conflicts now. In any case, all the patches have my R-B already, and I don't have any further comments to add. Regards, Daniel
On 18.05.20 14:20, Maxim Levitsky wrote: > Hi! > Here is the updated series of my patches, incorporating all the feedback I received. You asked me on IRC what to do to get this series to move forward; considering I don’t think there were objections from anyone but me on v6, there are no further (substantial) objections from anyone on v7, and I gave all feedback I had on v6, I don’t think there’s much you can do right now. (Sorry for the delay, but, well, I was on PTO as you know.) As usual, I’ll try to get around to see whether I can rebase your series myself (because Dan hinted at some conflicts), and whether I feel like my comments were appropriately addressed (which I have little doubt about). That’s the plan. Note from a couple minutes later: From what I can see, the rebase conflicts don’t look too wild, but I don’t feel quite comfortable addressing all of them. There’s a functional one I could address in qemu-img.c (patch 3), where we need to bump OPTION_FORCE from 269 to 276. I could do that, but that’s not the only one, unfortunately. Patch 7 needs a bit more extensive modification: First, we need %s/bdrv_filter_default_perms/bdrv_default_perms/. Second, this will still not compile, because the .bdrv_child_perm interface has changed. BdrvChildRole is now an integer, so we also need s/const BdrvChildRole \*/BdrvChildRole /. (That gives us the nice side effect of being able to align the second line of the bdrv_default_perms() parameters (called from block_crypto_child_perms()) on the opening parenthesis.) Third, it becomes really interesting. With these changes, it would be wrong, because bdrv_default_perms() will then not use the permissions for a filter but for an image file with metadata – because that’s what the LUKS file child is. But that’s actually a bug that’s already there (and that I introduced). It makese sense to fix it in your series here, because to fix it we need a dedicated block_crypto_child_perms() function anyway. So, well. Do we want to cheat? Just let block_crypto_child_perms() call bdrv_default_perms() with role=BDRV_CHILD_FILTERED? That would be the previous behavior, but it would also be bad cheating. The comment that exists (before patch 7) above bdrv_crypto_luks.bdrv_child_perm says that we just want to allow share-rw=on, and we could achieve that simply by force-sharing the WRITE permission after invoking bdrv_default_perms() with the actual role (which is BDRV_CHILD_IMAGE). But what about the other stuff that sets bdrv_default_perms_for_storage() apart from bdrv_filter_default_perms()? I.e., force-taking WRITE and RESIZE permissions if the file is writable; force-taking the CONSISTENT_READ permission because we need the metadata; and unsharing the RESIZE permission? I think the best thing to do would be to call bdrv_default_perms() with the @role passed to block_crypto_child_perms() (i.e., BDRV_CHILD_IMAGE), and then relaxing the permissions, that is to share the WRITE and RESIZE persmissions, and to perhaps restore the WRITE and RESIZE permissions from what’s given to block_crypto_child_perms() (i.e., *nperm &= ~(WRITE | RESIZE); *nperm |= perm & (WRITE | RESIZE);), because LUKS doesn’t need them unless the image may actually be written. (OTOH, force-taking CONSISTENT_READ seems entirely reasonable.) Max
On Tue, 2020-06-02 at 18:29 +0200, Max Reitz wrote: > On 18.05.20 14:20, Maxim Levitsky wrote: > > Hi! > > Here is the updated series of my patches, incorporating all the feedback I received. > > You asked me on IRC what to do to get this series to move forward; > considering I don’t think there were objections from anyone but me on > v6, there are no further (substantial) objections from anyone on v7, and > I gave all feedback I had on v6, I don’t think there’s much you can do > right now. (Sorry for the delay, but, well, I was on PTO as you know.) > > As usual, I’ll try to get around to see whether I can rebase your series > myself (because Dan hinted at some conflicts), and whether I feel like > my comments were appropriately addressed (which I have little doubt > about). That’s the plan. Sorry in advance if I missed any feedback from you. That can happen, I do make mistakes like anybody of us. > > Note from a couple minutes later: From what I can see, the rebase > conflicts don’t look too wild, but I don’t feel quite comfortable > addressing all of them. There’s a functional one I could address in > qemu-img.c (patch 3), where we need to bump OPTION_FORCE from 269 to > 276. I could do that, but that’s not the only one, unfortunately. Yes, that conflict is easy. > > Patch 7 needs a bit more extensive modification: First, we need > %s/bdrv_filter_default_perms/bdrv_default_perms/. Second, this will > still not compile, because the .bdrv_child_perm interface has changed. > BdrvChildRole is now an integer, so we also need > s/const BdrvChildRole \*/BdrvChildRole /. > (That gives us the nice side effect of being able to align the second > line of the bdrv_default_perms() parameters (called from > block_crypto_child_perms()) on the opening parenthesis.) I did this. > > Third, it becomes really interesting. With these changes, it would be > wrong, because bdrv_default_perms() will then not use the permissions > for a filter but for an image file with metadata – because that’s what > the LUKS file child is. > > But that’s actually a bug that’s already there (and that I introduced). > It makese sense to fix it in your series here, because to fix it we > need a dedicated block_crypto_child_perms() function anyway. > > So, well. Do we want to cheat? Just let block_crypto_child_perms() > call bdrv_default_perms() with role=BDRV_CHILD_FILTERED? That would be > the previous behavior, but it would also be bad cheating. > > The comment that exists (before patch 7) above > bdrv_crypto_luks.bdrv_child_perm says that we just want to allow > share-rw=on, and we could achieve that simply by force-sharing the WRITE > permission after invoking bdrv_default_perms() with the actual role > (which is BDRV_CHILD_IMAGE). > > But what about the other stuff that sets > bdrv_default_perms_for_storage() apart from bdrv_filter_default_perms()? > I.e., force-taking WRITE and RESIZE permissions if the file is > writable; force-taking the CONSISTENT_READ permission because we need > the metadata; and unsharing the RESIZE permission? > > I think the best thing to do would be to call bdrv_default_perms() with > the @role passed to block_crypto_child_perms() (i.e., BDRV_CHILD_IMAGE), > and then relaxing the permissions, that is to share the WRITE and RESIZE > persmissions, and to perhaps restore the WRITE and RESIZE permissions > from what’s given to block_crypto_child_perms() (i.e., *nperm &= ~(WRITE > > RESIZE); *nperm |= perm & (WRITE | RESIZE);), because LUKS doesn’t > need them unless the image may actually be written. > > (OTOH, force-taking CONSISTENT_READ seems entirely reasonable.) Could we talk about this on IRC tomorrow? I don't fully understand you here, and I sort of understood this stuff back when I learned it, but that was long ago, and since we are talking about permissions here, plus an necessary hack that luks now have to make, I would like to understand exactly what I am doing. Other than that, I rebased the series, and all iotests (with the permission fix below) are passing. I'll send the rebased version once we finish the permission thing. Best regards, Maxim Levitsky PS: This code which is roughtly based on your suggestions, does pass my test write sharing test, but I don't have much confidence that it is correct: For example, I don't think that resize permission is needed to be touched, since resize of the 'luks' image shoudn't have any affect on encryption keys (since luks image is basically the underlying file minus the header, decrypted, and we don't really change the encryption key, but a encrypted keyslot which contains it). BlockCrypto *crypto = bs->opaque; bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, nshared); /* * Ask for consistent read permission so that if * someone else tries to open this image with this permission * neither will be able to edit encryption keys, since * we will unshare that permission while trying to * update the encryption keys */ if (!(bs->open_flags & BDRV_O_NO_IO)) { *nperm |= BLK_PERM_CONSISTENT_READ; *nshared |= (BLK_PERM_WRITE | BLK_PERM_RESIZE); *nperm &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); *nperm |= perm & (BLK_PERM_WRITE | BLK_PERM_RESIZE); } /* * This driver doesn't modify LUKS metadata except * when updating the encryption slots. * Thus unlike a proper format driver we don't ask for * shared write/read permission. However we need it * when we are updating the keys, to ensure that only we * have access to the device. * * Encryption update will set the crypto->updating_keys * during that period and refresh permissions * */ if (crypto->updating_keys) { /* need exclusive write access for header update */ *nperm |= BLK_PERM_WRITE; /* unshare read and write permission */ *nshared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE); }
Hi! Here is the updated series of my patches, incorporating all the feedback I received. This implements the API interface that we agreed upon except that I merged the LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise I need nested unions which are not supported currently by QAPI parser. This didn't change the API and thus once support for nested unions is there, it can always be implemented in backward compatible way. I hope that this series will finally be considered for merging, since I am somewhat running out of time to finish this task. Patches are strictly divided by topic to 3 groups, and each group depends on former groups. * Patches 1,2 implement qcrypto generic amend interface, including definition of structs used in crypto.json and implement this in luks crypto driver Nothing is exposed to the user at this stage * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management for luks and for qcow2, and add a bunch of iotests to cover that. * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it to luks and qcow2 driver to implement qmp based encryption slot management also using the code from patches 1,2, and also add a bunch of iotests to cover this. Tested with -raw,-qcow2,-nbd and -luks iotests and 'make check' Changes from V4: * Addresed feedback on patch 2 from Daniel (thanks!) * Gave real numbers to the tests * Updated the copyright in amend.c to RedHat * Rebased and adjusted the python iotests to latest changes in iotest infrastructure Changes from V5: * Updated all QMP docs to state that all added QMP features are since 5.1 * Rebased to latest master Changes from V6: * Addressed most of the review feedback from Max Reitz * Rebased on latest qemu master Best regards, Maxim Levitsky clone of "luks-keymgmnt-v2" Maxim Levitsky (14): qcrypto/core: add generic infrastructure for crypto options amendment qcrypto/luks: implement encryption key management block/amend: add 'force' option block/amend: separate amend and create options for qemu-img block/amend: refactor qcow2 amend options block/crypto: rename two functions block/crypto: implement the encryption key management block/qcow2: extend qemu-img amend interface with crypto options iotests: filter few more luks specific create options iotests: qemu-img tests for luks key management block/core: add generic infrastructure for x-blockdev-amend qmp command block/crypto: implement blockdev-amend block/qcow2: implement blockdev-amend iotests: add tests for blockdev-amend block.c | 4 +- block/Makefile.objs | 2 +- block/amend.c | 113 +++++++++ block/crypto.c | 203 +++++++++++++-- block/crypto.h | 37 +++ block/qcow2.c | 332 +++++++++++++----------- crypto/block-luks.c | 416 ++++++++++++++++++++++++++++++- crypto/block.c | 29 +++ crypto/blockpriv.h | 8 + docs/tools/qemu-img.rst | 5 +- include/block/block.h | 1 + include/block/block_int.h | 24 +- include/crypto/block.h | 22 ++ qapi/block-core.json | 68 +++++ qapi/crypto.json | 73 +++++- qapi/job.json | 4 +- qemu-img-cmds.hx | 4 +- qemu-img.c | 44 +++- tests/qemu-iotests/049.out | 102 ++++---- tests/qemu-iotests/061.out | 12 +- tests/qemu-iotests/079.out | 18 +- tests/qemu-iotests/082.out | 185 ++++---------- tests/qemu-iotests/085.out | 38 +-- tests/qemu-iotests/087.out | 6 +- tests/qemu-iotests/115.out | 2 +- tests/qemu-iotests/121.out | 4 +- tests/qemu-iotests/125.out | 192 +++++++------- tests/qemu-iotests/134.out | 2 +- tests/qemu-iotests/144.out | 4 +- tests/qemu-iotests/158.out | 4 +- tests/qemu-iotests/182.out | 2 +- tests/qemu-iotests/185.out | 8 +- tests/qemu-iotests/188.out | 2 +- tests/qemu-iotests/189.out | 4 +- tests/qemu-iotests/198.out | 4 +- tests/qemu-iotests/243.out | 16 +- tests/qemu-iotests/250.out | 2 +- tests/qemu-iotests/255.out | 8 +- tests/qemu-iotests/259.out | 2 +- tests/qemu-iotests/263.out | 4 +- tests/qemu-iotests/274.out | 46 ++-- tests/qemu-iotests/280.out | 2 +- tests/qemu-iotests/284.out | 6 +- tests/qemu-iotests/293 | 207 +++++++++++++++ tests/qemu-iotests/293.out | 99 ++++++++ tests/qemu-iotests/294 | 90 +++++++ tests/qemu-iotests/294.out | 30 +++ tests/qemu-iotests/295 | 279 +++++++++++++++++++++ tests/qemu-iotests/295.out | 40 +++ tests/qemu-iotests/296 | 234 +++++++++++++++++ tests/qemu-iotests/296.out | 33 +++ tests/qemu-iotests/common.filter | 6 +- tests/qemu-iotests/group | 4 + 53 files changed, 2521 insertions(+), 565 deletions(-) create mode 100644 block/amend.c create mode 100755 tests/qemu-iotests/293 create mode 100644 tests/qemu-iotests/293.out create mode 100755 tests/qemu-iotests/294 create mode 100644 tests/qemu-iotests/294.out create mode 100755 tests/qemu-iotests/295 create mode 100644 tests/qemu-iotests/295.out create mode 100755 tests/qemu-iotests/296 create mode 100644 tests/qemu-iotests/296.out