[libvirt] [PATCHv2 01/33] qemu: process: Refresh backing chain info when reconnecting to qemu

[Eric Blake]

On 05/22/2014 07:47 AM, Peter Krempa wrote:
> Refresh the disk backing chains when reconnecting to a qemu process
> after daemon restart. There are a few internal fields that don't get
> refreshed from the XML. Until we are able to do that, let's reload all
> the metadata by the backing chain crawler.

Not necessarily ideal.  Reading metadata from a file that is also
simultaneously opened read-write by qemu might, in super-rare
circumstances, hit a race where qemu is in the middle of updating
metadata due to the completion of some job that was started before the
libvirtd restart.  Although qcow2 has a hard cap of header fitting in
one cluster (typically 64k) (at least, after CVE-2014-0144, qemu.git
commit 24342f2ca), it is still plausible that qemu is updating the qcow2
header via sectors (perhaps as small as 512-byte chunks) rather than by
full clusters; and there are scenarios where an update touches two
non-adjacent sectors (such as adjusting the backing file name), such
that if a reader sees the old content of one sector but the new content
of the other, then it is completely broken in interpreting the data.

A slightly safer alternative might be using a QMP command to ask qemu
what the backing chain currently is when we reconnect - although that
may require correlation back to filenames we passed in, since in the
face of fd passing, qemu may only know the name "/dev/fdset/NNN" instead
of the filename the earlier libvirtd opened and passed in by fd.  And
that presupposes that qemu can always be trusted (we don't want to
introduce a CVE where a guest that compromises qemu into returning bogus
QMP results can then cause libvirt to mismanage the host filesystem).

So with those points made, a metadata read race is extremely rare, and I
see no point in implementing a half-way solution that requires parsing a
QMP command if we are eventually going to switch over to full XML
tracking anyways.  Therefore, since _this_ patch is a clear improvement
(even if not theoretically perfect), we shouldn't hold it up waiting for
some other solution.

ACK as-is.

>  src/qemu/qemu_process.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a83780f..4aa9ca3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3213,6 +3213,11 @@ qemuProcessReconnect(void *opaque)
>          if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0)
>              goto error;
> 
> +        /* XXX we should be able to restore all data from XML in the future */

and thanks for the comment to remind us what we should improve.

> +        if (qemuDomainDetermineDiskChain(driver, obj,
> +                                         obj->def->disks[i], true) < 0)
> +            goto error;
> +
>          dev.type = VIR_DOMAIN_DEVICE_DISK;
>          dev.data.disk = obj->def->disks[i];
>          if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140522/9b54cfd3/attachment-0001.sig>