@def is owned by @obj after adding it the node device object list. As soon as the @obj lock has been released, another thread could free @obj and therefore @def. If now someone accesses @def this would lead to a heap-use-after-free and therefore most likely to a segmentation fault, therefore set @def to NULL after the ownership has moved. While at it, add comments to other code places why @def is set to NULL. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 4 ++++ src/test/test_driver.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4730a5b986ca..6613528d0e37 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1588,6 +1588,8 @@ udevAddOneDevice(struct udev_device *device) * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + /* @def is now owned by @obj */ + def = NULL; virNodeDeviceObjSetPersistent(obj, persistent); virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj); @@ -1983,6 +1985,8 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + /* @def is now owned by @obj */ + def = NULL; virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetAutostart(obj, true); virNodeDeviceObjSetPersistent(obj, true); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b656..81b1ba4294bd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7656,8 +7656,9 @@ testNodeDeviceMockCreateVport(testDriver *driver, if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjSetSkipUpdateCaps(obj, true); + /* @def is now owned by @obj */ def = NULL; + virNodeDeviceObjSetSkipUpdateCaps(obj, true); objdef = virNodeDeviceObjGetDef(obj); event = virNodeDeviceEventLifecycleNew(objdef->name, -- 2.34.1

From: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; When a mdev device is destroyed or stopped the udev remove event handling needs to reset the active config data of the node object representing a persisted mdev. Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; --- src/node_device/node_device_driver.h | 3 +++ src/util/virmdev.h | 4 ++++ src/conf/node_device_conf.c | 10 ++-------- src/node_device/node_device_driver.c | 13 +++++++++++++ src/node_device/node_device_udev.c | 1 + src/util/virmdev.c | 20 ++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 7 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b3bc4b2e96ed..f195cfef9d49 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -197,3 +197,6 @@ int nodeDeviceUpdate(virNodeDevice *dev, const char *xmlDesc, unsigned int flags); + +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def); diff --git a/src/util/virmdev.h b/src/util/virmdev.h index 853041ac0619..e8e69040e5d4 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig { size_t nattributes; }; +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config); +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree); + typedef struct _virMediatedDeviceType virMediatedDeviceType; struct _virMediatedDeviceType { char *id; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb72..897c67d6af91 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) g_free(data->sg.path); break; case VIR_NODE_DEV_CAP_MDEV: - g_free(data->mdev.defined_config.type); - g_free(data->mdev.active_config.type); g_free(data->mdev.uuid); - for (i = 0; i < data->mdev.defined_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]); - g_free(data->mdev.defined_config.attributes); - for (i = 0; i < data->mdev.active_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]); - g_free(data->mdev.active_config.attributes); + virMediatedDeviceConfigClear(&data->mdev.defined_config); + virMediatedDeviceConfigClear(&data->mdev.active_config); g_free(data->mdev.parent_addr); break; case VIR_NODE_DEV_CAP_CSS_DEV: diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d99b48138ebf..f623339dc973 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, } +/* A mediated device definition contains data from mdevctl about the active + * device. When the device is deactivated the active configuration data needs + * to be removed. */ +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def) +{ + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return; + + virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config); +} + + int nodeDeviceSetAutostart(virNodeDevice *device, int autostart) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e272d9f1ea2b..921f7806afbd 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1464,6 +1464,7 @@ udevRemoveOneDeviceSysPath(const char *path) if (virNodeDeviceObjIsPersistent(obj)) { VIR_FREE(def->sysfs_path); virNodeDeviceObjSetActive(obj, false); + nodeDeviceDefResetMdevActiveConfig(def); } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 992f3eb1b74c..6ecdbdf0ab77 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -516,6 +516,26 @@ void virMediatedDeviceAttrFree(virMediatedDeviceAttr *attr) g_free(attr); } +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config) +{ + if (!config) + return; + + virMediatedDeviceConfigClear(config); + g_free(config); +} + +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config) +{ + size_t i = 0; + + g_clear_pointer(&config->type, g_free); + for (i = 0; i < config->nattributes; i++) + virMediatedDeviceAttrFree(config->attributes[i]); + config->nattributes = 0; + g_clear_pointer(&config->attributes, g_free); +} + #define MDEV_BUS_DIR "/sys/class/mdev_bus" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 839fe4f545f1..b3c0c576aa60 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2780,6 +2780,8 @@ virMacMapWriteFile; # util/virmdev.h virMediatedDeviceAttrFree; virMediatedDeviceAttrNew; +virMediatedDeviceConfigClear; +virMediatedDeviceConfigFree; virMediatedDeviceFree; virMediatedDeviceGetIOMMUGroupDev; virMediatedDeviceGetIOMMUGroupNum; -- 2.34.1

It is done a little differently everywhere in libvirt, but most common is to test for != -1. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 945cfea0851e..616f98885a60 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,7 +88,7 @@ udevEventDataDispose(void *obj) if (priv->watch != -1) virEventRemoveHandle(priv->watch); - if (priv->mdevctlTimeout > 0) + if (priv->mdevctlTimeout != -1) virEventRemoveTimeout(priv->mdevctlTimeout); if (!priv->udev_monitor) @@ -139,6 +139,7 @@ udevEventDataNew(void) return NULL; } + ret->mdevctlTimeout = -1; ret->watch = -1; return ret; } @@ -2082,7 +2083,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) udevEventData *priv = opaque; virThread thread; - if (priv->mdevctlTimeout > 0) { + if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; } @@ -2192,7 +2193,7 @@ scheduleMdevctlUpdate(udevEventData *data, bool force) { if (!force) { - if (data->mdevctlTimeout > 0) + if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, data, NULL); -- 2.34.1

Since @driver->privateData is modified take the lock. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index c2e6c593709b..ee96a8a6c92b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2232,7 +2232,9 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * configuration change, try to coalesce these changes by waiting for the * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + } } -- 2.34.1

Inline `udevRemoveOneDevice` as it's used only once. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 5be09aeb23d6..0ad83efc86a8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1485,16 +1485,6 @@ udevRemoveOneDeviceSysPath(const char *path) return 0; } - -static int -udevRemoveOneDevice(struct udev_device *device) -{ - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(path); -} - - static int udevSetParent(struct udev_device *device, virNodeDeviceDef *def) @@ -1778,8 +1768,11 @@ udevHandleOneDevice(struct udev_device *device) if (STREQ(action, "add") || STREQ(action, "change")) return udevAddOneDevice(device); - if (STREQ(action, "remove")) - return udevRemoveOneDevice(device); + if (STREQ(action, "remove")) { + const char *path = udev_device_get_syspath(device); + + return udevRemoveOneDeviceSysPath(path); + } if (STREQ(action, "move")) { const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); -- 2.34.1

Even if `priv->udev_monitor` was never initialized, the mdevctlLock, udevThread were. Therefore let's match the order of releasing the resources the order of allocating the resources in `nodeStateInitialize`. In addition, use `g_steal_pointer` in `g_list_free_full`. Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9aab4340fa2d..906b16778eef 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,6 +88,10 @@ udevEventDataDispose(void *obj) g_clear_pointer(&priv->initThread, g_free); + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); + } + if (priv->watch != -1) virEventRemoveHandle(priv->watch); @@ -96,16 +100,12 @@ udevEventDataDispose(void *obj) g_clear_pointer(&priv->udevThread, g_free); - if (!priv->udev_monitor) - return; - - udev = udev_monitor_get_udev(priv->udev_monitor); - udev_monitor_unref(priv->udev_monitor); - udev_unref(udev); - - VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { - g_list_free_full(priv->mdevctlMonitors, g_object_unref); + if (priv->udev_monitor) { + udev = udev_monitor_get_udev(priv->udev_monitor); + udev_monitor_unref(priv->udev_monitor); + udev_unref(udev); } + virMutexDestroy(&priv->mdevctlLock); virCondDestroy(&priv->udevThreadCond); -- 2.34.1

The documentation of gobject signals reads: "If you are connecting handlers to signals and using a GObject instance as your signal handler user data, you should remember to pair calls to g_signal_connect() with calls to g_signal_handler_disconnect() or g_signal_handlers_disconnect_by_func(). While signal handlers are automatically disconnected when the object emitting the signal is finalised..." [1] This means that the signal handlers are automatically disconnected as soon as the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`. But this also means that it's possible that new work is tried to be scheduled for the workerpool by the `mdevctlEventHandleCallback` (main thread context) even if the workerpool has already been stopped by `nodeStateShutdownWait`. To fully understand this, it's important to know that the main loop of the main thread is still running for some time even after `nodeStateShutdownPrepare` has been called. Let's avoid this situation by explicitly disconnect the signals during `nodeStateShutdownPrepare`, which is called in the main thread, so that no new work is attempted to be scheduled for the worker pool. [1] https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-han... Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b2c5d11acbdf..2c9c7466da4a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2226,6 +2226,12 @@ nodeStateShutdownPrepare(void) if (!priv) return 0; + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + GList *tmp; + for (tmp = priv->mdevctlMonitors; tmp; tmp = tmp->next) + g_signal_handlers_disconnect_by_data(tmp->data, priv); + } + VIR_WITH_OBJECT_LOCK_GUARD(priv) { if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); -- 2.34.1

Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events. Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_driver.c | 9 +- src/node_device/node_device_udev.c | 241 +++++++++++++++++++-------- src/test/test_driver.c | 8 +- 3 files changed, 185 insertions(+), 73 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 59c5f9b417a4..a51537d87ceb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1421,10 +1421,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) goto cleanup; /* Because we're about to release the lock and thus run into a race - * possibility (however improbable) with a udevAddOneDevice change - * event which would essentially free the existing @def (obj->def) and - * replace it with something new, we need to grab the parent field - * and then find the parent obj in order to manage the vport */ + * possibility (however improbable) with a + * processNodeDeviceAddAndChangeEvent change event which would + * essentially free the existing @def (obj->def) and replace it with + * something new, we need to grab the parent field and then find the + * parent obj in order to manage the vport */ parent = g_strdup(def->parent); virNodeDeviceObjEndAPI(&obj); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4f8dae3f85c8..1f7123a5fafa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -43,6 +43,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include "virthreadpool.h" #include "configmake.h" @@ -69,13 +70,13 @@ struct _udevEventData { bool udevThreadQuit; bool udevDataReady; - /* init thread */ - virThread *initThread; - /* Protects @mdevctlMonitors */ virMutex mdevctlLock; GList *mdevctlMonitors; int mdevctlTimeout; + + /* Immutable pointer, self-locking APIs */ + virThreadPool *workerPool; }; static virClass *udevEventDataClass; @@ -86,8 +87,6 @@ udevEventDataDispose(void *obj) struct udev *udev = NULL; udevEventData *priv = obj; - g_clear_pointer(&priv->initThread, g_free); - VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); } @@ -100,6 +99,8 @@ udevEventDataDispose(void *obj) udev_unref(udev); } + g_clear_pointer(&priv->workerPool, virThreadPoolFree); + virMutexDestroy(&priv->mdevctlLock); virCondDestroy(&priv->udevThreadCond); @@ -143,6 +144,66 @@ udevEventDataNew(void) return ret; } +typedef enum { + NODE_DEVICE_EVENT_INIT = 0, + NODE_DEVICE_EVENT_UDEV_ADD, + NODE_DEVICE_EVENT_UDEV_REMOVE, + NODE_DEVICE_EVENT_UDEV_CHANGE, + NODE_DEVICE_EVENT_UDEV_MOVE, + NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED, + + NODE_DEVICE_EVENT_LAST +} nodeDeviceEventType; + +struct _nodeDeviceEvent { + nodeDeviceEventType eventType; + void *data; + virFreeCallback dataFreeFunc; +}; +typedef struct _nodeDeviceEvent nodeDeviceEvent; + +static void +nodeDeviceEventFree(nodeDeviceEvent *event) +{ + if (!event) + return; + + if (event->dataFreeFunc) + event->dataFreeFunc(event->data); + g_free(event); +} +G_DEFINE_AUTOPTR_CLEANUP_FUNC(nodeDeviceEvent, nodeDeviceEventFree); + + /** + * nodeDeviceEventSubmit: + * @eventType: the event to be processed + * @data: additional data for the event processor (the pointer is stolen and it + * will be properly freed using @dataFreeFunc) + * @dataFreeFunc: callback to free @data + * + * Submits @eventType to be processed by the asynchronous event handling + * thread. + */ +static int nodeDeviceEventSubmit(nodeDeviceEventType eventType, void *data, virFreeCallback dataFreeFunc) +{ + nodeDeviceEvent *event = g_new0(nodeDeviceEvent, 1); + udevEventData *priv = NULL; + + if (!driver) + return -1; + + priv = driver->privateData; + + event->eventType = eventType; + event->data = data; + event->dataFreeFunc = dataFreeFunc; + if (virThreadPoolSendJob(priv->workerPool, 0, event) < 0) { + nodeDeviceEventFree(event); + return -1; + } + return 0; +} + static bool udevHasDeviceProperty(struct udev_device *dev, @@ -1446,8 +1507,8 @@ udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, static int -udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, - const char *path) +processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, + const char *path) { virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; @@ -1529,8 +1590,8 @@ udevSetParent(virNodeDeviceDriverState *driver_state, } static int -udevAddOneDevice(virNodeDeviceDriverState *driver_state, - struct udev_device *device) +processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, + struct udev_device *device) { g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1643,7 +1704,7 @@ udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, device = udev_device_new_from_syspath(udev, name); if (device != NULL) { - if (udevAddOneDevice(driver_state, device) != 0) { + if (processNodeDeviceAddAndChangeEvent(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1752,26 +1813,23 @@ udevHandleOneDevice(struct udev_device *device) VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); - if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(driver, device); - - if (STREQ(action, "remove")) { - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(driver, path); - } - - if (STREQ(action, "move")) { - const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); - - if (devpath_old) { - g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); - - udevRemoveOneDeviceSysPath(driver, devpath_old_fixed); - } - - return udevAddOneDevice(driver, device); + /* Reference is either released via workerpool logic or at the end of this + * function. */ + device = udev_device_ref(device); + if (STREQ(action, "add")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_ADD, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "change")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_CHANGE, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "remove")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_REMOVE, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "move")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_MOVE, device, + (virFreeCallback)udev_device_unref); } + udev_device_unref(device); return 0; } @@ -1990,23 +2048,24 @@ udevSetupSystemDev(void) static void -nodeStateInitializeEnumerate(void *opaque) +processNodeStateInitializeEnumerate(virNodeDeviceDriverState *driver_state, + void *opaque) { struct udev *udev = opaque; - udevEventData *priv = driver->privateData; + udevEventData *priv = driver_state->privateData; /* Populate with known devices */ - if (udevEnumerateDevices(driver, udev) != 0) + if (udevEnumerateDevices(driver_state, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices(driver) != 0) + if (nodeDeviceUpdateMediatedDevices(driver_state) != 0) goto error; cleanup: - VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - driver->initialized = true; - virCondBroadcast(&driver->initCond); + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + driver_state->initialized = true; + virCondBroadcast(&driver_state->initCond); } return; @@ -2048,31 +2107,16 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void -mdevctlUpdateThreadFunc(void *opaque) -{ - virNodeDeviceDriverState *driver_state = opaque; - - if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) - VIR_WARN("mdevctl failed to update mediated devices"); -} - - -static void -launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) +submitMdevctlUpdate(int timer G_GNUC_UNUSED, void *opaque) { udevEventData *priv = opaque; - virThread thread; if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; } - if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, driver) < 0) { - virReportSystemError(errno, "%s", - _("failed to create mdevctl thread")); - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED, NULL, NULL); } @@ -2167,7 +2211,7 @@ mdevctlEnableMonitor(udevEventData *priv) /* Schedules an mdevctl update for 100ms in the future, canceling any existing * timeout that may have been set. In this way, multiple update requests in * quick succession can be collapsed into a single update. if @force is true, - * an update thread will be spawned immediately. */ + * the worker job is submitted immediately. */ static void scheduleMdevctlUpdate(udevEventData *data, bool force) @@ -2175,12 +2219,12 @@ scheduleMdevctlUpdate(udevEventData *data, if (!force) { if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); return; } - launchMdevctlUpdateThread(-1, data); + submitMdevctlUpdate(-1, data); } @@ -2220,6 +2264,62 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, } +static void nodeDeviceEventHandler(void *data, void *opaque) +{ + virNodeDeviceDriverState *driver_state = opaque; + g_autoptr(nodeDeviceEvent) processEvent = data; + + switch (processEvent->eventType) { + case NODE_DEVICE_EVENT_INIT: + { + struct udev *udev = processEvent->data; + + processNodeStateInitializeEnumerate(driver_state, udev); + } + break; + case NODE_DEVICE_EVENT_UDEV_ADD: + case NODE_DEVICE_EVENT_UDEV_CHANGE: + { + struct udev_device *device = processEvent->data; + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_UDEV_REMOVE: + { + struct udev_device *device = processEvent->data; + const char *path = udev_device_get_syspath(device); + + processNodeDeviceRemoveEvent(driver_state, path); + } + break; + case NODE_DEVICE_EVENT_UDEV_MOVE: + { + struct udev_device *device = processEvent->data; + const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); + + if (devpath_old) { + g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); + + processNodeDeviceRemoveEvent(driver_state, devpath_old_fixed); + } + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED: + { + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); + } + break; + case NODE_DEVICE_EVENT_LAST: + g_assert_not_reached(); + break; + } +} + + /* Note: It must be safe to call this function even if the driver was not * successfully initialized. This must be considered when changing this * function. */ @@ -2255,6 +2355,9 @@ nodeStateShutdownPrepare(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } + + if (priv->workerPool) + virThreadPoolStop(priv->workerPool); return 0; } @@ -2275,11 +2378,12 @@ nodeStateShutdownWait(void) return 0; VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); if (priv->udevThread) virThreadJoin(priv->udevThread); } + + if (priv->workerPool) + virThreadPoolDrain(priv->workerPool); return 0; } @@ -2350,6 +2454,19 @@ nodeStateInitialize(bool privileged, driver->parserCallbacks.postParse = nodeDeviceDefPostParse; driver->parserCallbacks.validate = nodeDeviceDefValidate; + /* With the current design, we can only have exactly *one* worker thread as + * otherwise we cannot guarantee that the 'order(udev_events) == + * order(nodedev_events)' is preserved. The worker pool must be initialized + * before trying to reconnect to all the running mdevs since there might + * occur some mdevctl monitor events that will be dispatched to the worker + * pool. */ + priv->workerPool = virThreadPoolNewFull(1, 1, 0, nodeDeviceEventHandler, + "nodev-device-event", + NULL, + driver); + if (!priv->workerPool) + goto unlock; + if (udevPCITranslateInit(privileged) < 0) goto unlock; @@ -2407,13 +2524,7 @@ nodeStateInitialize(bool privileged, if (udevSetupSystemDev() != 0) goto cleanup; - priv->initThread = g_new0(virThread, 1); - if (virThreadCreateFull(priv->initThread, true, nodeStateInitializeEnumerate, - "nodedev-init", false, udev) < 0) { - virReportSystemError(errno, "%s", - _("failed to create udev enumerate thread")); - goto cleanup; - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_INIT, udev_ref(udev), (virFreeCallback)udev_unref); return VIR_DRV_STATE_INIT_COMPLETE; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 81b1ba4294bd..76f89a224f21 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7750,10 +7750,10 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - /* Unlike the real code we cannot run into the udevAddOneDevice race - * which would replace obj->def, so no need to save off the parent, - * but do need to drop the @obj lock so that the FindByName code doesn't - * deadlock on ourselves */ + /* Unlike the real code we cannot run into the + * processNodeDeviceAddAndChangeEvent race which would replace obj->def, so + * no need to save off the parent, but do need to drop the @obj lock so that + * the FindByName code doesn't deadlock on ourselves */ virObjectUnlock(obj); /* We do this just for basic validation and throw away the parentobj -- 2.34.1

Use this feature in `udevEventDataNew`. Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 73b71607489f..8687be942722 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -78,6 +78,7 @@ struct _udevEventData { /* Immutable pointer, self-locking APIs */ virThreadPool *workerPool; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(udevEventData, virObjectUnref); static virClass *udevEventDataClass; @@ -121,7 +122,7 @@ VIR_ONCE_GLOBAL_INIT(udevEventData); static udevEventData * udevEventDataNew(void) { - udevEventData *ret = NULL; + g_autoptr(udevEventData) ret = NULL; if (udevEventDataInitialize() < 0) return NULL; @@ -129,19 +130,15 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL; - if (virCondInit(&ret->udevThreadCond) < 0) { - virObjectUnref(ret); + if (virCondInit(&ret->udevThreadCond) < 0) return NULL; - } - if (virMutexInit(&ret->mdevctlLock) < 0) { - virObjectUnref(ret); + if (virMutexInit(&ret->mdevctlLock) < 0) return NULL; - } ret->mdevctlTimeout = -1; ret->watch = -1; - return ret; + return g_steal_pointer(&ret); } typedef enum { -- 2.34.1

Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 14d44d5bcd0e..cc725997a39e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -791,7 +791,7 @@ udevGetSCSIType(virNodeDeviceDef *def G_GNUC_UNUSED, static int -udevProcessSCSIDevice(struct udev_device *device G_GNUC_UNUSED, +udevProcessSCSIDevice(struct udev_device *device, virNodeDeviceDef *def) { int ret = -1; -- 2.34.1

Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; On 4/23/24 8:09 PM, Marc Hartmayer wrote: -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294