On Fri, 10 Aug 2012 10:42:23 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > On Thu, 02 Aug 2012 13:53:08 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
> >>
> >> > This commit changes hmp_cont() to loop through all block devices
> >> > and proactively set an encryption key for any encrypted device
> >> > without a valid one.
> >> >
> >> > This change is needed because QERR_DEVICE_ENCRYPTED is going to be
> >> > dropped by a future commit.
> >> >
> >> > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
> >> > ---
> >> > hmp.c | 43 +++++++++++++++++++++++++------------------
> >> > 1 file changed, 25 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/hmp.c b/hmp.c
> >> > index 6b72a64..1ebeb63 100644
> >> > --- a/hmp.c
> >> > +++ b/hmp.c
> >> > @@ -610,34 +610,41 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
> >> >
> >> > static void hmp_cont_cb(void *opaque, int err)
> >> > {
> >> > - Monitor *mon = opaque;
> >> > -
> >> > if (!err) {
> >> > - hmp_cont(mon, NULL);
> >> > + qmp_cont(NULL);
> >> > }
> >> > }
> >> >
> >> > -void hmp_cont(Monitor *mon, const QDict *qdict)
> >> > +static bool blockinfo_is_encrypted(const BlockInfo *bdev)
> >> > {
> >> > - Error *errp = NULL;
> >> > -
> >> > - qmp_cont(&errp);
> >> > - if (error_is_set(&errp)) {
> >> > - if (error_is_type(errp, QERR_DEVICE_ENCRYPTED)) {
> >> > - const char *device;
> >> > + return (bdev->inserted && bdev->inserted->encrypted);
> >> > +}
> >> >
> >> > - /* The device is encrypted. Ask the user for the password
> >> > - and retry */
> >> > +static bool blockinfo_key_is_set(const BlockInfo *bdev)
> >> > +{
> >> > + return (bdev->inserted && bdev->inserted->valid_encryption_key);
> >> > +}
> >> >
> >> > - device = error_get_field(errp, "device");
> >> > - assert(device != NULL);
> >> > +void hmp_cont(Monitor *mon, const QDict *qdict)
> >> > +{
> >> > + BlockInfoList *bdev_list, *bdev;
> >> > + Error *errp = NULL;
> >> >
> >> > - monitor_read_block_device_key(mon, device, hmp_cont_cb,
> >> > mon);
> >> > - error_free(errp);
> >> > - return;
> >> > + bdev_list = qmp_query_block(NULL);
> >> > + for (bdev = bdev_list; bdev; bdev = bdev->next) {
> >> > + if (blockinfo_is_encrypted(bdev->value) &&
> >> > + !blockinfo_key_is_set(bdev->value)) {
> >> > + monitor_read_block_device_key(mon, bdev->value->device,
> >> > + hmp_cont_cb, NULL);
> >> > + goto out;
> >> > }
> >> > - hmp_handle_error(mon, &errp);
> >> > }
> >> > +
> >> > + qmp_cont(&errp);
> >> > + hmp_handle_error(mon, &errp);
> >> > +
> >> > +out:
> >> > + qapi_free_BlockInfoList(bdev_list);
> >> > }
> >> >
> >> > void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
> >>
> >> Quote my previous analysis:
> >>
> >> Diff makes this change look worse than it is. Odd: M-x ediff gets it
> >> right. Anyway, here's how I think it works:
> >>
> >> Unchanged qmp_cont(): search the bdrv_states for the first encrypted one
> >> without a key. If found, set err argument to QERR_DEVICE_ENCRYPTED.
> >> Other errors unrelated to encrypted devices are also possible.
> >>
> >> hmp_cont() before: try qmp_cont(). If we get QERR_DEVICE_ENCRYPTED,
> >> extract the device from the error object, and prompt for its key, with a
> >> callback that retries hmp_cont() if the key was provided.
> >>
> >> After: search the bdrv_states for an encrypted one without a key. If
> >> there is none, qmp_cont(), no special error handling. If there is one,
> >> prompt for its key, with a callback that runs qmp_cont() if the key was
> >> provided.
> >>
> >> End quote.
> >>
> >> Two observations:
> >>
> >> 1. I don't understand how this works for multiple encrypted BDSs without
> >> keys. If there are any, hmp_cont() starts reading the first one's key,
> >> then returns. But the callback doesn't start reading the next one's
> >> key. Please explain.
> >
> > The callback calls qmp_cont(), which will fail. Then the user will enter
> > cont again, and the loop on BlockInfos will run again and the user will
> > be asked for the password of the next image.
> >
> > IOW, each time cont is issued by the user it will ask for the password
> > of a different device.
> >
> > That's the current behavior, and I believe it was also the behavior before
> > I converted cont to the qapi.
>
> Ugh. Clunky even for QEMU standards.
>
> cont gives no indication that the run state change didn't happen.
Agreed. We should have freedom to change cont's semantics on HMP if we
need/want to in order to fix this.
But that's out of the scope of this series, of course.
>
> >> 2. qmp_cont() uses bdrv_key_required() to test whether a BDS lacks a
> >> key. Your new hmp_cont() uses blockinfo_is_encrypted() &&
> >> !blockinfo_key_is_set(). Not obvious that the two are equivalent.
> >>
> >> I'm afraid they are not. bdrv_key_required() checks the backing image
> >> first:
> >>
> >> int bdrv_key_required(BlockDriverState *bs)
> >> {
> >> BlockDriverState *backing_hd = bs->backing_hd;
> >>
> >> if (backing_hd && backing_hd->encrypted && !backing_hd->valid_key)
> >> return 1;
> >> return (bs->encrypted && !bs->valid_key);
> >> }
> >>
> >> Your code doesn't:
> >>
> >> static bool blockinfo_is_encrypted(const BlockInfo *bdev)
> >> {
> >> return (bdev->inserted && bdev->inserted->encrypted);
> >> }
> >>
> >> static bool blockinfo_key_is_set(const BlockInfo *bdev)
> >> {
> >> return (bdev->inserted && bdev->inserted->valid_encryption_key);
> >> }
> >>
> >> BlockInfoList *qmp_query_block(Error **errp)
> >> {
> >> BlockInfoList *head = NULL, *cur_item = NULL;
> >> BlockDriverState *bs;
> >>
> >> QTAILQ_FOREACH(bs, &bdrv_states, list) {
> >> BlockInfoList *info = g_malloc0(sizeof(*info));
> >> [...]
> >> if (bs->drv) {
> >> info->value->has_inserted = true;
> >> info->value->inserted =
> >> g_malloc0(sizeof(*info->value->inserted));
> >> [...]
> >> info->value->inserted->encrypted = bs->encrypted;
> >> info->value->inserted->valid_encryption_key =
> >> bs->valid_key;
> >> [...]
> >>
> >> Are you sure this is correct?
> >
> > Is it actually possible for backing_hd to be false and valid_key to be true?
>
> Yes. Let's create an encrypted QCOW2 image without backing_file:
>
> $ qemu-img create -f qcow2 -o encryption,size=1G foo.qcow2
> Formatting 'foo.qcow2', fmt=qcow2 size=1073741824 encryption=on
> cluster_size=65536 lazy_refcounts=off
> $ qemu-img info foo.qcow2
> Disk image 'foo.qcow2' is encrypted.
> password:
> image: foo.qcow2
> file format: qcow2
> virtual size: 1.0G (1073741824 bytes)
> disk size: 136K
> encrypted: yes
> cluster_size: 65536
> $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0 -usb
> -monitor stdio -drive if=ide,file=foo.qcow2,id=foo
> QEMU 1.1.50 monitor - type 'help' for more information
> (qemu) info block
> foo: removable=0 io-status=ok file=foo.qcow2 ro=0 drv=qcow2 encrypted=1
> bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
> (qemu) c
> foo (foo.qcow2) is encrypted.
> Password:
>
> Now wrap an unencrypted one around it:
>
> $ qemu-img create -f qcow2 -o size=1G,backing_file=foo.qcow2 bar.qcow2
> Formatting 'bar.qcow2', fmt=qcow2 size=1073741824
> backing_file='foo.qcow2' encryption=off cluster_size=65536 lazy_refcounts=off
> $ qemu-img info bar.qcow2
> image: bar.qcow2
> file format: qcow2
> virtual size: 1.0G (1073741824 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: foo.qcow2
> $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0 -usb
> -monitor stdio -drive if=ide,file=bar.qcow2,id=foo
> QEMU 1.1.50 monitor - type 'help' for more information
> (qemu) c
> 'foo' (foo.qcow2) is encrypted
> (qemu)
>
> Regression :)
Hmm, right. I think this can also be reproduced by passing -snapshot
when using an encrypted image.
> >> I understand we require HMP code to go via QMP for everything, to keep
> >> HMP honest. This case shows a drawback: duplicated code, leading to
> >> inconsistencies.
> >
> > Keeping DeviceEncrypted would solve this.
>
> Another way is to replace valid-encryption-key by the predicate that's
> actually wanted: key-required.
Looks good, this would fix the bug above too, right?
