Re: [libvirt PATCH v2 09/16] nodedev: add an mdevctl thread

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(a)redhat.com&gt; > > > > --- > > > > 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.