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

[Jonathon Jongsma]

On Wed, 26 Aug 2020 07:48:15 +0200
Erik Skultety <eskultet at 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 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?  
> >
> > 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