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

[Erik Skultety]

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 at 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?

Erik