On Wed, 26 Aug 2020 07:48:15 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Tue, Aug 25, 2020 at 03:59:47PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 25, 2020 at 04:55:06PM +0200, Erik Skultety wrote:
On Tue, Aug 25, 2020 at 01:44:40PM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 18, 2020 at 09:47:59AM -0500, Jonathon Jongsma wrote:
We need to peridocally query mdevctl for changes to device definitions since an administrator can define new devices with mdevctl outside of libvirt.
In order to do this, a new thread is created to handle the querying. In the future, mdevctl may add a way to signal device add / remove via events, but for now we resort to a bit of a workaround: monitoring the mdevctl config directories for changes to files. When a change is detected, we query mdevctl and update our device list.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_udev.c | 182 +++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e06584a3dc..a85418e549 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -41,6 +41,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include <gio/gio.h>
#include "configmake.h"
@@ -66,6 +67,11 @@ struct _udevEventData { virCond threadCond; bool threadQuit; bool dataReady; + + virThread mdevctlThread; + virCond mdevctlCond; + bool mdevctlReady; + GList *mdevctl_monitors; };
static virClassPtr udevEventDataClass; @@ -85,8 +91,10 @@ udevEventDataDispose(void *obj) udev = udev_monitor_get_udev(priv->udev_monitor); udev_monitor_unref(priv->udev_monitor); udev_unref(udev); + g_list_free_full(priv->mdevctl_monitors, g_object_unref);
virCondDestroy(&priv->threadCond); + virCondDestroy(&priv->mdevctlCond); }
@@ -117,6 +125,11 @@ udevEventDataNew(void) return NULL; }
+ if (virCondInit(&ret->mdevctlCond) < 0) { + virObjectUnref(ret); + return NULL; + } + ret->watch = -1; return ret; } @@ -1520,6 +1533,7 @@ nodeStateCleanup(void) virObjectLock(priv); priv->threadQuit = true; virCondSignal(&priv->threadCond); + virCondSignal(&priv->mdevctlCond); virObjectUnlock(priv); virThreadJoin(&priv->th); } @@ -1601,6 +1615,40 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv, }
+/* Thread to query mdevctl for the current state of persistent mediated device + * defintions when any changes are detected */ +static +void mdevctlThread(void *opaque G_GNUC_UNUSED) +{ + udevEventDataPtr priv = driver->privateData; + + while (1) { + virObjectLock(priv); + while (!priv->mdevctlReady && !priv->threadQuit) { + if (virCondWait(&priv->mdevctlCond, &priv->parent.lock)) { + virReportSystemError(errno, "%s", + _("handler failed to wait on condition")); + virObjectUnlock(priv); + return; + } + } + + if (priv->threadQuit) { + virObjectUnlock(priv); + return; + } + + virObjectUnlock(priv); + + mdevctlEnumerateDevices(); + + virObjectLock(priv); + priv->mdevctlReady = false; + virObjectUnlock(priv); + } +} + + /** * udevEventHandleThread * @opaque: unused @@ -1708,6 +1756,69 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, }
+static int timeout_id;
Please keep driver state in the driver state struct.
+ +static gboolean +signalMdevctlThread(void *user_data) +{ + udevEventDataPtr priv = user_data; + + if (timeout_id) { + g_source_remove(timeout_id); + timeout_id = 0; + } + + virObjectLock(priv); + priv->mdevctlReady = true; + virCondSignal(&priv->mdevctlCond); + virObjectUnlock(priv); + + return G_SOURCE_REMOVE; +}
I don't see why we need to have a timer that then signals a persistent thread. Why cant this just spawn a throw-away thread to do the work and avoid all the conditional signaling complexity.
We sure don't - in that case we'd have to introduce another lock in mdevctlEnumerateDevices to serialize the threads so that the updates to the devices are guaranteed to be performed in the order the events come. Just a question, if we spawn a thread for each event, can't we by any chance theoretically DOS the host system if there's a malicious process creating/removing/stating files?
Yes, it can but the code is already delaying and merging processing of the events. Also the only person able to inflict such a DoS is root themselves, so I don't think there's risk from malicious attacks here, just broken apps.
It's true that /etc/mdevctl.d is owned by root:root, so only a misconfiguration or a broken app like you say could exploit this with the former being the case for libvirt as well. Okay, individual threads it is then.
Sorry it took a little while to get back to this patch, but I don't really see how spawning throw-away threads makes this much simpler. As Erik mentions, if we spawn new threads for each event, we will need a mechanism to serialize these threads and maintain coherent ordering of events, etc. I agree that the DoS issue isn't something that we need to protect against, since it require root access to exploit, but the ordering issue does seem important. There is already a persistent thread to handle udev events. What if I just add the mdevctl functionality to this existing thread? Jonathon