Re: [Qemu-devel] [PATCH v3 1/5] qga: win32: fix crashes when PCI info cannot be retrived

[Michael Roth]

Quoting Tomáš Golembiovský (2018-09-07 06:42:09)
> The guest-get-fsinfo command collects also information about PCI
> controller where the disk is attached. When this fails for some reasons
> it tries to return just the partial information. However in certain
> cases the pointer to the structure was not initialized and was set to
> NULL. This breaks the serializer and leads to a crash of the guest agent.
> 
> Signed-off-by: Tomáš Golembiovský <address@hidden>

For a win10 guest started with:

qemu-system-x86_64 -m 2G -smp 2,cores=2,sockets=1 -drive 
file=/home/mdroth/vm/win10_pro_n_snap0.qcow2,if=virtio -drive 
file=/home/mdroth/vm/virtio-win-0.1.102.iso,if=ide,media=cdrom -rtc 
base=localtime,driftfix=slew -vga std -boot d -name vm4 -netdev 
tap,script=/etc/qemu-ifup,vhost=on,id=vnet0 -device 
virtio-net-pci,mac=52:54:00:12:34:04,id=vnic0,netdev=vnet0,disable-modern=true 
-vnc :4 -device virtio-serial -balloon virtio -mon chardev=hmp0 -chardev 
socket,path=/tmp/vm4-hmp0.sock,server,nowait,id=hmp0 -mon 
chardev=qmp0,mode=control -chardev 
socket,path=/tmp/vm4-qmp0.sock,server,nowait,id=qmp0 -device 
virtserialport,chardev=vs0,name=vs0,id=vs_vs0 -chardev 
socket,path=/tmp/vm4-vs0.sock,server,nowait,id=vs0 -device 
virtserialport,chardev=vs1,name=vs1,id=vs_vs1 -chardev 
socket,path=/tmp/vm4-vs1.sock,server,nowait,id=vs1 -device 
virtserialport,chardev=qga,name=org.qemu.guest_agent.0,id=vs_qga -chardev 
socket,path=/tmp/vm4-qga.sock,server,nowait,id=qga -device 
isa-serial,chardev=serial0,id=serial_serial0 -chardev 
socket,path=/tmp/vm4-serial0.sock,server,nowait,id=serial0 -L 
/home/mdroth/w/build/qemu-2.11.2-build/pc-bios --enable-kvm

this yields the following:

{'execute':'guest-get-fsinfo'}
{"return": [{"name": "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", 
"total-bytes": 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", 
"bus": 0, "unit": 0, "pci-controller": {"bus": 0, "slot": 0, "domain": 0, 
"function": 0}, "target": 0}], "used-bytes": 160755712, "type": "CDFS"}, 
{"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\", "mountpoint": 
"System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, 
"pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function": 0}, "target": 
0}], "type": "NTFS"}, {"name": 
"\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes": 
52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": 0, 
"unit": 0, "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function": 0}, 
"target": 0}], "used-bytes": 25265487872, "type": "NTFS"}, {"name": 
"\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint": "System 
Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": 
{"bus": 0, "slot": 0, "domain": 0, "function": 0}, "target": 0}], "type": 
"NTFS"}]}

domain/bus/slot/function=0 are valid PCI addresses so initializing to 0 is
wrong. Sameeh had a previous series that initializes to -1 that I think
is more appropriate (it hasn't gone in yet since we opted not to enable
CONFIG_QGA_NTDDSCSI for 3.0 since the PCI stuff seems be generally
broken for Windows, also because the 2nd patch needs some fixups:

  https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07500.html

With that series (and some fixups I have on top at
https://github.com/mdroth/qemu/commits/qga-test), we get the following
output:

{'execute':'guest-get-fsinfo'}
{"return": [{"name": "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", 
"total-bytes": 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", 
"bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1, 
"function": -1}, "target": 0}], "used-bytes": 160755712, "type": "CDFS"}, 
{"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\", "mountpoint": 
"System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, 
"pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 3}, 
"target": 0}], "type": "NTFS"}, {"name": 
"\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes": 
52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": 0, 
"unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 
2}, "target": 0}], "used-bytes": 25267560448, "type": "NTFS"}, {"name": 
"\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint": "System 
Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": 
{"bus": -1, "slot": -1, "domain": 0, "function": 1}, "target": 0}], "type": 
"NTFS"}]}

Here we see the non-sensical PCI topology info I mentioned previously.
There are values like '"function": 3' even though there are no
multifunction devices present. This will be exposed to users if we
enable CONFIG_QGA_NTDDSCSI with things as they stand. Currently we
just get an empty array for "disk" field of GuestFilesystemInfo
for w32, which fortunately aligns with the current QAPI schema (it's
an array since the volume can span multiple disks). I'm not about the
SCSI bus/unit/target data either. It's a real mess (maybe things
work a bit better when an actual SCSI controller is used?) and I'm
not sure why/how I didn't notice during initial testing.

I think all this needs to be addressed before we enable
CONFIG_QGA_NTDDSCSI (in terms of what that's used for in the current
code at least, i.e. enabling everything reported by
GuestFilesystemInfo.disk).

What is the oVirt use-case? It doesn't seem like you need PCI topology,
but what about SCSI topology (and if so, does that information seem
correct to you)? Or is just a list a serials/dev paths by themselves
all that's needed? Trying to see how we might decouple things from the
broken PCI topology stuff. May need to deprecate
GuestFilesystemInfo.disk in favor of something less monolithic if all
of this isn't fixable in a reasonable-enough timeframe.


> ---
>  qga/commands-win32.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 98d9735389..9c959122d9 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -633,15 +633,32 @@ static GuestDiskAddressList *build_guest_disk_info(char 
> *guid, Error **errp)
>           * 
> https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */
>          if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
>                              sizeof(SCSI_ADDRESS), &len, NULL)) {
> +            Error *local_err = NULL;
>              disk->unit = addr.Lun;
>              disk->target = addr.TargetId;
>              disk->bus = addr.PathId;
> -            disk->pci_controller = get_pci_info(name, errp);
> +            g_debug("unit=%lld target=%lld bus=%lld",
> +                disk->unit, disk->target, disk->bus);
> +            disk->pci_controller = get_pci_info(name, &local_err);
> +
> +            if (local_err) {
> +                g_debug("failed to get PCI controller info: %s",
> +                    error_get_pretty(local_err));
> +                error_free(local_err);
> +            } else if (disk->pci_controller != NULL) {
> +                g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld",
> +                    disk->pci_controller->domain,
> +                    disk->pci_controller->bus,
> +                    disk->pci_controller->slot,
> +                    disk->pci_controller->function);
> +            }
>          }
> -        /* We do not set error in this case, because we still have enough
> -         * information about volume. */
> -    } else {
> -         disk->pci_controller = NULL;
> +    }
> +    /* We do not set error in case pci_controller is NULL, because we still
> +     * have enough information about volume. */
> +    if (disk->pci_controller == NULL) {
> +        g_debug("no PCI controller info");
> +        disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
>      }
> 
>      list = g_malloc0(sizeof(*list));
> -- 
> 2.18.0
>