Re: [libvirt] Potential segfault in udev driver

2010/1/25 Dave Allan <dallan(a)redhat.com&gt;: > On 01/25/2010 02:33 PM, Matthias Bolte wrote: >> >> 2010/1/25 Dave Allan&lt;dallan(a)redhat.com&gt;: >>> >>> On 01/25/2010 06:37 AM, Daniel P. Berrange wrote: >>>> >>>> On Sun, Jan 24, 2010 at 11:07:59PM +0100, Matthias Bolte wrote: >>>>> >>>>> udevDeviceMonitorStartup registers udevEventHandleCallback as event >>>>> handle, but doesn't store the returned watch id to remove it later on. >>>>> Also it's not clear to me whether the event handle should be register >>>>> for the whole lifetime of the udev driver instance or just for the >>>>> udevEnumerateDevices call. >>>> >>>> The handler should be active for the lifetime of libvirtd, since the >>>> udev driver has to detect hotplug/unplug events over time. >>>> >>>>> >>>>> If for example the call to udevSetupSystemDev [1] fails >>>>> udevDeviceMonitorShutdown is called to cleanup, but >>>>> udevEventHandleCallback is still registered and may be called when >>>>> driverState is NULL again, resulting in a segfault in >>>>> udevEventHandleCallback. >>>>> >>>>> So to solve this the udevEventHandleCallback event handle must be >>>>> removed at the appropriate place. >>>> >>>> Yes, sounds like its needs to be removed in the failure path there >>> >>> Matthias, >>> >>> Indeed, that's correct--can you submit a patch? >>> >>> Dave >>> >> >> Yes, im going to do that. >> >> One last question. The udev driver stores the udev monitor handle in >> the private data pointer of the gloval driver state variable. >> >>     driverState->privateData = udev_monitor; >> >> To solve the event handle problem we need to store the watch id >> returned by virEventAddHandle somewhere. We could either add a new >> private data struct to hold the udev_monitor pointer and the watch id, >> or store the watch id variable globally side by side with the driver >> state variable. I prefer the first option, because it's cleaner and >> the DRV_STATE_UDEV_MONITOR define allows for changing the storage >> location of the udev_monitor pointer easily. > > Yes, I agree--a struct is the right approach.  Thanks for doing this! > > Dave > Here's the patch. Maximilian Wilhelm tested it. Matthias

From 07678696504179c70b987947061de2ff658e665c Mon Sep 17 00:00:00 2001 From: Matthias Bolte <matthias.bolte(a)googlemail.com&gt; Date: Tue, 26 Jan 2010 02:58:37 +0100 Subject: [PATCH] udev: Remove event handle on shutdown This fixes a segfault when the event handler is called after shutdown when the global driver state is NULL again. Also fix a locking issue in an error path. --- src/node_device/node_device_udev.c | 49 +++++++++++++++++++++++++++--------- src/node_device/node_device_udev.h | 4 ++- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 2e459d1..a625d76 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -41,6 +41,11 @@ #define VIR_FROM_THIS VIR_FROM_NODEDEV +struct _udevPrivate { + struct udev_monitor *udev_monitor; + int watch; +}; + static virDeviceMonitorStatePtr driverState = NULL; static int udevStrToLong_ull(char const *s, @@ -1354,12 +1359,18 @@ static int udevDeviceMonitorShutdown(void) { int ret = 0; + udevPrivate *priv = NULL; struct udev_monitor *udev_monitor = NULL; struct udev *udev = NULL; if (driverState) { - nodeDeviceLock(driverState); + + priv = driverState->privateData; + + if (priv->watch != -1) + virEventRemoveHandle(priv->watch); + udev_monitor = DRV_STATE_UDEV_MONITOR(driverState); if (udev_monitor != NULL) { @@ -1375,7 +1386,7 @@ static int udevDeviceMonitorShutdown(void) nodeDeviceUnlock(driverState); virMutexDestroy(&driverState->lock); VIR_FREE(driverState); - + VIR_FREE(priv); } else { ret = -1; } @@ -1534,18 +1545,28 @@ out: static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED) { + udevPrivate *priv = NULL; struct udev *udev = NULL; - struct udev_monitor *udev_monitor = NULL; int ret = 0; + if (VIR_ALLOC(priv) < 0) { + virReportOOMError(NULL); + ret = -1; + goto out; + } + + priv->watch = -1; + if (VIR_ALLOC(driverState) < 0) { virReportOOMError(NULL); + VIR_FREE(priv); ret = -1; goto out; } if (virMutexInit(&driverState->lock) < 0) { VIR_ERROR0("Failed to initialize mutex for driverState"); + VIR_FREE(priv); VIR_FREE(driverState); ret = -1; goto out; @@ -1562,18 +1583,19 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED) udev = udev_new(); udev_set_log_fn(udev, udevLogFunction); - udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); - if (udev_monitor == NULL) { + priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); + if (priv->udev_monitor == NULL) { + VIR_FREE(priv); + nodeDeviceUnlock(driverState); VIR_ERROR0("udev_monitor_new_from_netlink returned NULL"); ret = -1; goto out; } - udev_monitor_enable_receiving(udev_monitor); + udev_monitor_enable_receiving(priv->udev_monitor); /* udev can be retrieved from udev_monitor */ - driverState->privateData = udev_monitor; - nodeDeviceUnlock(driverState); + driverState->privateData = priv; /* We register the monitor with the event callback so we are * notified by udev of device changes before we enumerate existing @@ -1583,14 +1605,17 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED) * enumeration. The alternative is to register the callback after * we enumerate, in which case we will fail to create any devices * that appear while the enumeration is taking place. */ - if (virEventAddHandle(udev_monitor_get_fd(udev_monitor), - VIR_EVENT_HANDLE_READABLE, - udevEventHandleCallback, - NULL, NULL) == -1) { + priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), + VIR_EVENT_HANDLE_READABLE, + udevEventHandleCallback, NULL, NULL); + if (priv->watch == -1) { + nodeDeviceUnlock(driverState); ret = -1; goto out; } + nodeDeviceUnlock(driverState); + /* Create a fictional 'computer' device to root the device tree. */ if (udevSetupSystemDev() != 0) { ret = -1; diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h index 6c83412..8367494 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -23,8 +23,10 @@ #include <libudev.h> #include <stdint.h> +typedef struct _udevPrivate udevPrivate; + #define SYSFS_DATA_SIZE 4096 -#define DRV_STATE_UDEV_MONITOR(ds) ((struct udev_monitor *)((ds)->privateData)) +#define DRV_STATE_UDEV_MONITOR(ds) (((udevPrivate *)((ds)->privateData))->udev_monitor) #define DMI_DEVPATH "/sys/devices/virtual/dmi/id" #define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" #define PROPERTY_FOUND 0