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@xxxxxxxxxx>
> > > ---
> > > 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.
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 :|
