[libvirt] [PATCH] qemu: Fix crash hot plugging luks volume
Daniel P. Berrange
berrange at redhat.com
Wed Aug 17 15:22:25 UTC 2016
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 at entry=0x7fffc03be160, disk=disk at 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 at redhat.com>
> >> ---
> >>
> >> 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 :|
More information about the libvir-list
mailing list