On Fri, Mar 18, 2016 at 01:09:35PM +0100, Kevin Wolf wrote: > Am 17.03.2016 um 18:51 hat Daniel P. Berrange geschrieben: > > Add a block driver that is capable of supporting any full disk > > encryption format. This utilizes the previously added block > > encryption code, and at this time supports the LUKS format. > > > > The driver code is capable of supporting any format supported > > by the QCryptoBlock module, so it registers one block driver > > for each format. This patch only registers the "luks" driver > > since the "qcow" driver is there only for back-compatibility > > with existing qcow built-in encryption. > > > > New LUKS compatible volumes can be formatted using qemu-img > > with defaults for all settings. > > > > $ qemu-img create --object secret,data=123456,id=sec0 \ > > -f luks -o key-secret=sec0 demo.luks 10G > > > > Alternatively the cryptographic settings can be explicitly > > set > > > > $ qemu-img create --object secret,data=123456,id=sec0 \ > > -f luks -o key-secret=sec0,cipher-alg=aes-256,\ > > cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha256 \ > > demo.luks 10G > > > > And query its size > > > > $ qemu-img info demo.img > > image: demo.img > > file format: luks > > virtual size: 10G (10737418240 bytes) > > disk size: 132K > > encrypted: yes > > > > Note that it was not necessary to provide the password > > when querying info for the volume. The password is only > > required when performing I/O on the volume > > > > All volumes created by this new 'luks' driver should be > > capable of being opened by the kernel dm-crypt driver. > > > > The only algorithms listed in the LUKS spec that are > > not currently supported by this impl are sha512 and > > ripemd160 hashes and cast6 cipher. > > > > A new I/O test is added which is used to validate the > > interoperability of the QEMU implementation of LUKS, > > with the dm-crypt/cryptsetup implementation. Due to > > the need to run cryptsetup as root, this test requires > > that the user have password-less sudo configured. > > > > The test is set to only run against the 'luks' format > > so needs to be manually invoked with > > > > cd tests/qemu-iotests > > ./check -luks 149 > > > > Reviewed-by: Eric Blake <ebl...@redhat.com> > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > > This is a huge patch. I'm not sure if it wouldn't be better to split off > the test. I also think that having some test cases that don't require > root privileges would be helpful, so maybe the test can be split in two > halves as well, one requiring passwordless sudo and the other without > special requirements.
I can certainly split off the test, however, I don't think it can be split into 2 as you describe, as both halves of the two require sudo privileges. So in one half we're creating images with dm-crypt and processing them with qemu, and in the other half we're creating images with qemu-img and processing them with dm-crypt. This test was specifically designed to validate dm-crypt interoperability, and my intention was that "pure" QEMU block driver testing of it would be covered by the various pre-existing I/O tests. The problem is that I need to update those existing tests to know how to pass the right args to qemu-img to setup passwords. If I can do that, then we'll have some good testing cover of the luks driver without needing sudo. So I'll look at converting a couple of key pre-existing tests to get such coverage, and put this test in its own patch. > > +static ssize_t block_crypto_init_func(QCryptoBlock *block, > > + size_t headerlen, > > + Error **errp, > > + void *opaque) > > +{ > > + struct BlockCryptoCreateData *data = opaque; > > + int ret; > > + > > + /* User provided size should reflect amount of space made > > + * available to the guest, so we must take account of that > > + * which will be used by the crypto header > > + */ > > + data->size += headerlen; > > + > > + qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, > > &error_abort); > > + ret = bdrv_create_file(data->filename, data->opts, errp); > > + if (ret < 0) { > > + return -1; > > + } > > + > > + ret = bdrv_open(&data->bs, data->filename, NULL, NULL, > > + BDRV_O_RDWR | BDRV_O_PROTOCOL, errp); > > We should probably use blk_open() here like the other image format > drivers do now, and preferably use blk_*() functions to access the > image. > > You will also want to specify BDRV_O_CACHE_WB (while it exists, I'm > going to remove it soon). Ok, will do. > > +static QCryptoBlockOpenOptions * > > +block_crypto_open_opts_init(QCryptoBlockFormat format, > > + QemuOpts *opts, > > + Error **errp) > > +{ > > + OptsVisitor *ov; > > + QCryptoBlockOpenOptions *ret = NULL; > > + Error *local_err = NULL; > > + > > + ret = g_new0(QCryptoBlockOpenOptions, 1); > > + ret->format = format; > > + > > + ov = opts_visitor_new(opts); > > + > > + visit_start_struct(opts_get_visitor(ov), > > + "luks", NULL, 0, &local_err); > > As this refers to "luks" specifically, shouldn't it be inside the switch > below? Or probably better if I just change it to "", since this parameter is not actually used in this context IIRC. > > + if (local_err) { > > + goto out; > > + } > > + > > + switch (format) { > > + case Q_CRYPTO_BLOCK_FORMAT_LUKS: > > + visit_type_QCryptoBlockOptionsLUKS_members( > > + opts_get_visitor(ov), &ret->u.luks, &local_err); > > + break; > > + > > + default: > > + error_setg(&local_err, "Unsupported block format %d", format); > > + break; > > + } > > + error_propagate(errp, local_err); > > + local_err = NULL; > > + > > + visit_end_struct(opts_get_visitor(ov), &local_err); > > + > > + out: > > + if (local_err) { > > + error_propagate(errp, local_err); > > + qapi_free_QCryptoBlockOpenOptions(ret); > > + ret = NULL; > > + } > > + opts_visitor_cleanup(ov); > > + return ret; > > +} > > +#define BLOCK_CRYPTO_MAX_SECTORS 32 > > + > > +static coroutine_fn int > > +block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, > > + int remaining_sectors, QEMUIOVector *qiov) > > +{ > > + BlockCrypto *crypto = bs->opaque; > > + int cur_nr_sectors; /* number of sectors in current iteration */ > > + uint64_t bytes_done = 0; > > + uint8_t *cipher_data = NULL; > > + QEMUIOVector hd_qiov; > > + int ret = 0; > > + size_t payload_offset = > > + qcrypto_block_get_payload_offset(crypto->block) / 512; > > + > > + qemu_iovec_init(&hd_qiov, qiov->niov); > > + > > + qemu_co_mutex_lock(&crypto->lock); > > + > > + /* Bounce buffer so we have a linear mem region for > > + * entire sector. XXX optimize so we avoid bounce > > + * buffer in case that qiov->niov == 1 > > + */ > > + cipher_data = > > + qemu_try_blockalign(bs->file->bs, BLOCK_CRYPTO_MAX_SECTORS * 512); > > As long as we create an individual buffer for each request, shouldn't > MIN(BLOCK_CRYPTO_MAX_SECTORS * 512, qiov->size) be enough? Yes, you're right. I didn't realize there was a pre-calcuated total size field in QEMUIOVector > > + if (cipher_data == NULL) { > > + ret = -ENOMEM; > > + goto cleanup; > > + } > > + > > + while (remaining_sectors) { > > + cur_nr_sectors = remaining_sectors; > > + > > + if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) { > > + cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS; > > + } > > + > > + qemu_iovec_reset(&hd_qiov); > > + qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512); > > + > > + qemu_co_mutex_unlock(&crypto->lock); > > Between qemu_co_mutex_lock() and here, there is no yield point... > > > + ret = bdrv_co_readv(bs->file->bs, > > + payload_offset + sector_num, > > + cur_nr_sectors, &hd_qiov); > > + qemu_co_mutex_lock(&crypto->lock); > > + if (ret < 0) { > > + goto cleanup; > > + } > > + > > + if (qcrypto_block_decrypt(crypto->block, > > + sector_num, > > + cipher_data, cur_nr_sectors * 512, > > + NULL) < 0) { > > + ret = -1; > > Need a real -errno code here. > > > + goto cleanup; > > + } > > ...nor is there one between here and the end of the function. > > So what does this CoMutex protect? If qcrypto_block_decrypt() needs this > for some reason (it doesn't seem to be touching anything that isn't per > request, but maybe I'm missing something), would it be clearer to put > the locking only around that call? This just a result of me blindly copying the locking pattern from qcow2.c qcow2_co_readv() method without really understanding what it was protecting. If it not possible for two calls to bdrv_co_readv() to run in parallel, then I can drop this mutex. > > > + > > + qemu_iovec_from_buf(qiov, bytes_done, > > + cipher_data, cur_nr_sectors * 512); > > + > > + remaining_sectors -= cur_nr_sectors; > > + sector_num += cur_nr_sectors; > > + bytes_done += cur_nr_sectors * 512; > > + } > > + > > + cleanup: > > + qemu_co_mutex_unlock(&crypto->lock); > > + > > + qemu_iovec_destroy(&hd_qiov); > > + qemu_vfree(cipher_data); > > + > > + return ret; > > +} > > + > > +BlockDriver bdrv_crypto_luks = { > > + .format_name = "luks", > > + .instance_size = sizeof(BlockCrypto), > > + .bdrv_probe = block_crypto_probe_luks, > > + .bdrv_open = block_crypto_open_luks, > > + .bdrv_close = block_crypto_close, > > + .bdrv_create = block_crypto_create_luks, > > + .create_opts = &block_crypto_create_opts_luks, > > + > > + .bdrv_co_readv = block_crypto_co_readv, > > + .bdrv_co_writev = block_crypto_co_writev, > > + .bdrv_getlength = block_crypto_getlength, > > +}; > > Rather minimalistic, but we can always add the missing functions later. Do you have any recommendations on which are top priority / important callbacks to add support for so I can prioritize future effort. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|