Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Feb 28, 2024 at 12:21:02PM +0100, Kevin Wolf wrote:
> Am 28.02.2024 um 11:23 hat Daniel P. Berrangé geschrieben:
> > On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> > > Am 28.02.2024 um 07:43 hat Markus Armbruster geschrieben:
> > > > Yong Huang <yong.huang@smartx.com> writes:
> > > > 
> > > > > On Wed, Feb 21, 2024 at 4:26 PM Markus Armbruster <armbru@redhat.com> wrote:
> > > > >
> > > > >> Yong Huang <yong.huang@smartx.com> writes:
> > > > >>
> > > > >> > On Wed, Feb 21, 2024 at 2:43 PM Markus Armbruster <armbru@redhat.com>
> > > > >> wrote:
> > > > >> >
> > > > >> >> Hyman Huang <yong.huang@smartx.com> writes:
> > > > >> >>
> > > > >> >> > Add comment in detail for commit 433957bb7f (qapi:
> > > > >> >> > Make parameter 'file' optional for
> > > > >> >> > BlockdevCreateOptionsLUKS).
> > > > >> >> >
> > > > >> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > > > >> >> > ---
> > > > >> >> >  qapi/block-core.json | 20 +++++++++++++++++++-
> > > > >> >> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > > > >> >> >
> > > > >> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > >> >> > index ab5a93a966..42b0840d43 100644
> > > > >> >> > --- a/qapi/block-core.json
> > > > >> >> > +++ b/qapi/block-core.json
> > > > >> >> > @@ -4973,7 +4973,25 @@
> > > > >> >> >  ##
> > > > >> >> >  # @BlockdevCreateOptionsLUKS:
> > > > >> >> >  #
> > > > >> >> > -# Driver specific image creation options for LUKS.
> > > > >> >> > +# Driver specific image creation options for LUKS. Note that
> > > > >> >> > +# @file is required if @preallocation is specified and equals
> > > > >> >> > +# PREALLOC_MODE_ON. The following three scenarios determine how
> > > > >> >> > +# creation logic behaves when @preallocation is either equal to
> > > > >> >> > +# PREALLOC_MODE_OFF or is not given:
> > > > >> >> > +#
> > > > >> >> > +#  1) When @file is given only, format the block device referenced
> > > > >> >> > +#     by @file as the LUKS specification and trunk it to the @size.
> > > > >> >>
> > > > >> >> Do you mean "truncate it to @size"?
> > > > >> >>
> > > > >> > Yes, :( sorry for the spelling mistake.
> > > > >>
> > > > >> Writing good documentation in a second language is *hard*.  All we can
> > > > >> reasonably expect from contributors to try their best.  And then we
> > > > >> improve the text together in review.  Just like we do for code :)
> > > > >>
> > > > >> >> > +#     In this case, the @size should reflect amount of space made
> > > > >> >> > +#     available to the guest, so the trunk size must take account
> > > > >> >> > +#     of that which will be used by the crypto header.
> > > > >> >> > +#
> > > > >> >> > +#  2) When @header is given only, just format the block device
> > > > >> >> > +#     referenced by @header as the LUKS specification.
> > > > >> >> > +#
> > > > >> >> > +#  3) When both @file and @header are given, block device
> > > > >> >> > +#     referenced by @file should be trunked to @size, and block
> > > > >> >> > +#     device referenced by @header should be formatted as the LUKS
> > > > >> >> > +#     specification.
> > > > >> >> >  #
> > > > >> >> >  # @file: Node to create the image format on, mandatory except when
> > > > >> >> >  #        'preallocation' is not requested
> > > > >> >>
> > > > >> >> Let's see whether I understand.
> > > > >> >>
> > > > >> >> blockdev-create with "driver": "luks" can work in three different ways:
> > > > >> >>
> > > > >> >> 1. Create an image with a LUKS header
> > > > >> >>
> > > > >> >> 2. Create just a detached LUKS header
> > > > >> >>
> > > > >> >> 3. Create an image and a detached LUKS header
> > > > >> >>
> > > > >> >> Correct?
> > > > >> >>
> > > > >> >
> > > > >> > Yes
> > > > >> >
> > > > >> >
> > > > >> >> @file and @header are BlockdevRef, which means they refer to existing
> > > > >> >> images with arbitrary driver.  Could be "file", "qcow2", or anything.
> > > > >> >>
> > > > >> >> Correct?
> > > > >> >>
> > > > >> > Yes
> > > > >> >
> > > > >> >
> > > > >> >>
> > > > >> >> To get 1., specify @file, but not @header.
> > > > >> >>
> > > > >> >> To get 2., specify @header, but not @file.
> > > > >> >>
> > > > >> >> To get 3., specify both.
> > > > >> >>
> > > > >> >> Specifying neither is an error.
> > > > >> >>
> > > > >> >> Correct?
> > > > >> >>
> > > > >> >
> > > > >> > Yes
> > > > >> >
> > > > >> >
> > > > >> >> In any case, @size is the logical size of the image (how much data it
> > > > >> >> can hold).
> > > > >> >>
> > > > >> >
> > > > >> > Yes
> > > > >> >
> > > > >> >
> > > > >> >>
> > > > >> >> With 1., the actual image size is a bit larger due to the LUKS header.
> > > > >> >> The @file image is resized to that size: if it's shorter, it's grown, if
> > > > >> >> it's longer, it's truncated.
> > > > >> >>
> > > > >> >
> > > > >> > Yes
> > > > >> >
> > > > >> >
> > > > >> >> With 2., @size is merely recorded in the detached LUKS header.
> > > > >> >>
> > > > >> >
> > > > >> > In LUKS1 specification, payload data size is not contained in the header,
> > > > >> > so in this case, @size is not recorded in the detached LUKS header.
> > > > >> > The creation logic just does the LUKS header formatting only.
> > > > >>
> > > > >> Is @size unused then?
> > > > >>
> > > > >
> > > > > IIUC, yes. Creation logic will ignore the @size. See the following code
> > > > > in function block_crypto_co_create_luks:
> > > > >
> > > > >     if (luks_opts->header) {
> > > > >         /* LUKS volume with detached header */
> > > > >         hdr_bs = bdrv_co_open_blockdev_ref(luks_opts->header, errp);
> > > > >         if (hdr_bs == NULL) {
> > > > >             return -EIO;
> > > > >         }
> > > > >
> > > > >         cflags |= QCRYPTO_BLOCK_CREATE_DETACHED;
> > > > >
> > > > >         /* Format the LUKS header node, here just ignore the size
> > > > >           * and passed zero to block_crypto_co_create_generic */
> > > > >         ret = block_crypto_co_create_generic(hdr_bs, 0, &create_opts,
> > > > >                                              PREALLOC_MODE_OFF, cflags, errp);
> > > > >         if (ret < 0) {
> > > > >             goto fail;
> > > > >         }
> > > > >
> > > > >         /* Format the LUKS payload node */
> > > > >         if (luks_opts->file) {
> > > > >             ret = block_crypto_co_format_luks_payload(luks_opts, errp);
> > > > >             if (ret < 0) {
> > > > >                 goto fail;
> > > > >             }
> > > > >         }
> > > > 
> > > > @size is a required argument, but silently ignored when @header is
> > > > present and @file is absent (2. Create just a detached LUKS header).
> > > > Feels awkward.
> > > > 
> > > > Should @size be optional, absent when and only when @header is present
> > > > and @file is absent?
> > > > 
> > > > Kevin or Hanna, got an opinion?
> > > 
> > > What is the use case for creating a header without a corresponding
> > > image?
> > 
> > eg you're creating a detached LUKS header that will be used in
> > combination with a block device, or an NBD export, or some other
> > storage that was pre-created in some manner, and onto which you
> > now want to store LUKS encrypted data.
> 
> Why don't you pass a host_device/nbd/... node as @file then?

It is possible of course, but it serves no functional purpose. The
act of formatting the LUKS volume would never touch that data payload
storage, unless preallocation is requested. IOW, aside from QEMU
opening the @file (which proves it exists) it would be ignored.

With 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 :|