[libvirt] [PATCH 00/12] introduce locking into every driver
As part of the proposal to introduce an embedded driver feature, we decided we ought to have each driver acquire a lock against the virtual root it is configured to use. This will prevent two apps from running an embedded driver with the same root. https://www.redhat.com/archives/libvir-list/2019-May/msg00467.html It turns out that this will also be useful for the per-driver split daemons work I am working on. In that case libvirtd will exist as a monolithic daemon running all drivers, and we'll also have per-driver daemons like virtqemud, virtnetworkd, etc. It is only valid to run in one setup on the host. ie you must choose libvirtd, or choose per-driver daemons, never both. The per-driver locking will provide very useful protection against mistakes in this respect. Since the locking is functionally independant of both patch series, I'm sending it now. Daniel P. Berrangé (12): qemu: acquire a pidfile in the driver root directory secrets: acquire a pidfile in the driver root directory network: acquire a pidfile in the driver root directory storage: acquire a pidfile in the driver root directory nodedev: acquire a pidfile in the driver root directory interface: acquire a pidfile in the driver root directory nwfilter: acquire a pidfile in the driver root directory libxl: remove obsolete check for xend during driver startup libxl: acquire a pidfile in the driver root directory lxc: acquire a pidfile in the driver root directory vz: acquire a pidfile in the driver root directory bhyve: acquire a pidfile in the driver root directory src/bhyve/bhyve_driver.c | 9 +++++ src/bhyve/bhyve_utils.h | 3 ++ src/conf/virnodedeviceobj.h | 5 +++ src/conf/virnwfilterobj.h | 4 +++ src/conf/virstorageobj.h | 3 ++ src/interface/interface_backend_netcf.c | 45 +++++++++++++++++++++++-- src/interface/interface_backend_udev.c | 44 +++++++++++++++++++++++- src/libxl/libxl_conf.h | 3 ++ src/libxl/libxl_driver.c | 37 ++++++++------------ src/lxc/lxc_conf.h | 3 ++ src/lxc/lxc_driver.c | 9 +++++ src/network/bridge_driver.c | 10 ++++++ src/network/bridge_driver_platform.h | 3 ++ src/node_device/node_device_hal.c | 31 +++++++++++++++++ src/node_device/node_device_udev.c | 32 ++++++++++++++++++ src/nwfilter/nwfilter_driver.c | 19 +++++++++++ src/qemu/qemu_conf.h | 3 ++ src/qemu/qemu_driver.c | 9 +++++ src/secret/secret_driver.c | 44 +++++++++++++++++++----- src/storage/storage_driver.c | 11 ++++++ src/vz/vz_driver.c | 40 +++++++++++++++++++--- 21 files changed, 327 insertions(+), 40 deletions(-) -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/qemu/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/qemu/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_driver.c | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e51514a344..2229b76e89 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -221,6 +221,9 @@ struct _virQEMUDriver { * then lockless thereafter */ virQEMUDriverConfigPtr config; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + /* Immutable pointer, self-locking APIs */ virThreadPoolPtr workerPool; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5a75f23981..8bc069d3e2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -70,6 +70,7 @@ #include "node_device_conf.h" #include "virpci.h" #include "virusb.h" +#include "virpidfile.h" #include "virprocess.h" #include "libvirt_internal.h" #include "virxml.h" @@ -587,6 +588,8 @@ qemuStateInitialize(bool privileged, if (VIR_ALLOC(qemu_driver) < 0) return -1; + qemu_driver->lockFD = -1; + if (virMutexInit(&qemu_driver->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); @@ -673,6 +676,10 @@ qemuStateInitialize(bool privileged, goto error; } + if ((qemu_driver->lockFD = + virPidFileAcquire(cfg->stateDir, "driver", true, getpid())) < 0) + goto error; + qemu_driver->qemuImgBinary = virFindFileInPath("qemu-img"); if (!(qemu_driver->lockManager = @@ -1032,6 +1039,8 @@ qemuStateCleanup(void) if (!qemu_driver) return -1; + if (qemu_driver->lockFD != -1) + virPidFileRelease(qemu_driver->config->stateDir, "driver", qemu_driver->lockFD); virThreadPoolFree(qemu_driver->workerPool); virObjectUnref(qemu_driver->config); virObjectUnref(qemu_driver->hostdevMgr); -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/secrets/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/secrets/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/secret/secret_driver.c | 44 +++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index ac85f5d195..9344948db4 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -37,6 +37,7 @@ #include "viruuid.h" #include "virerror.h" #include "virfile.h" +#include "virpidfile.h" #include "configmake.h" #include "virstring.h" #include "viraccessapicheck.h" @@ -56,8 +57,12 @@ struct _virSecretDriverState { virMutex lock; bool privileged; /* readonly */ virSecretObjListPtr secrets; + char *stateDir; char *configDir; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr secretEventState; }; @@ -434,6 +439,10 @@ secretStateCleanup(void) virObjectUnref(driver->secretEventState); + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", driver->lockFD); + + VIR_FREE(driver->stateDir); secretDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -447,11 +456,10 @@ secretStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - char *base = NULL; - if (VIR_ALLOC(driver) < 0) return -1; + driver->lockFD = -1; if (virMutexInit(&driver->lock) < 0) { VIR_FREE(driver); return -1; @@ -462,15 +470,26 @@ secretStateInitialize(bool privileged, driver->privileged = privileged; if (privileged) { - if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0) + if (virAsprintf(&driver->configDir, + "%s/libvirt/secrets", SYSCONFDIR) < 0) + goto error; + if (virAsprintf(&driver->stateDir, + "%s/run/libvirt/secrets", LOCALSTATEDIR) < 0) goto error; } else { - if (!(base = virGetUserConfigDirectory())) + VIR_AUTOFREE(char *) rundir = NULL; + VIR_AUTOFREE(char *) cfgdir = NULL; + + if (!(cfgdir = virGetUserConfigDirectory())) + goto error; + if (virAsprintf(&driver->configDir, "%s/secrets/", cfgdir) < 0) + goto error; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto error; + if (virAsprintf(&driver->stateDir, "%s/secrets/run", rundir) < 0) goto error; } - if (virAsprintf(&driver->configDir, "%s/secrets", base) < 0) - goto error; - VIR_FREE(base); if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) { virReportSystemError(errno, _("cannot create config directory '%s'"), @@ -478,6 +497,16 @@ secretStateInitialize(bool privileged, goto error; } + if (virFileMakePathWithMode(driver->stateDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + driver->stateDir); + goto error; + } + + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", true, getpid())) < 0) + goto error; + if (!(driver->secrets = virSecretObjListNew())) goto error; @@ -488,7 +517,6 @@ secretStateInitialize(bool privileged, return 0; error: - VIR_FREE(base); secretDriverUnlock(); secretStateCleanup(); return -1; -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/network/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/network/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/bridge_driver.c | 10 ++++++++++ src/network/bridge_driver_platform.h | 3 +++ 2 files changed, 13 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 19faf7d514..6292e3b90a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -597,6 +597,7 @@ networkStateInitialize(bool privileged, if (VIR_ALLOC(network_driver) < 0) goto error; + network_driver->lockFD = -1; if (virMutexInit(&network_driver->lock) < 0) { VIR_FREE(network_driver); goto error; @@ -651,6 +652,11 @@ networkStateInitialize(bool privileged, goto error; } + if ((network_driver->lockFD = + virPidFileAcquire(network_driver->stateDir, "driver", + true, getpid())) < 0) + goto error; + /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ network_driver->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); @@ -764,6 +770,10 @@ networkStateCleanup(void) /* free inactive networks */ virObjectUnref(network_driver->networks); + if (network_driver->lockFD != -1) + virPidFileRelease(network_driver->stateDir, "driver", + network_driver->lockFD); + VIR_FREE(network_driver->networkConfigDir); VIR_FREE(network_driver->networkAutostartDir); VIR_FREE(network_driver->stateDir); diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 1efa0d2af4..95993c5e31 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -34,6 +34,9 @@ struct _virNetworkDriverState { /* Read-only */ bool privileged; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + /* Immutable pointer, self-locking APIs */ virNetworkObjListPtr networks; -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/storage/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/storage/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virstorageobj.h | 3 +++ src/storage/storage_driver.c | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 92d229f9b4..4547a0df9b 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -37,6 +37,9 @@ typedef virStorageDriverState *virStorageDriverStatePtr; struct _virStorageDriverState { virMutex lock; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + virStoragePoolObjListPtr pools; char *configDir; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 496d51b1e0..03ac6a6845 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -43,6 +43,7 @@ #include "virlog.h" #include "virfile.h" #include "virfdstream.h" +#include "virpidfile.h" #include "configmake.h" #include "virsecret.h" #include "virstring.h" @@ -256,6 +257,7 @@ storageStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) return -1; + driver->lockFD = -1; if (virMutexInit(&driver->lock) < 0) { VIR_FREE(driver); return -1; @@ -296,6 +298,11 @@ storageStateInitialize(bool privileged, goto error; } + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", + true, getpid())) < 0) + goto error; + if (virStoragePoolObjLoadAllState(driver->pools, driver->stateDir) < 0) goto error; @@ -371,6 +378,10 @@ storageStateCleanup(void) /* free inactive pools */ virObjectUnref(driver->pools); + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", + driver->lockFD); + VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); VIR_FREE(driver->stateDir); -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/nodedev/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/nodedev/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnodedeviceobj.h | 5 +++++ src/node_device/node_device_hal.c | 31 +++++++++++++++++++++++++++++ src/node_device/node_device_udev.c | 32 ++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 1abfcb9af4..c4d3c55d73 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -37,6 +37,11 @@ typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr; struct _virNodeDeviceDriverState { virMutex lock; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + + char *stateDir; + virNodeDeviceObjListPtr devs; /* currently-known devices */ void *privateData; /* driver-specific private data */ bool privileged; /* whether we run in privileged mode */ diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index d1eb6c7851..876e808dce 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -33,10 +33,13 @@ #include "viralloc.h" #include "viruuid.h" #include "virpci.h" +#include "virpidfile.h" #include "virlog.h" #include "virdbus.h" #include "virstring.h" +#include "configmake.h" + #define VIR_FROM_THIS VIR_FROM_NODEDEV VIR_LOG_INIT("node_device.node_device_hal"); @@ -606,12 +609,36 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, if (VIR_ALLOC(driver) < 0) return -1; + driver->lockFD = -1; if (virMutexInit(&driver->lock) < 0) { VIR_FREE(driver); return -1; } nodeDeviceLock(); + if (privileged) { + if (virAsprintf(&driver->stateDir, + "%s/run/libvirt/nodedev", LOCALSTATEDIR) < 0) + goto failure; + } else { + VIR_AUTOFREE(char *) rundir = NULL; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto failure; + if (virAsprintf(&driver->stateDir, "%s/nodedev/run", rundir) < 0) + goto failure; + } + + if (virFileMakePathWithMode(driver->stateDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + driver->stateDir); + goto failure; + } + + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", true, getpid())) < 0) + goto failure; + if (!(driver->devs = virNodeDeviceObjListNew())) goto failure; @@ -708,6 +735,10 @@ nodeStateCleanup(void) virNodeDeviceObjListFree(driver->devs); (void)libhal_ctx_shutdown(hal_ctx, NULL); (void)libhal_ctx_free(hal_ctx); + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", driver->lockFD); + + VIR_FREE(driver->stateDir); nodeDeviceUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 276bf3dd99..d883462948 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -38,10 +38,13 @@ #include "virbuffer.h" #include "virfile.h" #include "virpci.h" +#include "virpidfile.h" #include "virstring.h" #include "virnetdev.h" #include "virmdev.h" +#include "configmake.h" + #define VIR_FROM_THIS VIR_FROM_NODEDEV VIR_LOG_INIT("node_device.node_device_udev"); @@ -1494,6 +1497,11 @@ nodeStateCleanup(void) virObjectUnref(driver->nodeDeviceEventState); virNodeDeviceObjListFree(driver->devs); + + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", driver->lockFD); + + VIR_FREE(driver->stateDir); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -1810,6 +1818,7 @@ nodeStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) return -1; + driver->lockFD = -1; if (virMutexInit(&driver->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to initialize mutex")); @@ -1819,6 +1828,29 @@ nodeStateInitialize(bool privileged, driver->privileged = privileged; + if (privileged) { + if (virAsprintf(&driver->stateDir, + "%s/run/libvirt/nodedev", LOCALSTATEDIR) < 0) + goto cleanup; + } else { + VIR_AUTOFREE(char *) rundir = NULL; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto cleanup; + if (virAsprintf(&driver->stateDir, "%s/nodedev/run", rundir) < 0) + goto cleanup; + } + + if (virFileMakePathWithMode(driver->stateDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + driver->stateDir); + goto cleanup; + } + + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", true, getpid())) < 0) + goto cleanup; + if (!(driver->devs = virNodeDeviceObjListNew()) || !(priv = udevEventDataNew())) goto cleanup; -- 2.21.0
On 7/10/19 5:47 PM, Daniel P. Berrangé wrote:
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this.
In privileged libvirtd this ends up locking
/var/run/libvirt/nodedev/driver.pid
In unprivileged libvirtd this ends up locking
/run/user/$UID/libvirt/nodedev/run/driver.pid
NB, the latter can vary depending on $XDG_RUNTIME_DIR
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnodedeviceobj.h | 5 +++++ src/node_device/node_device_hal.c | 31 +++++++++++++++++++++++++++++ src/node_device/node_device_udev.c | 32 ++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+)
Side note, isn't it time to finally kill hal backend? Is somebody still using it? Michal
On Wed, Jul 10, 2019 at 07:02:08PM +0200, Michal Privoznik wrote:
On 7/10/19 5:47 PM, Daniel P. Berrangé wrote:
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this.
In privileged libvirtd this ends up locking
/var/run/libvirt/nodedev/driver.pid
In unprivileged libvirtd this ends up locking
/run/user/$UID/libvirt/nodedev/run/driver.pid
NB, the latter can vary depending on $XDG_RUNTIME_DIR
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnodedeviceobj.h | 5 +++++ src/node_device/node_device_hal.c | 31 +++++++++++++++++++++++++++++ src/node_device/node_device_udev.c | 32 ++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+)
Side note, isn't it time to finally kill hal backend? Is somebody still using it?
We were wanting Roman's confirmation that its no longer desired for BSD. https://www.redhat.com/archives/libvir-list/2019-May/msg00207.html Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/interface/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/interface/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/interface/interface_backend_netcf.c | 45 +++++++++++++++++++++++-- src/interface/interface_backend_udev.c | 44 +++++++++++++++++++++++- 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index cf8eb9488d..868e49c56e 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -29,10 +29,14 @@ #include "interface_conf.h" #include "viralloc.h" #include "virlog.h" +#include "virfile.h" +#include "virpidfile.h" #include "virstring.h" #include "viraccessapicheck.h" #include "virinterfaceobj.h" +#include "configmake.h" + #define VIR_FROM_THIS VIR_FROM_INTERFACE VIR_LOG_INIT("interface.interface_backend_netcf"); @@ -43,6 +47,10 @@ VIR_LOG_INIT("interface.interface_backend_netcf"); typedef struct { virObjectLockable parent; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + + char *stateDir; struct netcf *netcf; bool privileged; } virNetcfDriverState, *virNetcfDriverStatePtr; @@ -71,6 +79,11 @@ virNetcfDriverStateDispose(void *obj) if (_driver->netcf) ncf_close(_driver->netcf); + + if (_driver->lockFD != -1) + virPidFileRelease(_driver->stateDir, "driver", _driver->lockFD); + + VIR_FREE(_driver->stateDir); } @@ -87,15 +100,41 @@ netcfStateInitialize(bool privileged, driver->privileged = privileged; + if (privileged) { + if (virAsprintf(&driver->stateDir, + "%s/run/libvirt/nodedev", LOCALSTATEDIR) < 0) + goto error; + } else { + VIR_AUTOFREE(char *) rundir = NULL; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto error; + if (virAsprintf(&driver->stateDir, "%s/nodedev/run", rundir) < 0) + goto error; + } + + if (virFileMakePathWithMode(driver->stateDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + driver->stateDir); + goto error; + } + + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", true, getpid())) < 0) + goto error; + /* open netcf */ if (ncf_init(&driver->netcf, NULL) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to initialize netcf")); - virObjectUnref(driver); - driver = NULL; - return -1; + goto error; } return 0; + + error: + virObjectUnref(driver); + driver = NULL; + return -1; } diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 1373356246..fcd7f1c04a 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -32,14 +32,21 @@ #include "interface_conf.h" #include "viralloc.h" #include "virstring.h" +#include "virpidfile.h" #include "viraccessapicheck.h" #include "virinterfaceobj.h" #include "virnetdev.h" +#include "configmake.h" + #define VIR_FROM_THIS VIR_FROM_INTERFACE struct udev_iface_driver { struct udev *udev; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + + char *stateDir; bool privileged; }; @@ -1157,6 +1164,9 @@ udevInterfaceIsActive(virInterfacePtr ifinfo) } +static int +udevStateCleanup(void); + static int udevStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, @@ -1167,6 +1177,31 @@ udevStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) goto cleanup; + driver->lockFD = -1; + + if (privileged) { + if (virAsprintf(&driver->stateDir, + "%s/run/libvirt/nodedev", LOCALSTATEDIR) < 0) + goto cleanup; + } else { + VIR_AUTOFREE(char *) rundir = NULL; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto cleanup; + if (virAsprintf(&driver->stateDir, "%s/nodedev/run", rundir) < 0) + goto cleanup; + } + + if (virFileMakePathWithMode(driver->stateDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + driver->stateDir); + goto cleanup; + } + + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", true, getpid())) < 0) + goto cleanup; + driver->udev = udev_new(); if (!driver->udev) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1178,6 +1213,8 @@ udevStateInitialize(bool privileged, ret = 0; cleanup: + if (ret < 0) + udevStateCleanup(); return ret; } @@ -1187,8 +1224,13 @@ udevStateCleanup(void) if (!driver) return -1; - udev_unref(driver->udev); + if (driver->udev) + udev_unref(driver->udev); + + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", driver->lockFD); + VIR_FREE(driver->stateDir); VIR_FREE(driver); return 0; } -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/nwfilter/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/nwfilter/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnwfilterobj.h | 4 ++++ src/nwfilter/nwfilter_driver.c | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index bdf5c51fe2..a6bdfb3864 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -36,10 +36,14 @@ struct _virNWFilterDriverState { virMutex lock; bool privileged; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + virNWFilterObjListPtr nwfilters; virNWFilterBindingObjListPtr bindings; + char *stateDir; char *configDir; char *bindingDir; }; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index fdfc6f48fa..43561241f6 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -38,6 +38,7 @@ #include "nwfilter_gentech_driver.h" #include "configmake.h" #include "virfile.h" +#include "virpidfile.h" #include "virstring.h" #include "viraccessapicheck.h" @@ -188,6 +189,7 @@ nwfilterStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) return -1; + driver->lockFD = -1; if (virMutexInit(&driver->lock) < 0) goto err_free_driverstate; @@ -203,6 +205,19 @@ nwfilterStateInitialize(bool privileged, nwfilterDriverLock(); + if (VIR_STRDUP(driver->stateDir, LOCALSTATEDIR "/run/libvirt/nwfilter") < 0) + goto error; + + if (virFileMakePathWithMode(driver->stateDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + driver->stateDir); + goto error; + } + + if ((driver->lockFD = + virPidFileAcquire(driver->stateDir, "driver", true, getpid())) < 0) + goto error; + if (virNWFilterIPAddrMapInit() < 0) goto err_free_driverstate; if (virNWFilterLearnInit() < 0) @@ -346,6 +361,10 @@ nwfilterStateCleanup(void) nwfilterDriverRemoveDBusMatches(); + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", driver->lockFD); + + VIR_FREE(driver->stateDir); VIR_FREE(driver->configDir); VIR_FREE(driver->bindingDir); nwfilterDriverUnlock(); -- 2.21.0
No supported build targets for libvirt still ship xend, so there is no need for the libxl driver to check for it anymore. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 731700ded6..ac10fb6dbc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -525,12 +525,10 @@ libxlStateCleanup(void) static bool libxlDriverShouldLoad(bool privileged) { - bool ret = false; - /* Don't load if non-root */ if (!privileged) { VIR_INFO("Not running privileged, disabling libxenlight driver"); - return ret; + return false; } if (virFileExists(HYPERVISOR_CAPABILITIES)) { @@ -549,31 +547,15 @@ libxlDriverShouldLoad(bool privileged) VIR_INFO("No Xen capabilities detected, probably not running " "in a Xen Dom0. Disabling libxenlight driver"); - return ret; + return false; } } else if (!virFileExists(HYPERVISOR_XENSTORED)) { VIR_INFO("Disabling driver as neither " HYPERVISOR_CAPABILITIES " nor " HYPERVISOR_XENSTORED " exist"); - return ret; + return false; } - /* Don't load if legacy xen toolstack (xend) is in use */ - if (virFileExists("/usr/sbin/xend")) { - virCommandPtr cmd; - - cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL); - if (virCommandRun(cmd, NULL) == 0) { - VIR_INFO("Legacy xen tool stack seems to be in use, disabling " - "libxenlight driver."); - } else { - ret = true; - } - virCommandFree(cmd); - } else { - ret = true; - } - - return ret; + return true; } /* Callbacks wrapping libvirt's event loop interface */ -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/libxl/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/libxl/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_conf.h | 3 +++ src/libxl/libxl_driver.c | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 136b5ae1ac..552f039d2a 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -111,6 +111,9 @@ struct _libxlDriverPrivate { * then lockless thereafter */ libxlDriverConfigPtr config; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + /* Atomic inc/dec only */ unsigned int nactive; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index ac10fb6dbc..a99c7471bb 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -53,6 +53,7 @@ #include "viraccessapicheck.h" #include "viratomic.h" #include "virhostdev.h" +#include "virpidfile.h" #include "locking/domain_lock.h" #include "virnetdevtap.h" #include "cpu/cpu.h" @@ -506,7 +507,6 @@ libxlStateCleanup(void) return -1; virObjectUnref(libxl_driver->hostdevMgr); - virObjectUnref(libxl_driver->config); virObjectUnref(libxl_driver->xmlopt); virObjectUnref(libxl_driver->domains); virPortAllocatorRangeFree(libxl_driver->reservedGraphicsPorts); @@ -516,6 +516,10 @@ libxlStateCleanup(void) virObjectUnref(libxl_driver->domainEventState); virSysinfoDefFree(libxl_driver->hostsysinfo); + if (libxl_driver->lockFD != -1) + virPidFileRelease(libxl_driver->config->stateDir, "driver", libxl_driver->lockFD); + + virObjectUnref(libxl_driver->config); virMutexDestroy(&libxl_driver->lock); VIR_FREE(libxl_driver); @@ -658,6 +662,7 @@ libxlStateInitialize(bool privileged, if (VIR_ALLOC(libxl_driver) < 0) return -1; + libxl_driver->lockFD = -1; if (virMutexInit(&libxl_driver->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); @@ -741,6 +746,10 @@ libxlStateInitialize(bool privileged, goto error; } + if ((libxl_driver->lockFD = + virPidFileAcquire(cfg->stateDir, "driver", true, getpid())) < 0) + goto error; + if (!(libxl_driver->lockManager = virLockManagerPluginNew(cfg->lockManagerName ? cfg->lockManagerName : "nop", -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/lxc/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/lxc/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_conf.h | 3 +++ src/lxc/lxc_driver.c | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index dc5531ebf9..e26ca22d3c 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -70,6 +70,9 @@ struct _virLXCDriver { * then lockless thereafter */ virLXCDriverConfigPtr config; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + /* Require lock to get a reference on the object, * lockless access thereafter */ virCapsPtr caps; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 9db2a02dee..3982c24f34 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1559,6 +1559,7 @@ static int lxcStateInitialize(bool privileged, if (VIR_ALLOC(lxc_driver) < 0) return -1; + lxc_driver->lockFD = -1; if (virMutexInit(&lxc_driver->lock) < 0) { VIR_FREE(lxc_driver); return -1; @@ -1605,6 +1606,10 @@ static int lxcStateInitialize(bool privileged, goto cleanup; } + if ((lxc_driver->lockFD = + virPidFileAcquire(cfg->stateDir, "driver", true, getpid())) < 0) + goto cleanup; + /* Get all the running persistent or transient configs first */ if (virDomainObjListLoadAllConfigs(lxc_driver->domains, cfg->stateDir, @@ -1696,6 +1701,10 @@ static int lxcStateCleanup(void) virObjectUnref(lxc_driver->caps); virObjectUnref(lxc_driver->securityManager); virObjectUnref(lxc_driver->xmlopt); + + if (lxc_driver->lockFD != -1) + virPidFileRelease(lxc_driver->config->stateDir, "driver", lxc_driver->lockFD); + virObjectUnref(lxc_driver->config); virMutexDestroy(&lxc_driver->lock); VIR_FREE(lxc_driver); -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/vz/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/vz/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/vz/vz_driver.c | 40 +++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2286f9a04f..c5152c309c 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -41,6 +41,7 @@ #include "vircommand.h" #include "configmake.h" #include "virfile.h" +#include "virpidfile.h" #include "virstoragefile.h" #include "virstring.h" #include "cpu/cpu.h" @@ -59,8 +60,13 @@ VIR_LOG_INIT("parallels.parallels_driver"); #define PRLCTL "prlctl" +#define VZ_STATEDIR LOCALSTATEDIR "/run/libvirt/vz" + static virClassPtr vzDriverClass; +static bool vz_driver_privileged; +/* pid file FD, ensures two copies of the driver can't use the same root */ +static int vz_driver_lock_fd = -1; static virMutex vz_driver_lock; static vzDriverPtr vz_driver; static vzConnPtr vz_conn_list; @@ -166,6 +172,11 @@ VIR_ONCE_GLOBAL_INIT(vzDriver); vzDriverPtr vzGetDriverConnection(void) { + if (!vz_driver_privileged) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("vz state driver is not active")); + return NULL; + } virMutexLock(&vz_driver_lock); if (!vz_driver) vz_driver = vzDriverObjNew(); @@ -4087,18 +4098,37 @@ static virConnectDriver vzConnectDriver = { static int vzStateCleanup(void) { - virObjectUnref(vz_driver); - vz_driver = NULL; - virMutexDestroy(&vz_driver_lock); - prlsdkDeinit(); + if (vz_driver_privileged) { + virObjectUnref(vz_driver); + vz_driver = NULL; + if (vz_driver_lock_fd != -1) + virPidFileRelease(VZ_STATEDIR, "driver", vz_driver_lock_fd); + virMutexDestroy(&vz_driver_lock); + prlsdkDeinit(); + } return 0; } static int -vzStateInitialize(bool privileged ATTRIBUTE_UNUSED, +vzStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!privileged) + return 0; + + vz_driver_privileged = privileged; + + if (virFileMakePathWithMode(VZ_STATEDIR, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create state directory '%s'"), + VZ_STATEDIR); + return -1; + } + + if ((vz_driver_lock_fd = + virPidFileAcquire(VZ_STATEDIR, "driver", true, getpid())) < 0) + return -1; + if (prlsdkInit() < 0) { VIR_DEBUG("%s", _("Can't initialize Parallels SDK")); return -1; -- 2.21.0
When we allow multiple instances of the driver for the same user account, using a separate root directory, we need to ensure mutual exclusion. Use a pidfile to guarantee this. In privileged libvirtd this ends up locking /var/run/libvirt/bhyve/driver.pid In unprivileged libvirtd this ends up locking /run/user/$UID/libvirt/bhyve/run/driver.pid NB, the latter can vary depending on $XDG_RUNTIME_DIR Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/bhyve/bhyve_driver.c | 9 +++++++++ src/bhyve/bhyve_utils.h | 3 +++ 2 files changed, 12 insertions(+) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 4ce9ef0b95..cfcf4e1fba 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -43,6 +43,7 @@ #include "virthread.h" #include "virlog.h" #include "virfile.h" +#include "virpidfile.h" #include "virtypedparam.h" #include "virrandom.h" #include "virstring.h" @@ -1203,6 +1204,9 @@ bhyveStateCleanup(void) virObjectUnref(bhyve_driver->config); virPortAllocatorRangeFree(bhyve_driver->remotePorts); + if (bhyve_driver->lockFD != -1) + virPidFileRelease(BHYVE_STATE_DIR, "driver", bhyve_driver->lockFD); + virMutexDestroy(&bhyve_driver->lock); VIR_FREE(bhyve_driver); @@ -1222,6 +1226,7 @@ bhyveStateInitialize(bool privileged, if (VIR_ALLOC(bhyve_driver) < 0) return -1; + bhyve_driver->lockFD = -1; if (virMutexInit(&bhyve_driver->lock) < 0) { VIR_FREE(bhyve_driver); return -1; @@ -1274,6 +1279,10 @@ bhyveStateInitialize(bool privileged, goto cleanup; } + if ((bhyve_driver->lockFD = + virPidFileAcquire(BHYVE_STATE_DIR, "driver", true, getpid())) < 0) + goto cleanup; + if (virDomainObjListLoadAllConfigs(bhyve_driver->domains, BHYVE_STATE_DIR, NULL, true, diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h index 26956d7d21..3d212e3ccf 100644 --- a/src/bhyve/bhyve_utils.h +++ b/src/bhyve/bhyve_utils.h @@ -48,6 +48,9 @@ struct _bhyveConn { virBhyveDriverConfigPtr config; + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + virDomainObjListPtr domains; virCapsPtr caps; virDomainXMLOptionPtr xmlopt; -- 2.21.0
On 7/10/19 5:47 PM, Daniel P. Berrangé wrote:
As part of the proposal to introduce an embedded driver feature, we decided we ought to have each driver acquire a lock against the virtual root it is configured to use. This will prevent two apps from running an embedded driver with the same root.
https://www.redhat.com/archives/libvir-list/2019-May/msg00467.html
It turns out that this will also be useful for the per-driver split daemons work I am working on. In that case libvirtd will exist as a monolithic daemon running all drivers, and we'll also have per-driver daemons like virtqemud, virtnetworkd, etc. It is only valid to run in one setup on the host. ie you must choose libvirtd, or choose per-driver daemons, never both.
The per-driver locking will provide very useful protection against mistakes in this respect.
Since the locking is functionally independant of both patch series, I'm sending it now.
Daniel P. Berrangé (12): qemu: acquire a pidfile in the driver root directory secrets: acquire a pidfile in the driver root directory network: acquire a pidfile in the driver root directory storage: acquire a pidfile in the driver root directory nodedev: acquire a pidfile in the driver root directory interface: acquire a pidfile in the driver root directory nwfilter: acquire a pidfile in the driver root directory libxl: remove obsolete check for xend during driver startup libxl: acquire a pidfile in the driver root directory lxc: acquire a pidfile in the driver root directory vz: acquire a pidfile in the driver root directory bhyve: acquire a pidfile in the driver root directory
src/bhyve/bhyve_driver.c | 9 +++++ src/bhyve/bhyve_utils.h | 3 ++ src/conf/virnodedeviceobj.h | 5 +++ src/conf/virnwfilterobj.h | 4 +++ src/conf/virstorageobj.h | 3 ++ src/interface/interface_backend_netcf.c | 45 +++++++++++++++++++++++-- src/interface/interface_backend_udev.c | 44 +++++++++++++++++++++++- src/libxl/libxl_conf.h | 3 ++ src/libxl/libxl_driver.c | 37 ++++++++------------ src/lxc/lxc_conf.h | 3 ++ src/lxc/lxc_driver.c | 9 +++++ src/network/bridge_driver.c | 10 ++++++ src/network/bridge_driver_platform.h | 3 ++ src/node_device/node_device_hal.c | 31 +++++++++++++++++ src/node_device/node_device_udev.c | 32 ++++++++++++++++++ src/nwfilter/nwfilter_driver.c | 19 +++++++++++ src/qemu/qemu_conf.h | 3 ++ src/qemu/qemu_driver.c | 9 +++++ src/secret/secret_driver.c | 44 +++++++++++++++++++----- src/storage/storage_driver.c | 11 ++++++ src/vz/vz_driver.c | 40 +++++++++++++++++++--- 21 files changed, 327 insertions(+), 40 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik