[systemd-devel] [PATCH] allow udev to correctly handle 'change' after device has disappeared

[Kay Sievers]

On Thu, 2012-11-08 at 11:13 +0100, Robert Milasan wrote:
> From: Neil Brown <nfbrown at suse.com>
> Date: Thu, 8 Nov 2012 10:39:06 +0100
> Subject: [PATCH] If a 'change' event does not get handled by udev until
> after the device has subsequently disappeared, udev mis-handles
>  it. This can happen with 'md' devices which emit a change
>  event and then a remove event when they are stopped. It is
>  normally only noticed if udev is very busy (lots of arrays
>  being stopped at once) or the machine is otherwise loaded
>  and reponding slowly.
> 
> There are two problems.
> 
> 1/ udev_device_new_from_syspath() will refuse to create the device
>   structure if the device does not exist in /sys, and particularly if
> the uevent file does not exist.
>   If a 'db' file does exist, that is sufficient evidence that the device
>   is genuine and should be created.  Equally if we have just received an
>   event from the kernel about the device, it must be real.
> 
>   This patch just disabled the test for the 'uevent' file, it doesn't
>   try imposing any other tests - it isn't clear that they are really
>   needed.
> 
> 2/ udev_event_execute_rules() calls udev_device_read_db() on a 'device'
>    structure that is largely uninitialised and in particular does not
>    have the 'subsystem' set.  udev_device_read_db() needs the subsystem
>    so it tries to read the 'subsystem' symlink out of sysfs.  If the
> device is already deleted, this naturally fails.
>    udev_event_execute_rules() knows the subsystem (as it was in the
> event message) so this patch simply sets the subsystem for the device
> structure to be loaded to match the subsystem of the device structure
> that is handling the event.
> 
> With these two changes, deleted handling of change events will still
> correctly remove any symlinks that are not needed any more.

The problem is that at the time the 'change' event is handled, the
device is already removed by the kernel, right? That way we fail to read
the database with the currently created symlinks?

The udev_device_new_from_syspath() is a public API from libudev. We
should probably not change that and not allow other tools than udev
itself, to create device structures for devices which are not around
anymore.

Any chance to check if the patch below fixes the issue you see too. It
would preserve the current libudev behavior.

Thanks,
Kay

diff --git a/src/libudev/libudev-device.c b/src/libudev/libudev-device.c
index 08476e6..b267141 100644
--- a/src/libudev/libudev-device.c
+++ b/src/libudev/libudev-device.c
@@ -246,7 +246,7 @@ static int udev_device_set_devtype(struct udev_device *udev_device, const char *
         return 0;
 }
 
-static int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem)
+int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem)
 {
         free(udev_device->subsystem);
         udev_device->subsystem = strdup(subsystem);
diff --git a/src/libudev/libudev-private.h b/src/libudev/libudev-private.h
index d233565..49cebc1 100644
--- a/src/libudev/libudev-private.h
+++ b/src/libudev/libudev-private.h
@@ -48,6 +48,7 @@ struct udev_list_entry *udev_get_properties_list_entry(struct udev *udev);
 /* libudev-device.c */
 struct udev_device *udev_device_new(struct udev *udev);
 mode_t udev_device_get_devnode_mode(struct udev_device *udev_device);
+int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem);
 int udev_device_set_syspath(struct udev_device *udev_device, const char *syspath);
 int udev_device_set_devnode(struct udev_device *udev_device, const char *devnode);
 int udev_device_add_devlink(struct udev_device *udev_device, const char *devlink);
diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
index 2b9fdf6..39f36f9 100644
--- a/src/udev/udev-event.c
+++ b/src/udev/udev-event.c
@@ -797,8 +797,10 @@ int udev_event_execute_rules(struct udev_event *event, struct udev_rules *rules,
                 if (major(udev_device_get_devnum(dev)) != 0)
                         udev_node_remove(dev);
         } else {
-                event->dev_db = udev_device_new_from_syspath(event->udev, udev_device_get_syspath(dev));
+                event->dev_db = udev_device_new(event->udev);
                 if (event->dev_db != NULL) {
+                        udev_device_set_syspath(event->dev_db, udev_device_get_syspath(dev));
+                        udev_device_set_subsystem(event->dev_db, udev_device_get_subsystem(dev));
                         udev_device_read_db(event->dev_db, NULL);
                         udev_device_set_info_loaded(event->dev_db);