| Message ID | 20190912223028.18496-10-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC crypto/luks: encryption key managment using amend interface | expand |
On 13.09.19 00:30, Maxim Levitsky wrote: > Currently only for changing crypto parameters Yep, that elegantly avoids most of the problems we’d have otherwise. :-) > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > block/qcow2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 6 ++-- > 2 files changed, 75 insertions(+), 2 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 26f83aeb44..c8847ec6e2 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -3079,6 +3079,18 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2); > qcow2_opts = &create_options->u.qcow2; > > + if (!qcow2_opts->has_size) { > + error_setg(errp, "Size is manadatory for image creation"); > + return -EINVAL; > + > + } > + > + if (!qcow2_opts->has_file) { > + error_setg(errp, "'file' is manadatory for image creation"); > + return -EINVAL; > + > + } > + > bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp); > if (bs == NULL) { > return -EIO; > @@ -5111,6 +5123,64 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, > return 0; > } > > + > +static int coroutine_fn qcow2_co_amend(BlockDriverState *bs, > + BlockdevCreateOptions *opts, > + bool force, > + Error **errp) > +{ > + BlockdevCreateOptionsQcow2 *qopts = &opts->u.qcow2; > + BDRVQcow2State *s = bs->opaque; > + int ret; > + > + /* > + * This is ugly as hell, in later versions of this patch > + * something has to be done about this Well, at least the language of the comment needs adjustment. :-) > + */ > + if (qopts->has_file || qopts->has_size || qopts->has_data_file || > + qopts->has_data_file_raw || qopts->has_version || > + qopts->has_backing_file || qopts->has_backing_fmt || > + qopts->has_cluster_size || qopts->has_preallocation || > + qopts->has_lazy_refcounts || qopts->has_refcount_bits) { > + > + error_setg(errp, > + "Only LUKS encryption options can be amended for qcow2 with blockdev-amend"); > + return -EOPNOTSUPP; > + > + } > + > + if (qopts->has_encrypt) { > + if (!s->crypto) { > + error_setg(errp, "QCOW2 image is not encrypted, can't amend"); > + return -EOPNOTSUPP; > + } > + > + if (qopts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) { > + error_setg(errp, > + "Amend can't be used to change the qcow2 encryption format"); > + return -EOPNOTSUPP; > + } > + > + if (s->crypt_method_header != QCOW_CRYPT_LUKS) { > + error_setg(errp, > + "Only LUKS encryption options can be amended for qcow2 with blockdev-amend"); > + return -EOPNOTSUPP; > + } > + > + ret = qcrypto_block_amend_options(s->crypto, > + qcow2_crypto_hdr_read_func, > + qcow2_crypto_hdr_write_func, > + bs, > + qopts->encrypt, > + force, > + errp); > + if (ret) { > + return ret; > + } > + } > + return 0; I suppose we need to do the same permission stuff as for LUKS, though; i.e., unshare CONSISTENT_READ while this operation is ongoing. > +} > + > /* > * If offset or size are negative, respectively, they will not be included in > * the BLOCK_IMAGE_CORRUPTED event emitted. > @@ -5303,6 +5373,7 @@ BlockDriver bdrv_qcow2 = { > .mutable_opts = mutable_opts, > .bdrv_co_check = qcow2_co_check, > .bdrv_amend_options = qcow2_amend_options, > + .bdrv_co_amend = qcow2_co_amend, > > .bdrv_detach_aio_context = qcow2_detach_aio_context, > .bdrv_attach_aio_context = qcow2_attach_aio_context, > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 4a6db98938..0eb4e45168 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4294,6 +4294,7 @@ > # Driver specific image creation options for qcow2. > # > # @file Node to create the image format on > +# Mandatory for create > # @data-file Node to use as an external data file in which all guest > # data is stored so that only metadata remains in the qcow2 > # file (since: 4.0) > @@ -4301,6 +4302,7 @@ > # standalone (read-only) raw image without looking at qcow2 > # metadata (default: false; since: 4.0) > # @size Size of the virtual disk in bytes > +# Mandatory for create > # @version Compatibility level (default: v3) > # @backing-file File name of the backing file if a backing file > # should be used > @@ -4315,10 +4317,10 @@ > # Since: 2.12 > ## > { 'struct': 'BlockdevCreateOptionsQcow2', > - 'data': { 'file': 'BlockdevRef', > + 'data': { '*file': 'BlockdevRef', > '*data-file': 'BlockdevRef', > '*data-file-raw': 'bool', > - 'size': 'size', > + '*size': 'size', > '*version': 'BlockdevQcow2Version', > '*backing-file': 'str', > '*backing-fmt': 'BlockdevDriver', > Making these fields optional makes me wonder whether it actually make sense to have both create and amend share a single QAPI structure. Wouldn’t it better to have a separate amend structure that then actually reflects what we support? (This would also solve the problem of how to express in the code what is and what isn’t supported through blockdev-amend.) Max
Max Reitz <mreitz@redhat.com> writes: > On 13.09.19 00:30, Maxim Levitsky wrote: >> Currently only for changing crypto parameters > > Yep, that elegantly avoids most of the problems we’d have otherwise. :-) > >> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> [...] >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 4a6db98938..0eb4e45168 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -4294,6 +4294,7 @@ >> # Driver specific image creation options for qcow2. >> # >> # @file Node to create the image format on >> +# Mandatory for create >> # @data-file Node to use as an external data file in which all guest >> # data is stored so that only metadata remains in the qcow2 >> # file (since: 4.0) >> @@ -4301,6 +4302,7 @@ >> # standalone (read-only) raw image without looking at qcow2 >> # metadata (default: false; since: 4.0) >> # @size Size of the virtual disk in bytes >> +# Mandatory for create >> # @version Compatibility level (default: v3) >> # @backing-file File name of the backing file if a backing file >> # should be used >> @@ -4315,10 +4317,10 @@ >> # Since: 2.12 >> ## >> { 'struct': 'BlockdevCreateOptionsQcow2', >> - 'data': { 'file': 'BlockdevRef', >> + 'data': { '*file': 'BlockdevRef', >> '*data-file': 'BlockdevRef', >> '*data-file-raw': 'bool', >> - 'size': 'size', >> + '*size': 'size', >> '*version': 'BlockdevQcow2Version', >> '*backing-file': 'str', >> '*backing-fmt': 'BlockdevDriver', >> My comments to the previous patch apply. > Making these fields optional makes me wonder whether it actually make > sense to have both create and amend share a single QAPI structure. > Wouldn’t it better to have a separate amend structure that then actually > reflects what we support? (This would also solve the problem of how to > express in the code what is and what isn’t supported through > blockdev-amend.) Good point. As is, the schema is rather confusing, at least to me. We reduce or avoid the confusion in documentation or in code. I'd prefer code unless it leads to too much duplication. "Too much" is of course subjective. Maxim, would you mind exploring it for us?
On Mon, 2019-10-07 at 10:04 +0200, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > > > On 13.09.19 00:30, Maxim Levitsky wrote: > > > Currently only for changing crypto parameters > > > > Yep, that elegantly avoids most of the problems we’d have otherwise. :-) > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > [...] > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > index 4a6db98938..0eb4e45168 100644 > > > --- a/qapi/block-core.json > > > +++ b/qapi/block-core.json > > > @@ -4294,6 +4294,7 @@ > > > # Driver specific image creation options for qcow2. > > > # > > > # @file Node to create the image format on > > > +# Mandatory for create > > > # @data-file Node to use as an external data file in which all guest > > > # data is stored so that only metadata remains in the qcow2 > > > # file (since: 4.0) > > > @@ -4301,6 +4302,7 @@ > > > # standalone (read-only) raw image without looking at qcow2 > > > # metadata (default: false; since: 4.0) > > > # @size Size of the virtual disk in bytes > > > +# Mandatory for create > > > # @version Compatibility level (default: v3) > > > # @backing-file File name of the backing file if a backing file > > > # should be used > > > @@ -4315,10 +4317,10 @@ > > > # Since: 2.12 > > > ## > > > { 'struct': 'BlockdevCreateOptionsQcow2', > > > - 'data': { 'file': 'BlockdevRef', > > > + 'data': { '*file': 'BlockdevRef', > > > '*data-file': 'BlockdevRef', > > > '*data-file-raw': 'bool', > > > - 'size': 'size', > > > + '*size': 'size', > > > '*version': 'BlockdevQcow2Version', > > > '*backing-file': 'str', > > > '*backing-fmt': 'BlockdevDriver', > > > > > My comments to the previous patch apply. > > > Making these fields optional makes me wonder whether it actually make > > sense to have both create and amend share a single QAPI structure. > > Wouldn’t it better to have a separate amend structure that then actually > > reflects what we support? (This would also solve the problem of how to > > express in the code what is and what isn’t supported through > > blockdev-amend.) > > Good point. As is, the schema is rather confusing, at least to me. We > reduce or avoid the confusion in documentation or in code. I'd prefer > code unless it leads to too much duplication. "Too much" is of course > subjective. Maxim, would you mind exploring it for us? Nothing against having a separate amend structure, I actually prefer this, and I don't think it will complicate the code. Best regards, Maxim Levitsky
On Fri, 2019-10-04 at 21:03 +0200, Max Reitz wrote: > On 13.09.19 00:30, Maxim Levitsky wrote: > > Currently only for changing crypto parameters > > Yep, that elegantly avoids most of the problems we’d have otherwise. :-) > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > block/qcow2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++ > > qapi/block-core.json | 6 ++-- > > 2 files changed, 75 insertions(+), 2 deletions(-) > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > index 26f83aeb44..c8847ec6e2 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -3079,6 +3079,18 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > > assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2); > > qcow2_opts = &create_options->u.qcow2; > > > > + if (!qcow2_opts->has_size) { > > + error_setg(errp, "Size is manadatory for image creation"); > > + return -EINVAL; > > + > > + } > > + > > + if (!qcow2_opts->has_file) { > > + error_setg(errp, "'file' is manadatory for image creation"); > > + return -EINVAL; > > + > > + } > > + > > bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp); > > if (bs == NULL) { > > return -EIO; > > @@ -5111,6 +5123,64 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, > > return 0; > > } > > > > + > > +static int coroutine_fn qcow2_co_amend(BlockDriverState *bs, > > + BlockdevCreateOptions *opts, > > + bool force, > > + Error **errp) > > +{ > > + BlockdevCreateOptionsQcow2 *qopts = &opts->u.qcow2; > > + BDRVQcow2State *s = bs->opaque; > > + int ret; > > + > > + /* > > + * This is ugly as hell, in later versions of this patch > > + * something has to be done about this > > Well, at least the language of the comment needs adjustment. :-) Thats for sure :-) BTW, if I opt for having a separate amend parameter struct, this will fix this problem as well, as all unsupported fields will just not be there. [..] Best regards, Maxim Levitsky
diff --git a/block/qcow2.c b/block/qcow2.c index 26f83aeb44..c8847ec6e2 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3079,6 +3079,18 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2); qcow2_opts = &create_options->u.qcow2; + if (!qcow2_opts->has_size) { + error_setg(errp, "Size is manadatory for image creation"); + return -EINVAL; + + } + + if (!qcow2_opts->has_file) { + error_setg(errp, "'file' is manadatory for image creation"); + return -EINVAL; + + } + bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp); if (bs == NULL) { return -EIO; @@ -5111,6 +5123,64 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, return 0; } + +static int coroutine_fn qcow2_co_amend(BlockDriverState *bs, + BlockdevCreateOptions *opts, + bool force, + Error **errp) +{ + BlockdevCreateOptionsQcow2 *qopts = &opts->u.qcow2; + BDRVQcow2State *s = bs->opaque; + int ret; + + /* + * This is ugly as hell, in later versions of this patch + * something has to be done about this + */ + if (qopts->has_file || qopts->has_size || qopts->has_data_file || + qopts->has_data_file_raw || qopts->has_version || + qopts->has_backing_file || qopts->has_backing_fmt || + qopts->has_cluster_size || qopts->has_preallocation || + qopts->has_lazy_refcounts || qopts->has_refcount_bits) { + + error_setg(errp, + "Only LUKS encryption options can be amended for qcow2 with blockdev-amend"); + return -EOPNOTSUPP; + + } + + if (qopts->has_encrypt) { + if (!s->crypto) { + error_setg(errp, "QCOW2 image is not encrypted, can't amend"); + return -EOPNOTSUPP; + } + + if (qopts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) { + error_setg(errp, + "Amend can't be used to change the qcow2 encryption format"); + return -EOPNOTSUPP; + } + + if (s->crypt_method_header != QCOW_CRYPT_LUKS) { + error_setg(errp, + "Only LUKS encryption options can be amended for qcow2 with blockdev-amend"); + return -EOPNOTSUPP; + } + + ret = qcrypto_block_amend_options(s->crypto, + qcow2_crypto_hdr_read_func, + qcow2_crypto_hdr_write_func, + bs, + qopts->encrypt, + force, + errp); + if (ret) { + return ret; + } + } + return 0; +} + /* * If offset or size are negative, respectively, they will not be included in * the BLOCK_IMAGE_CORRUPTED event emitted. @@ -5303,6 +5373,7 @@ BlockDriver bdrv_qcow2 = { .mutable_opts = mutable_opts, .bdrv_co_check = qcow2_co_check, .bdrv_amend_options = qcow2_amend_options, + .bdrv_co_amend = qcow2_co_amend, .bdrv_detach_aio_context = qcow2_detach_aio_context, .bdrv_attach_aio_context = qcow2_attach_aio_context, diff --git a/qapi/block-core.json b/qapi/block-core.json index 4a6db98938..0eb4e45168 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4294,6 +4294,7 @@ # Driver specific image creation options for qcow2. # # @file Node to create the image format on +# Mandatory for create # @data-file Node to use as an external data file in which all guest # data is stored so that only metadata remains in the qcow2 # file (since: 4.0) @@ -4301,6 +4302,7 @@ # standalone (read-only) raw image without looking at qcow2 # metadata (default: false; since: 4.0) # @size Size of the virtual disk in bytes +# Mandatory for create # @version Compatibility level (default: v3) # @backing-file File name of the backing file if a backing file # should be used @@ -4315,10 +4317,10 @@ # Since: 2.12 ## { 'struct': 'BlockdevCreateOptionsQcow2', - 'data': { 'file': 'BlockdevRef', + 'data': { '*file': 'BlockdevRef', '*data-file': 'BlockdevRef', '*data-file-raw': 'bool', - 'size': 'size', + '*size': 'size', '*version': 'BlockdevQcow2Version', '*backing-file': 'str', '*backing-fmt': 'BlockdevDriver',
Currently only for changing crypto parameters Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- block/qcow2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 6 ++-- 2 files changed, 75 insertions(+), 2 deletions(-)