[PATCH 0/5] nodedev: Couple of fixes after last mdevctl patches
Recently, the nodedev driver gained ability to define (some type of) devices. However, it uncovered some dormant bugs we had (patches 2/5 and 3/5) and also introduced some (patches 1/5 and 5/5). I've came across these problems because I don't have /etc/mdevctl.d on my machine. Michal Prívozník (5): nodedev: Unlock @priv if initialization of mdevctlMonitors fails nodedev: Lock @priv sooner nodedev: Don't join not spawned threads nodedev: Separate mdevctl monitor setup into a function nodedev: Only set up mdevctl monitors if mdevctl.d exist src/node_device/node_device_udev.c | 75 ++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 24 deletions(-) -- 2.26.3
If initialization of priv->mdevctlMonitors fails, then the control jumps over to cleanup label where nodeStateCleanup() is called which tries to lock @priv. But since @priv was already locked before taking the jump a deadlock occurs. The solution is to jump onto @unlock label, just like the code around is doing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 7d4e8f5c0b..04e1094e21 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2274,7 +2274,7 @@ nodeStateInitialize(bool privileged, if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, mdevctlConfigDir))) { virMutexUnlock(&priv->mdevctlLock); - goto cleanup; + goto unlock; } virMutexUnlock(&priv->mdevctlLock); -- 2.26.3
The nodedev driver private data object @priv is created by calling udevEventDataNew(). After that, driver->privateData pointer is set to the freshly allocated object and only a few lines after all of this the object is locked. Technically it is safe because there should not be any other thread at this point, but defensive style of programming says it's better if the object is locked before driver's privateData is set. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 04e1094e21..da3754ff80 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2212,21 +2212,21 @@ nodeStateInitialize(bool privileged, !(priv = udevEventDataNew())) goto cleanup; + virObjectLock(priv); + driver->privateData = priv; driver->nodeDeviceEventState = virObjectEventStateNew(); if (udevPCITranslateInit(privileged) < 0) - goto cleanup; + goto unlock; udev = udev_new(); if (!udev) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create udev context")); - goto cleanup; + goto unlock; } - virObjectLock(priv); - priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (!priv->udev_monitor) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 2.26.3
During the nodedev driver initialization two threads are created: one for listening on udev events (like device plug/unplug) and the other for enumerating devices (so that the main thread doing the driver init is not blocked). If something goes wrong at any point then nodeStateCleanup() is called which joins those two threads (possibly) created before. But it tries to join them even they weren't created which is undefined behaviour (and it just so happens that it crashes on my system). If those two virThread variables are turned into pointers then we can use comparison against NULL to detect whether threads were created. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index da3754ff80..3e9c7a8d80 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -63,13 +63,13 @@ struct _udevEventData { int watch; /* Thread data */ - virThread th; + virThread *th; virCond threadCond; bool threadQuit; bool dataReady; /* init thread */ - virThread initThread; + virThread *initThread; GList *mdevctlMonitors; virMutex mdevctlLock; @@ -1685,8 +1685,14 @@ nodeStateCleanup(void) priv->threadQuit = true; virCondSignal(&priv->threadCond); virObjectUnlock(priv); - virThreadJoin(&priv->initThread); - virThreadJoin(&priv->th); + if (priv->initThread) { + virThreadJoin(priv->initThread); + g_clear_pointer(&priv->initThread, g_free); + } + if (priv->th) { + virThreadJoin(priv->th); + g_clear_pointer(&priv->th, g_free); + } } virObjectUnref(priv); @@ -2243,10 +2249,12 @@ nodeStateInitialize(bool privileged, udev_monitor_set_receive_buffer_size(priv->udev_monitor, 128 * 1024 * 1024); - if (virThreadCreateFull(&priv->th, true, udevEventHandleThread, + priv->th = g_new0(virThread, 1); + if (virThreadCreateFull(priv->th, true, udevEventHandleThread, "udev-event", false, NULL) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); + g_clear_pointer(&priv->th, g_free); goto unlock; } @@ -2284,10 +2292,12 @@ nodeStateInitialize(bool privileged, if (udevSetupSystemDev() != 0) goto cleanup; - if (virThreadCreateFull(&priv->initThread, true, nodeStateInitializeEnumerate, + 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")); + g_clear_pointer(&priv->initThread, g_free); goto cleanup; } -- 2.26.3
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 37 +++++++++++++++++++----------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 3e9c7a8d80..18219175c5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2119,6 +2119,29 @@ monitorFileRecursively(udevEventData *udev, } +static int +mdevctlEnableMonitor(udevEventData *priv) +{ + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); + + /* mdevctl may add notification events in the future: + * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to + * monitoring the mdevctl configuration directory for changes. + * mdevctl configuration is stored in a directory tree within + * /etc/mdevctl.d/. There is a directory for each parent device, which + * contains a file defining each mediated device */ + virMutexLock(&priv->mdevctlLock); + if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, + mdevctlConfigDir))) { + virMutexUnlock(&priv->mdevctlLock); + return -1; + } + virMutexUnlock(&priv->mdevctlLock); + + return 0; +} + + static void mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, GFile *file, @@ -2168,7 +2191,6 @@ nodeStateInitialize(bool privileged, { udevEventDataPtr priv = NULL; struct udev *udev = NULL; - g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2272,19 +2294,8 @@ nodeStateInitialize(bool privileged, if (priv->watch == -1) goto unlock; - /* mdevctl may add notification events in the future: - * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to - * monitoring the mdevctl configuration directory for changes. - * mdevctl configuration is stored in a directory tree within - * /etc/mdevctl.d/. There is a directory for each parent device, which - * contains a file defining each mediated device */ - virMutexLock(&priv->mdevctlLock); - if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, - mdevctlConfigDir))) { - virMutexUnlock(&priv->mdevctlLock); + if (mdevctlEnableMonitor(priv) < 0) goto unlock; - } - virMutexUnlock(&priv->mdevctlLock); virObjectUnlock(priv); -- 2.26.3
During its initialization, the nodedev driver tries to set up monitors for /etc/mdevctl.d directory, so that it can register mdevs as they come and go. However, if the file doesn't exist there is nothing to monitor and therefore we can exit early. In fact, we have to otherwise monitorFileRecursively() fails and whole driver initialization fails with it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 18219175c5..c4040c2fd6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2122,7 +2122,13 @@ monitorFileRecursively(udevEventData *udev, static int mdevctlEnableMonitor(udevEventData *priv) { - g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); + g_autoptr(GFile) mdevctlConfigDir = NULL; + const char *mdevctlDir = "/etc/mdevctl.d"; + + if (!virFileExists(mdevctlDir)) + return 0; + + mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); /* mdevctl may add notification events in the future: * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to -- 2.26.3
On Mon, Apr 12, 2021 at 06:22:24PM +0200, Michal Privoznik wrote:
During its initialization, the nodedev driver tries to set up monitors for /etc/mdevctl.d directory, so that it can register mdevs as they come and go. However, if the file doesn't exist there is nothing to monitor and therefore we can exit early. In fact, we have to otherwise monitorFileRecursively() fails and whole driver initialization fails with it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 18219175c5..c4040c2fd6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2122,7 +2122,13 @@ monitorFileRecursively(udevEventData *udev, static int mdevctlEnableMonitor(udevEventData *priv) { - g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); + g_autoptr(GFile) mdevctlConfigDir = NULL; + const char *mdevctlDir = "/etc/mdevctl.d"; + + if (!virFileExists(mdevctlDir)) + return 0; + + mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d");
s|"/etc/mdevctl.d"|mdevctlDir|
/* mdevctl may add notification events in the future: * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to -- 2.26.3
On Mon, Apr 12, 2021 at 06:22:19PM +0200, Michal Privoznik wrote:
Recently, the nodedev driver gained ability to define (some type of) devices. However, it uncovered some dormant bugs we had (patches 2/5 and 3/5) and also introduced some (patches 1/5 and 5/5).
I've came across these problems because I don't have /etc/mdevctl.d on my machine.
Michal Prívozník (5): nodedev: Unlock @priv if initialization of mdevctlMonitors fails nodedev: Lock @priv sooner nodedev: Don't join not spawned threads nodedev: Separate mdevctl monitor setup into a function nodedev: Only set up mdevctl monitors if mdevctl.d exist
With the comment for last patch fixed: Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Michal Privoznik -
Pavel Hrdina