Re: [Qemu-devel] [PATCH 12/34] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 10 Aug 2012 10:42:23 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Thu, 02 Aug 2012 13:53:08 +0200
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Luiz Capitulino <address@hidden> 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 <address@hidden>
>> >> > ---
>> >> >  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.

Yup.

>> >> 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?

The one I marked "Regression :)"?  Yes, I think it would fix that one.