[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] Potential segfault in udev driver



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]