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;
+
+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;
+}
+
+
+static void
+mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
+ GFile *file,
+ GFile *other_file G_GNUC_UNUSED,
+ GFileMonitorEvent event_type,
+ gpointer user_data)
+{
+ udevEventDataPtr priv = user_data;
+
+ /* if a new directory appears, monitor that directory for changes */
+ if (g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) ==
+ G_FILE_TYPE_DIRECTORY) {
+ GFileMonitor *mon;
+
+ if ((mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, NULL))) {
+ g_signal_connect(mon, "changed",
+ G_CALLBACK(mdevctlEventHandleCallback),
+ user_data);
+ virObjectLock(priv);
+ priv->mdevctl_monitors = g_list_append(priv->mdevctl_monitors, mon);
+ virObjectUnlock(priv);
+ }
+ }
+
+ /* Sometimes a single configuration change results in multiple notify
+ * events (e.g. several CHANGED events, or a CREATED and CHANGED event
+ * followed by CHANGES_DONE_HINT). To avoid triggering the mdevctl thread
+ * multiple times for a single configuration change, try to coalesce these
+ * changes by waiting for the CHANGES_DONE_HINT event. As a fallback, add
+ * a timeout to trigger the signal if that event never comes */
+ if (event_type == G_FILE_MONITOR_EVENT_CREATED ||
+ event_type == G_FILE_MONITOR_EVENT_CHANGED) {
+ if (timeout_id)
+ g_source_remove(timeout_id);
+ timeout_id = g_timeout_add(100, signalMdevctlThread, priv);
+ return;
+ }
+
+ signalMdevctlThread(priv);
+}
+
+
/* DMI is intel-compatible specific */
#if defined(__x86_64__) || defined(__i386__) || defined(__amd64__)
static void
@@ -1858,6 +1969,58 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
}
+/* Recursively monitors dir and any subdirectory for file changes and returns a
+ * GList of GFileMonitor objects */
+static GList*
+monitorDirRecursively(GFile *dir,
+ GCallback changed_cb,
+ gpointer user_data)
+{
+ GList *monitors = NULL;
+ g_autoptr(GFileInfo) dirinfo = NULL;
+ g_autoptr(GError) error = NULL;
+ g_autoptr(GFileEnumerator) children = NULL;
+ GFileMonitor *mon;
+
+ if (!(children = g_file_enumerate_children(dir, "standard::*",
+ G_FILE_QUERY_INFO_NONE, NULL, &error))) {
+ if (error->code == G_IO_ERROR_NOT_DIRECTORY)
+ return NULL;
+ goto bail;
+ }
+
+ if (!(mon = g_file_monitor(dir, G_FILE_MONITOR_NONE, NULL, &error)))
+ goto bail;
+
+ g_signal_connect(mon, "changed", changed_cb, user_data);
+ monitors = g_list_append(monitors, mon);
+
+ while (true) {
+ GFileInfo *info;
+ GFile *child;
+ GList *child_monitors = NULL;
+
+ if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error))
+ goto bail;
+ if (!info)
+ break;
+
+ child_monitors = monitorDirRecursively(child, changed_cb, user_data);
+ if (child_monitors)
+ monitors = g_list_concat(monitors, child_monitors);
+ }
+
+ return monitors;
+
+ bail:
+ if (error)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to monitor directory: %s"), error->message);
+
+ g_list_free_full(monitors, g_object_unref);
+ return NULL;
+}
+
static int
nodeStateInitialize(bool privileged,
const char *root,
@@ -1867,6 +2030,8 @@ nodeStateInitialize(bool privileged,
udevEventDataPtr priv = NULL;
struct udev *udev = NULL;
virThread enumThread;
+ g_autoptr(GError) error = NULL;
+ g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d");
if (root != NULL) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -1969,6 +2134,23 @@ nodeStateInitialize(bool privileged,
if (priv->watch == -1)
goto unlock;
+ if (virThreadCreateFull(&priv->mdevctlThread, true, mdevctlThread,
+ "mdevctl-event", false, NULL) < 0) {
+ virReportSystemError(errno, "%s",
+ _("failed to create mdevctl handler thread"));
+ goto unlock;
+ }
+
+ /* mdevctl may add notification events in the future:
+ * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to
+ * monitoring the mdevctl configuration directory for changes.
+ * mdevctl configuration is stored in a directory tree within
+ * /etc/mdevctl.d/. There is a directory for each parent device, which
+ * contains a file defining each mediated device */
+ priv->mdevctl_monitors = monitorDirRecursively(mdevctlConfigDir,
+ G_CALLBACK(mdevctlEventHandleCallback),
+ priv);
+
virObjectUnlock(priv);
/* Create a fictional 'computer' device to root the device tree. */
--
2.26.2
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.
> +
> +
> +static void
> +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
> + GFile *file,
> + GFile *other_file G_GNUC_UNUSED,
> + GFileMonitorEvent event_type,
> + gpointer user_data)
> +{
> + udevEventDataPtr priv = user_data;
> +
> + /* if a new directory appears, monitor that directory for changes */
> + if (g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) ==
> + G_FILE_TYPE_DIRECTORY) {
> + GFileMonitor *mon;
> +
> + if ((mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, NULL))) {
> + g_signal_connect(mon, "changed",
> + G_CALLBACK(mdevctlEventHandleCallback),
> + user_data);
> + virObjectLock(priv);
> + priv->mdevctl_monitors = g_list_append(priv->mdevctl_monitors, mon);
> + virObjectUnlock(priv);
> + }
> + }
> +
> + /* Sometimes a single configuration change results in multiple notify
> + * events (e.g. several CHANGED events, or a CREATED and CHANGED event
> + * followed by CHANGES_DONE_HINT). To avoid triggering the mdevctl thread
> + * multiple times for a single configuration change, try to coalesce these
> + * changes by waiting for the CHANGES_DONE_HINT event. As a fallback, add
> + * a timeout to trigger the signal if that event never comes */
> + if (event_type == G_FILE_MONITOR_EVENT_CREATED ||
> + event_type == G_FILE_MONITOR_EVENT_CHANGED) {
> + if (timeout_id)
> + g_source_remove(timeout_id);
> + timeout_id = g_timeout_add(100, signalMdevctlThread, priv);
> + return;
> + }
> +
> + signalMdevctlThread(priv);
> +}
> +
> +
> /* DMI is intel-compatible specific */
> #if defined(__x86_64__) || defined(__i386__) || defined(__amd64__)
> static void
> @@ -1858,6 +1969,58 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
> }
>
>
> +/* Recursively monitors dir and any subdirectory for file changes and returns a
> + * GList of GFileMonitor objects */
> +static GList*
> +monitorDirRecursively(GFile *dir,
> + GCallback changed_cb,
> + gpointer user_data)
> +{
> + GList *monitors = NULL;
> + g_autoptr(GFileInfo) dirinfo = NULL;
> + g_autoptr(GError) error = NULL;
> + g_autoptr(GFileEnumerator) children = NULL;
> + GFileMonitor *mon;
> +
> + if (!(children = g_file_enumerate_children(dir, "standard::*",
> + G_FILE_QUERY_INFO_NONE, NULL, &error))) {
> + if (error->code == G_IO_ERROR_NOT_DIRECTORY)
> + return NULL;
> + goto bail;
> + }
> +
> + if (!(mon = g_file_monitor(dir, G_FILE_MONITOR_NONE, NULL, &error)))
> + goto bail;
> +
> + g_signal_connect(mon, "changed", changed_cb, user_data);
> + monitors = g_list_append(monitors, mon);
> +
> + while (true) {
> + GFileInfo *info;
> + GFile *child;
> + GList *child_monitors = NULL;
> +
> + if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error))
> + goto bail;
> + if (!info)
> + break;
> +
> + child_monitors = monitorDirRecursively(child, changed_cb, user_data);
> + if (child_monitors)
> + monitors = g_list_concat(monitors, child_monitors);
> + }
> +
> + return monitors;
> +
> + bail:
error: is our naming convention
> + if (error)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to monitor directory: %s"), error->message);
This looks questionable - every codepath sets 'error' and we don't
want to return NULL without setting an error.
> +
> + g_list_free_full(monitors, g_object_unref);
> + return NULL;
> +}
> +
> static int
> nodeStateInitialize(bool privileged,
> const char *root,
> @@ -1867,6 +2030,8 @@ nodeStateInitialize(bool privileged,
> udevEventDataPtr priv = NULL;
> struct udev *udev = NULL;
> virThread enumThread;
> + g_autoptr(GError) error = NULL;
> + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d");
>
> if (root != NULL) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -1969,6 +2134,23 @@ nodeStateInitialize(bool privileged,
> if (priv->watch == -1)
> goto unlock;
>
> + if (virThreadCreateFull(&priv->mdevctlThread, true, mdevctlThread,
> + "mdevctl-event", false, NULL) < 0) {
> + virReportSystemError(errno, "%s",
> + _("failed to create mdevctl handler thread"));
> + goto unlock;
> + }
> +
> + /* mdevctl may add notification events in the future:
> + * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to
> + * monitoring the mdevctl configuration directory for changes.
> + * mdevctl configuration is stored in a directory tree within
> + * /etc/mdevctl.d/. There is a directory for each parent device, which
> + * contains a file defining each mediated device */
> + priv->mdevctl_monitors = monitorDirRecursively(mdevctlConfigDir,
> + G_CALLBACK(mdevctlEventHandleCallback),
> + priv);
> +
> virObjectUnlock(priv);
>
> /* Create a fictional 'computer' device to root the device tree. */
> --
> 2.26.2
>
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 :|
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?
Erik
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.
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 :|
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.
Erik
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
On a Tuesday in 2020, 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
>@@ -1858,6 +1969,58 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
> }
>
>
>+/* Recursively monitors dir and any subdirectory for file changes and returns a
>+ * GList of GFileMonitor objects */
>+static GList*
>+monitorDirRecursively(GFile *dir,
>+ GCallback changed_cb,
>+ gpointer user_data)
>+{
>+ GList *monitors = NULL;
>+ g_autoptr(GFileInfo) dirinfo = NULL;
../src/node_device/node_device_udev.c:1980:26: error: unused variable 'dirinfo' [-Werror,-Wunused-variable]
g_autoptr(GFileInfo) dirinfo = NULL;
^
>+ g_autoptr(GError) error = NULL;
>+ g_autoptr(GFileEnumerator) children = NULL;
>+ GFileMonitor *mon;
>+
>+ if (!(children = g_file_enumerate_children(dir, "standard::*",
>+ G_FILE_QUERY_INFO_NONE, NULL, &error))) {
>+ if (error->code == G_IO_ERROR_NOT_DIRECTORY)
>+ return NULL;
>+ goto bail;
s/bail/error/
>+ }
>+
>+ if (!(mon = g_file_monitor(dir, G_FILE_MONITOR_NONE, NULL, &error)))
>+ goto bail;
>+
>+ g_signal_connect(mon, "changed", changed_cb, user_data);
>+ monitors = g_list_append(monitors, mon);
>+
>+ while (true) {
>+ GFileInfo *info;
>+ GFile *child;
>+ GList *child_monitors = NULL;
>+
>+ if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error))
>+ goto bail;
>+ if (!info)
>+ break;
>+
>+ child_monitors = monitorDirRecursively(child, changed_cb, user_data);
>+ if (child_monitors)
>+ monitors = g_list_concat(monitors, child_monitors);
>+ }
>+
>+ return monitors;
>+
>+ bail:
>+ if (error)
>+ virReportError(VIR_ERR_INTERNAL_ERROR,
>+ _("Unable to monitor directory: %s"), error->message);
When returning an error code, any libvirt function should either report
an error in all cases or stay quiet, so something like:
error ? error->message : _("unknown error")
is a tiny bit frendlier for debugging.
>+
>+ g_list_free_full(monitors, g_object_unref);
>+ return NULL;
>+}
>+
> static int
> nodeStateInitialize(bool privileged,
> const char *root,
>@@ -1867,6 +2030,8 @@ nodeStateInitialize(bool privileged,
> udevEventDataPtr priv = NULL;
> struct udev *udev = NULL;
> virThread enumThread;
>+ g_autoptr(GError) error = NULL;
../src/node_device/node_device_udev.c:2033:23: error: unused variable 'error' [-Werror,-Wunused-variable]
g_autoptr(GError) error = NULL;
^
Jano
>+ g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d");
>
> if (root != NULL) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
>@@ -1969,6 +2134,23 @@ nodeStateInitialize(bool privileged,
> if (priv->watch == -1)
> goto unlock;
>
>+ if (virThreadCreateFull(&priv->mdevctlThread, true, mdevctlThread,
>+ "mdevctl-event", false, NULL) < 0) {
>+ virReportSystemError(errno, "%s",
>+ _("failed to create mdevctl handler thread"));
>+ goto unlock;
>+ }
>+
>+ /* mdevctl may add notification events in the future:
>+ * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to
>+ * monitoring the mdevctl configuration directory for changes.
>+ * mdevctl configuration is stored in a directory tree within
>+ * /etc/mdevctl.d/. There is a directory for each parent device, which
>+ * contains a file defining each mediated device */
>+ priv->mdevctl_monitors = monitorDirRecursively(mdevctlConfigDir,
>+ G_CALLBACK(mdevctlEventHandleCallback),
>+ priv);
>+
> virObjectUnlock(priv);
>
> /* Create a fictional 'computer' device to root the device tree. */
>--
>2.26.2
>
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;
> +
> +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;
> +}
> +
> +
> +static void
> +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
> + GFile *file,
> + GFile *other_file G_GNUC_UNUSED,
> + GFileMonitorEvent event_type,
> + gpointer user_data)
> +{
> + udevEventDataPtr priv = user_data;
> +
> + /* if a new directory appears, monitor that directory for changes */
> + if (g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) ==
> + G_FILE_TYPE_DIRECTORY) {
> + GFileMonitor *mon;
> +
> + if ((mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, NULL))) {
> + g_signal_connect(mon, "changed",
> + G_CALLBACK(mdevctlEventHandleCallback),
> + user_data);
> + virObjectLock(priv);
> + priv->mdevctl_monitors = g_list_append(priv->mdevctl_monitors, mon);
> + virObjectUnlock(priv);
> + }
> + }
> +
> + /* Sometimes a single configuration change results in multiple notify
> + * events (e.g. several CHANGED events, or a CREATED and CHANGED event
Isn't this a glib bug that it sends multiple (even different) events for the
same change? Same for CHANGES_DONE_HINT, how come it may not arrive at the end
of all the changes?
Regards,
Erik
> + * followed by CHANGES_DONE_HINT). To avoid triggering the mdevctl thread
> + * multiple times for a single configuration change, try to coalesce these
> + * changes by waiting for the CHANGES_DONE_HINT event. As a fallback, add
> + * a timeout to trigger the signal if that event never comes */
> + if (event_type == G_FILE_MONITOR_EVENT_CREATED ||
> + event_type == G_FILE_MONITOR_EVENT_CHANGED) {
> + if (timeout_id)
> + g_source_remove(timeout_id);
> + timeout_id = g_timeout_add(100, signalMdevctlThread, priv);
> + return;
> + }
> +
> + signalMdevctlThread(priv);
> +}
> +
> +
> /* DMI is intel-compatible specific */
> #if defined(__x86_64__) || defined(__i386__) || defined(__amd64__)
> static void
> @@ -1858,6 +1969,58 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
> }
>
>
> +/* Recursively monitors dir and any subdirectory for file changes and returns a
> + * GList of GFileMonitor objects */
> +static GList*
> +monitorDirRecursively(GFile *dir,
> + GCallback changed_cb,
> + gpointer user_data)
> +{
> + GList *monitors = NULL;
> + g_autoptr(GFileInfo) dirinfo = NULL;
> + g_autoptr(GError) error = NULL;
> + g_autoptr(GFileEnumerator) children = NULL;
> + GFileMonitor *mon;
> +
> + if (!(children = g_file_enumerate_children(dir, "standard::*",
> + G_FILE_QUERY_INFO_NONE, NULL, &error))) {
> + if (error->code == G_IO_ERROR_NOT_DIRECTORY)
> + return NULL;
> + goto bail;
> + }
> +
> + if (!(mon = g_file_monitor(dir, G_FILE_MONITOR_NONE, NULL, &error)))
> + goto bail;
> +
> + g_signal_connect(mon, "changed", changed_cb, user_data);
> + monitors = g_list_append(monitors, mon);
> +
> + while (true) {
> + GFileInfo *info;
> + GFile *child;
> + GList *child_monitors = NULL;
> +
> + if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error))
> + goto bail;
> + if (!info)
> + break;
> +
> + child_monitors = monitorDirRecursively(child, changed_cb, user_data);
> + if (child_monitors)
> + monitors = g_list_concat(monitors, child_monitors);
> + }
> +
> + return monitors;
> +
> + bail:
> + if (error)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to monitor directory: %s"), error->message);
> +
> + g_list_free_full(monitors, g_object_unref);
> + return NULL;
> +}
> +
> static int
> nodeStateInitialize(bool privileged,
> const char *root,
> @@ -1867,6 +2030,8 @@ nodeStateInitialize(bool privileged,
> udevEventDataPtr priv = NULL;
> struct udev *udev = NULL;
> virThread enumThread;
> + g_autoptr(GError) error = NULL;
> + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d");
>
> if (root != NULL) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -1969,6 +2134,23 @@ nodeStateInitialize(bool privileged,
> if (priv->watch == -1)
> goto unlock;
>
> + if (virThreadCreateFull(&priv->mdevctlThread, true, mdevctlThread,
> + "mdevctl-event", false, NULL) < 0) {
> + virReportSystemError(errno, "%s",
> + _("failed to create mdevctl handler thread"));
> + goto unlock;
> + }
> +
> + /* mdevctl may add notification events in the future:
> + * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to
> + * monitoring the mdevctl configuration directory for changes.
> + * mdevctl configuration is stored in a directory tree within
> + * /etc/mdevctl.d/. There is a directory for each parent device, which
> + * contains a file defining each mediated device */
> + priv->mdevctl_monitors = monitorDirRecursively(mdevctlConfigDir,
> + G_CALLBACK(mdevctlEventHandleCallback),
> + priv);
> +
> virObjectUnlock(priv);
>
> /* Create a fictional 'computer' device to root the device tree. */
> --
> 2.26.2
>
© 2016 - 2026 Red Hat, Inc.