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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]