- Devel - libvirt List Archives

[libvirt] [PATCH] storage_driver: Don't crash in storagePoolCreateXML
by Michal Privoznik 23 Aug '19

23 Aug '19

In my recent patches I've introduced virStoragePoolObjIsStarting() which is then used to protect storage pool definition when the pool object is locked and unlocked during long running jobs. Well, my patches did not anticipate that @obj can be NULL under 'cleanup' label in storagePoolCreateXML() (for instance when parsing XML fails). This imperfection is causing libvirtd to crash then. Fixes: 13284a6b83 storage_driver: Protect pool def during startup and build Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index cd9f14a2c0..30940b5dcf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -808,7 +808,7 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: - if (virStoragePoolObjIsStarting(obj)) { + if (obj && virStoragePoolObjIsStarting(obj)) { if (!virStoragePoolObjIsActive(obj)) virStoragePoolUpdateInactive(obj); virStoragePoolObjSetStarting(obj, false); -- 2.21.0

2 1

[libvirt] [PATCH 0/2] rpm: fix usage of 'nc' dep
by Daniel P. Berrangé 23 Aug '19

23 Aug '19

Daniel P. Berrangé (2): rpm: depend on /usr/bin/nc instead of nc rpm: move nc dep into the libvirt-daemon sub-RPM libvirt.spec.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.21.0

2 4

[libvirt] [PATCH v3] tools: console: Relax stream EOF handling
by Roman Bolshakov 23 Aug '19

23 Aug '19

Regular VM shutdown triggers the error for existing session of virsh console and it returns with non-zero exit code: error: internal error: console stream EOF The message and status code are misleading because there's no real error. virStreamRecv returns 0 correctly when EOF is reached. Existing implementations of esx, fd, and remote streams behave the same for virStreamFinish and virStreamAbort: they close the stream. So, we can continue to use virStreamAbort to handle EOF and errors from virStreamRecv but additonally we can report error if virStreamAbort fails. Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user") Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com> --- tools/virsh-console.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index e16f841e57..a235a9a283 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -106,7 +106,9 @@ virConsoleShutdown(virConsolePtr con) if (con->st) { virStreamEventRemoveCallback(con->st); - virStreamAbort(con->st); + if (virStreamAbort(con->st) < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot terminate console stream")); virStreamFree(con->st); con->st = NULL; } @@ -172,10 +174,6 @@ virConsoleEventOnStream(virStreamPtr st, if (got == -2) goto cleanup; /* blocking */ if (got <= 0) { - if (got == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("console stream EOF")); - virConsoleShutdown(con); goto cleanup; } -- 2.22.0

3 2

[libvirt] [PATCH v1 00/11] Couple of storage driver improvements
by Michal Privoznik 23 Aug '19

23 Aug '19

Patches 02/11, 09/11 and finally 11/11 are actual bug fixes. The rest is a preparation work. More detailed: 02/11 - Fixes a refcounting issue (and thus invalid memory access) 09/11 - Fixes a problem where starting a transitive pool which has a persistent config may lead to it's disappearance 11/11 - Tries to relax locking to allow more concurrent access Michal Prívozník (11): virStoragePoolObjRemove: Don't unlock pool object upon return virStoragePoolObjListForEach: Grab a reference for pool object storagePoolRefreshImpl: Fix variable name in comment virStoragePoolUpdateInactive: Don't call virStoragePoolObjEndAPI virstorageobj: Rename virStoragePoolObjAssignDef virStoragePoolObjListAdd: Turn boolean arg into flags virStoragePoolObjListAdd: Separate out definition assignment virstorageobj: Introduce VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE flag storagePoolCreateXML: Don't lose persistent storage on failed create storage_driver: Protect pool def during startup and build storage: Drop and reacquire pool obj lock in some backends src/conf/virstorageobj.c | 89 ++++++++++++++++++++------ src/conf/virstorageobj.h | 17 ++++- src/libvirt_private.syms | 4 +- src/storage/storage_backend_disk.c | 11 +++- src/storage/storage_backend_fs.c | 13 +++- src/storage/storage_backend_logical.c | 15 +++-- src/storage/storage_backend_vstorage.c | 9 ++- src/storage/storage_backend_zfs.c | 7 +- src/storage/storage_driver.c | 67 +++++++++++-------- src/test/test_driver.c | 21 ++---- 10 files changed, 181 insertions(+), 72 deletions(-) -- 2.21.0

3 25

[libvirt] RFC: revival of hotplug/unplug for PCI Multifunction devices in QEMU guests
by Daniel Henrique Barboza 23 Aug '19

23 Aug '19

Hi, This is labeled as RFC but it's more like a FYI to let people know and comment beforehand. Shiva sent a 28 patch series last year that implements hotplug/unplug support for PCI multifunction devices [1]. The design motivation of his work was based in a RFC sent to this mailing list back in 2016 [2]. I'll briefly summarize the goals and motivations here. What we have today in Libvirt: - no hotplug/unplug support for multifunction PCI devices This is explained in details in [2]. When hotplugging a multifunction device, QEMU will queue the hotplug operation of all non-zero functions and, when function 0 is hotplugged, all functions are hotplugged together. This is true for all archs that supports PCI multifunction devices in QEMU. For unplug it varies: x86 will unplug all functions if any function is unplugged, ppc64 needs to unplug each one. Due to the nature of how Libvirt hotplug works now, hotplug of these devices is not possible. All hotplugs are considered in an isolated manner. Even if we hotplug each function in the proper order (i.e. leaving function 0 last), Libvirt can assign different slots in the guest for each. Similar problems happens with hot-unplug. This feature aims to address these by creating a new <devices> element, exclusive for multifunction devices, that aggregates all functions of a device in a single operation. To handle this new element, the existing attach/detach functions in the QEMU driver now handles multiple devices. Attaching/detaching a single device is routed away from the specialized multifunction code to be handled to the existing attach/detach code base. - no support for partial assignment of functions We can't make the assumption that the guest will always assign all devices of a multifunction device. Some functions might be a security risk to expose to a guest, or the device can behave differently depending on the amount of functions assigned. Even if the 'leftover' functions can't be used to anything else in the host, the decision of full/partial assignment of functions should come from the user, not us. We can't predict how any other hardware vendor will setup its devices. This patch series also handles this case. ------ The latest version of this feature, rebase to cdd362e0e7a (the current master as I'm writing this), can be found at: https://github.com/danielhb/libvirt/tree/multifunction-rc2 This is a work still ongoing that it's not ready for contribution yet (first patches that changes the unit test code are breaking existing tests). The feature itself is being stress tested using a Broadcom BCM5719 with 4 functions and it looks solid. I'll see if I can grab a x86 env with a multifunction card to test it there too when I have the chance. Thoughts/comments are welcome. Instead of dropping a 28+ patch set for review, I'm planning into sending smaller chunks that makes senses individually. Thanks, DHB [1] https://www.redhat.com/archives/libvir-list/2018-March/msg00729.html [2] https://www.redhat.com/archives/libvir-list/2016-April/msg01057.html

3 7

[libvirt] [PATCH] virpci: Rename virPCIDevice{Bind, Unbind}FromStubWithOverride
by Michal Privoznik 23 Aug '19

23 Aug '19

After my previous patches we have virPCIDeviceBindToStub() and virPCIDeviceUnbindFromStub() which really do nothing but call virPCIDeviceBindToStubWithOverride() and virPCIDeviceUnbindFromStubWithOverride() respectively. Drop "WithOverride" from the names and drop the thin wrappers. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Pushed under trivial rule. src/util/virpci.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 6724a8ad9e..ee78151e74 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1136,7 +1136,7 @@ virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, } static int -virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { if (!dev->unbind_from_stub) { VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); @@ -1147,13 +1147,7 @@ virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) } static int -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) -{ - return virPCIDeviceUnbindFromStubWithOverride(dev); -} - -static int -virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) +virPCIDeviceBindToStub(virPCIDevicePtr dev) { const char *stubDriverName; VIR_AUTOFREE(char *) stubDriverPath = NULL; @@ -1192,12 +1186,6 @@ virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) return 0; } -static int -virPCIDeviceBindToStub(virPCIDevicePtr dev) -{ - return virPCIDeviceBindToStubWithOverride(dev); -} - /* virPCIDeviceDetach: * * Detach this device from the host driver, attach it to the stub -- 2.21.0

1 0

[libvirt] [PATCH 00/12] Drop KVM assignment
by Michal Privoznik 23 Aug '19

23 Aug '19

The KVM style of PCI assignment is not used, and it hasn't been for a while. Any attempt to start a domain with it would result in error as kernel dropped its support in 4.12.0 (after being deprecated for 1.5 years). Michal Prívozník (12): qemu: Drop KVM assignment tests: Remove 'kvm' PCI backend from domaincapstest virhostdev: Unify virDomainHostdevDef to virPCIDevice translation qemu: Drop unused qemuOpenPCIConfig() virhostdev: Disable legacy kvm assignment virpci: Drop 'pci-stub' driver virpci: Remove unused virPCIDeviceWaitForCleanup virpci: Drop newid style of PCI device detach virpcimock: Don't create "pci-stub" driver virpcimock: Don't create new_id or remove_id files virpcimock: Drop @driverActions enum news: Document KVM assignment removal docs/news.xml | 13 + src/libvirt_private.syms | 1 - src/qemu/qemu_capabilities.c | 6 - src/qemu/qemu_command.c | 48 +-- src/qemu/qemu_command.h | 3 - src/qemu/qemu_driver.c | 14 +- src/qemu/qemu_hostdev.c | 44 +- src/qemu/qemu_hostdev.h | 1 - src/qemu/qemu_hotplug.c | 20 +- src/util/virhostdev.c | 97 +++-- src/util/virpci.c | 403 +----------------- src/util/virpci.h | 2 - .../qemu_1.7.0.x86_64.xml | 1 - .../qemu_2.12.0-virt.aarch64.xml | 1 - .../qemu_2.12.0.ppc64.xml | 1 - .../qemu_2.12.0.s390x.xml | 1 - .../qemu_2.12.0.x86_64.xml | 1 - .../qemu_2.6.0-virt.aarch64.xml | 1 - .../qemu_2.6.0.aarch64.xml | 1 - .../domaincapsschemadata/qemu_2.6.0.ppc64.xml | 1 - .../qemu_2.6.0.x86_64.xml | 1 - .../domaincapsschemadata/qemu_2.7.0.s390x.xml | 1 - .../qemu_2.8.0-tcg.x86_64.xml | 1 - .../domaincapsschemadata/qemu_2.8.0.s390x.xml | 1 - .../qemu_2.8.0.x86_64.xml | 1 - .../qemu_2.9.0-q35.x86_64.xml | 1 - .../qemu_2.9.0-tcg.x86_64.xml | 1 - .../qemu_2.9.0.x86_64.xml | 1 - .../domaincapsschemadata/qemu_3.0.0.s390x.xml | 1 - .../qemu_3.1.0.x86_64.xml | 1 - .../domaincapsschemadata/qemu_4.0.0.s390x.xml | 1 - .../qemu_4.0.0.x86_64.xml | 1 - .../qemu_4.1.0.x86_64.xml | 1 - tests/domaincapstest.c | 4 +- tests/virpcimock.c | 137 +----- 35 files changed, 92 insertions(+), 722 deletions(-) -- 2.21.0

3 41

[libvirt] [PATCH] mdev: point user to mdevctl for missing devices
by Jonathon Jongsma 22 Aug '19

22 Aug '19

When a host is rebooted, any mediated devices that were previously configured will disappear. There have been requests for libvirt to handle persisting these mediated devices across reboots, but the decision was made that this should be handled at a lower level. mdevctl is a new tool that handles registration and persistence of mediated devices. If desired, mdevctl can automatically start these mediated devices when the parent device becomes available. Since mdevctl is the recommended solution for handling persistent mediated devices, point users there when they encounter an error for a missing mediated device. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com> --- NOTE: - previous patch which attempted to start missing devices using mdevctl has been withdrawn. src/util/virmdev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 3d5488cdae..df02fc8d28 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -149,7 +149,9 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) if (!virFileExists(sysfspath)) { virReportError(VIR_ERR_DEVICE_MISSING, - _("mediated device '%s' not found"), uuidstr); + _("mediated device '%s' not found. " + "Persistent devices can be managed with 'mdevctl'."), + uuidstr); return NULL; } -- 2.21.0

2 2

[libvirt] [PATCH v2 0/2] security: Deal with stale XATTRs
by Michal Privoznik 22 Aug '19

22 Aug '19

v2 of: https://www.redhat.com/archives/libvir-list/2019-August/msg00520.html diff to v1: - use virOnce to obtain host boot time - switched to configure time check of getutxid - dropped host's UUID from timestamp Michal Prívozník (2): util: Introduce virhostuptime security_util: Remove stale XATTRs configure.ac | 1 + src/libvirt_private.syms | 4 + src/security/security_util.c | 196 +++++++++++++++++++++++++++++++- src/util/Makefile.inc.am | 2 + src/util/virhostuptime.c | 81 +++++++++++++ src/util/virhostuptime.h | 27 +++++ tests/qemusecuritymock.c | 12 ++ tools/libvirt_recover_xattrs.sh | 2 +- 8 files changed, 323 insertions(+), 2 deletions(-) create mode 100644 src/util/virhostuptime.c create mode 100644 src/util/virhostuptime.h -- 2.21.0

2 6

[libvirt] [PATCH] security: Don't increase XATTRs refcounter on failure
by Michal Privoznik 22 Aug '19

22 Aug '19

If user has two domains, each have the same disk (configured for RW) but each runs with different seclabel then we deny start of the second domain because in order to do that we would need to relabel the disk but that would cut the first domain off. Even if we did not do that, qemu would fail to start because it would be unable to lock the disk image for the second time. So far, this behaviour is expected. But what is not expected is that we increase the refcounter in XATTRs and leave it like that. What happens is that when the second domain starts, virSecuritySetRememberedLabel() is called, and since there are XATTRs from the first domain it increments the refcounter and returns it (refcounter == 2 at this point). Then callers (virSecurityDACSetOwnership() and virSecuritySELinuxSetFileconHelper()) realize that refcounter is greater than 1 and desired seclabel doesn't match the one the disk image already has and an error is produced. But the refcounter is never decremented. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/security/security_dac.c | 19 ++++++++++++++----- src/security/security_selinux.c | 17 +++++++++++------ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 137daf5d28..b0070f7390 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -754,6 +754,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, struct stat sb; int refcount; int rc; + bool rollback = false; + int ret = -1; if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) @@ -780,16 +782,18 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, } else if (refcount < 0) { return -1; } else if (refcount > 1) { + rollback = true; /* Refcount is greater than 1 which means that there * is @refcount domains using the @path. Do not * change the label (as it would almost certainly * cause the other domains to lose access to the - * @path). */ + * @path). However, the refcounter was incremented in + * XATTRs so decrease it. */ if (sb.st_uid != uid || sb.st_gid != gid) { virReportError(VIR_ERR_OPERATION_INVALID, _("Setting different DAC user or group on %s " "which is already in use"), path); - return -1; + goto cleanup; } } } @@ -797,7 +801,13 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) { + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + goto cleanup; + + ret = 0; + + cleanup: + if (ret < 0 && rollback) { virErrorPtr origerr; virErrorPreserveLast(&origerr); @@ -812,10 +822,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, NULLSTR(src ? src->path : path)); virErrorRestore(&origerr); - return -1; } - return 0; + return ret; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ea20373a90..0c6ace75fa 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1334,6 +1334,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, security_context_t econ = NULL; int refcount; int rc; + bool rollback = false; int ret = -1; if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, @@ -1358,11 +1359,13 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } else if (refcount < 0) { goto cleanup; } else if (refcount > 1) { + rollback = true; /* Refcount is greater than 1 which means that there * is @refcount domains using the @path. Do not * change the label (as it would almost certainly * cause the other domains to lose access to the - * @path). */ + * @path). However, the refcounter was + * incremented in XATTRs so decrease it. */ if (STRNEQ(econ, tcon)) { virReportError(VIR_ERR_OPERATION_INVALID, _("Setting different SELinux label on %s " @@ -1373,7 +1376,12 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) { + if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0 && rollback) { virErrorPtr origerr; virErrorPreserveLast(&origerr); @@ -1388,11 +1396,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, path); virErrorRestore(&origerr); - goto cleanup; - } - ret = 0; - cleanup: + } freecon(econ); return ret; } -- 2.21.0

2 1