On Wed, Aug 17, 2016 at 10:19:40AM -0400, John Ferlan wrote: > > > On 08/17/2016 05:40 AM, Daniel P. Berrange wrote: > > On Tue, Aug 16, 2016 at 11:25:35AM -0400, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1367259 > >> > >> Crash occurs because 'secrets' is being dereferenced in call: > >> > >> if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, > >> VIR_SECRET_USAGE_TYPE_VOLUME, NULL, > >> &src->encryption->secrets[0]->seclookupdef, > >> true) < 0) > >> > >> (gdb) p *src->encryption > >> $1 = {format = 2, nsecrets = 0, secrets = 0x0, encinfo = {cipher_size = 0, > >> cipher_name = 0x0, cipher_mode = 0x0, cipher_hash = 0x0, ivgen_name = 0x0, > >> ivgen_hash = 0x0}} > >> (gdb) bt > >> priv=priv@entry=0x7fffc03be160, disk=disk@entry=0x7fffb4002ae0) > >> at qemu/qemu_domain.c:1087 > >> disk=0x7fffb4002ae0, vm=0x7fffc03a2580, driver=0x7fffc02ca390, > >> conn=0x7fffb00009a0) at qemu/qemu_hotplug.c:355 > >> > >> Upon entry to qemuDomainAttachVirtioDiskDevice, src->encryption points > >> at a valid 'secret' buffer w/ nsecrets == 1; however, the call to > >> qemuDomainDetermineDiskChain will call virStorageFileGetMetadata > >> and eventually virStorageFileGetMetadataInternal where the src->encryption > >> was overwritten when probing the volume. > >> > >> Commit id 'a48c7141' added code to virStorageFileGetMetadataInternal > >> to determine if the disk/volume would use/need encryption and allocated > >> a meta->encryption. This overwrote an existing encryption buffer > >> already provided by the XML > >> > >> This patch adds an argument to virStorageFileGetMetadataInternal in > >> order to avoid the encryption probe of the volume. This will only be > >> be set to true for the storage volume buffer/fd parsing. > >> > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > >> > >> NB: This I believe the bare bones that could be done. If desired, I could > >> also modify virStorageFileGetMetadata[Recurse] and all the callers in > >> order to specifically add a 'probe_encryption' variable there as well. > >> For this path, the "root" callers would add the 'false', hence the > >> reason I chose to just modify the *Recurse function to pass 'false'. > >> I have tested creating a luks volume and of course hot plugging with > >> this patch... > > > > I'll assume you tested that 'virsh vol-dumpxml /path/to/storage/vol' still > > shows the encryption details too. > > > > It does... > > >> src/storage/storage_driver.c | 2 +- > >> src/util/virstoragefile.c | 8 +++++--- > >> src/util/virstoragefile.h | 3 ++- > >> 3 files changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > >> index 6f1e372..fe0e164 100644 > >> --- a/src/storage/storage_driver.c > >> +++ b/src/storage/storage_driver.c > >> @@ -3239,7 +3239,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, > >> &buf)) < 0) > >> goto cleanup; > >> > >> - if (virStorageFileGetMetadataInternal(src, buf, headerLen, > >> + if (virStorageFileGetMetadataInternal(src, buf, headerLen, false, > >> &backingFormat) < 0) > >> goto cleanup; > >> > >> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c > >> index 471aa1f..4b6bde3 100644 > >> --- a/src/util/virstoragefile.c > >> +++ b/src/util/virstoragefile.c > >> @@ -928,6 +928,7 @@ int > >> virStorageFileGetMetadataInternal(virStorageSourcePtr meta, > >> char *buf, > >> size_t len, > >> + bool probe_encryption, > >> int *backingFormat) > >> { > >> int ret = -1; > >> @@ -946,7 +947,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, > >> goto cleanup; > >> } > >> > >> - if (fileTypeInfo[meta->format].cryptInfo != NULL) { > >> + if (probe_encryption && fileTypeInfo[meta->format].cryptInfo != NULL) { > >> for (i = 0; fileTypeInfo[meta->format].cryptInfo[i].format != 0; i++) { > >> if (virStorageFileHasEncryptionFormat(&fileTypeInfo[meta->format].cryptInfo[i], > >> buf, len)) { > >> @@ -1129,7 +1130,7 @@ virStorageFileGetMetadataFromBuf(const char *path, > >> if (!(ret = virStorageFileMetadataNew(path, format))) > >> return NULL; > >> > >> - if (virStorageFileGetMetadataInternal(ret, buf, len, > >> + if (virStorageFileGetMetadataInternal(ret, buf, len, true, > >> backingFormat) < 0) { > >> virStorageSourceFree(ret); > >> return NULL; > >> @@ -1200,7 +1201,8 @@ virStorageFileGetMetadataFromFD(const char *path, > >> goto cleanup; > >> } > >> > >> - if (virStorageFileGetMetadataInternal(meta, buf, len, backingFormat) < 0) > >> + if (virStorageFileGetMetadataInternal(meta, buf, len, true, > >> + backingFormat) < 0) > >> goto cleanup; > >> > >> if (S_ISREG(sb.st_mode)) > >> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h > >> index 3d09468..bb71372 100644 > >> --- a/src/util/virstoragefile.h > >> +++ b/src/util/virstoragefile.h > >> @@ -289,8 +289,9 @@ int virStorageFileProbeFormatFromBuf(const char *path, > >> int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, > >> char *buf, > >> size_t len, > >> + bool probe_encryption, > >> int *backingFormat) > >> - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); > >> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); > >> > >> virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, > >> int fd, > > > > So a thought occurrs to me that perhaps rather than adding the flag, we could > > take a slightly different route. > > > > In virStorageFileGetMetadataInternal() we could still probe the > > encryption method unconditionally. If no meta->encryption is > > present, then we could just set that, but if one is present, then > > we can validate that the probed data matches the existing data > > and raise an error on mis-match. > > Hmm. OK, so something like: > > int expt_fmt = fileTypeInfo[meta->format].cryptInfo[i].format; > if (!meta->encryption) { > if (VIR_ALLOC(meta->encryption) < 0) > goto cleanup; > > meta->encryption->format = expt_fmt; > } else { > if (meta->encryption->format != expt_fmt) { > virReportError(VIR_ERR_XML_ERROR, > _("encryption format %d doesn't match " > "expected format %d"), > meta->encryption->format, expt_fmt); > goto cleanup; > } > } > > I can post a v2, but wanted to make sure I wasn't misunderstanding your > request. Yep, exactly that. 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list