Re: [libvirt] [PATCH] qemu: Fix crash hot plugging luks volume

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(a)redhat.com&gt; >> --- >> >> 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.