[libvirt] Potential segfault in udev driver
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. 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. Maximilian Wilhelm found this problem. [1] 22:42:20.960: error : udevSetupSystemDev:1460 : Failed to get udev device for syspath '/sys/devices/virtual/dmi/id' or '/sys/class/dmi/id' Matthias
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 Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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
2010/1/25 Dave Allan <dallan@redhat.com>:
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. Matthias
On 01/25/2010 02:33 PM, Matthias Bolte wrote:
2010/1/25 Dave Allan<dallan@redhat.com>:
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
2010/1/25 Dave Allan <dallan@redhat.com>:
On 01/25/2010 02:33 PM, Matthias Bolte wrote:
2010/1/25 Dave Allan<dallan@redhat.com>:
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
On Tue, Jan 26, 2010 at 03:17:42AM +0100, Matthias Bolte wrote:
2010/1/25 Dave Allan <dallan@redhat.com>:
On 01/25/2010 02:33 PM, Matthias Bolte wrote:
2010/1/25 Dave Allan<dallan@redhat.com>:
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@googlemail.com> 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
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/1/26 Daniel P. Berrange <berrange@redhat.com>:
On Tue, Jan 26, 2010 at 03:17:42AM +0100, Matthias Bolte wrote:
Here's the patch. Maximilian Wilhelm tested it.
Matthias
From 07678696504179c70b987947061de2ff658e665c Mon Sep 17 00:00:00 2001 From: Matthias Bolte <matthias.bolte@googlemail.com> 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. --- [...]
ACK
Daniel
Thanks, pushed. Matthias
participants (3)
-
Daniel P. Berrange -
Dave Allan -
Matthias Bolte