Skip to content

Commit

Permalink
udev: abort non-remove event processing if device is already removed
Browse files Browse the repository at this point in the history
If the device is already removed, we will not be able to load identifying
information from the device :-).  So we will fail to calculate the correct
tag.  Abort processing before we save the incorrect data to the DB!  Also
don't broadcast the incomplete event or run any of the collected RUN
programs.

This avoids losing tags in the DB, which we need for the "remove" event.
On the remove event, we will broadcast the event with the tags (and
other properties) loaded from the DB.

This fixes the systemd part of systemd#7587 / systemd#8221.

My test case is to plug and unplug a USB printer, while running on
kernel version 4.12+.  Before this fix, the device would remain active in
`systemctl list-devices *usb*.device`. After this fix, the device unit is
removed correctly.

If there are third-party rules which add tags (are there any of these??),
they can match our behaviour by handling all unknown events the same as
"change" events.  This is *already required* on kernels 4.12+ due to the
addition of "bind" and "unbind" events, combined with a change to
systemd-udev which allowed "bind" and "unbind" events to be executed with
all of the same features as "change" events).  If your rules skip unknown
events, you will lose symlinks and tags, for example.  Losing tags is
*not allowed for* by the libudev filter design.   (Again, I don't know if
anything really uses tags outside of the systemd project).
  • Loading branch information
sourcejedi committed Dec 8, 2018
1 parent 99b5b0d commit aa76361
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 9 deletions.
32 changes: 29 additions & 3 deletions src/udev/udev-event.c
Expand Up @@ -811,7 +811,9 @@ int udev_event_execute_rules(struct udev_event *event,
Hashmap *properties_list,
struct udev_rules *rules) {
sd_device *dev = event->dev;
const char *subsystem, *action;
const char *subsystem, *action, *syspath;
_cleanup_close_ int sysfd = -1;
struct stat st;
int r;

assert(event);
Expand All @@ -827,9 +829,26 @@ int udev_event_execute_rules(struct udev_event *event,

if (streq(action, "remove")) {
event_execute_rules_on_remove(event, timeout_usec, properties_list, rules);
return 0;
return 1;
}

/*
* If the device was removed since this event, we cannot get good data. Abort.
* We will see a "remove" event later, and process that using the last good data.
* This is particularly important to avoid missing tags on the "remove" event.
*
* Note, so far we can't guarantee to avoid "remove"+"add" e.g. after this event
* but before we write attributes. Just... hope no-one does that.
* In particular, that they don't tempt fate by removing and adding devices
* with the same names in quick succession.
*
* TODO: add DEVICE_SEQNUM in the kernel, and sysfd in sd_device :-).
*/
(void) sd_device_get_syspath(dev, &syspath);
sysfd = open(syspath, O_RDONLY);
if (sysfd < 0)
return 0;

r = device_clone_with_db(dev, &event->dev_db_clone);
if (r < 0)
log_device_debug_errno(dev, r, "Failed to clone sd_device object, ignoring: %m");
Expand All @@ -852,6 +871,13 @@ int udev_event_execute_rules(struct udev_event *event,

(void) udev_rules_apply_to_event(rules, event, timeout_usec, properties_list);

/* check if device was removed during processing */
r = fstat(sysfd, &st);
if (r < 0)
return log_device_error_errno(dev, r, "Failed to stat syspath: %m");
if (st.st_nlink == 0)
return 0;

(void) rename_netif(event);
(void) update_devnode(event);

Expand All @@ -873,7 +899,7 @@ int udev_event_execute_rules(struct udev_event *event,

event->dev_db_clone = sd_device_unref(event->dev_db_clone);

return 0;
return 1;
}

void udev_event_execute_run(struct udev_event *event, usec_t timeout_usec) {
Expand Down
21 changes: 15 additions & 6 deletions src/udev/udevd.c
Expand Up @@ -417,7 +417,13 @@ static int worker_process_device(Manager *manager, sd_device *dev) {
return r;

/* apply rules, create node, symlinks */
udev_event_execute_rules(udev_event, arg_event_timeout_usec, manager->properties, manager->rules);
r = udev_event_execute_rules(udev_event, arg_event_timeout_usec, manager->properties, manager->rules);
if (r < 0)
return r;
if (r == 0) {
log_device_debug(dev, "Device removed during processing. Discarding invalid results.");
return 0;
}
udev_event_execute_run(udev_event, arg_event_timeout_usec);

if (!manager->rtnl)
Expand All @@ -434,7 +440,7 @@ static int worker_process_device(Manager *manager, sd_device *dev) {

log_device_debug(dev, "Device (SEQNUM=%s) processed", seqnum);

return 0;
return 1;
}

static int worker_device_monitor_handler(sd_device_monitor *monitor, sd_device *dev, void *userdata) {
Expand All @@ -449,11 +455,14 @@ static int worker_device_monitor_handler(sd_device_monitor *monitor, sd_device *
log_device_warning_errno(dev, r, "Failed to process device, ignoring: %m");

/* send processed event back to libudev listeners */
r = device_monitor_send_device(monitor, NULL, dev);
if (r < 0)
log_device_warning_errno(dev, r, "Failed to send device, ignoring: %m");
if (r == 1) {
r = device_monitor_send_device(monitor, NULL, dev);
if (r < 0)
log_device_warning_errno(dev, r, "Failed to send device, ignoring: %m");
}


/* send udevd the result of the event execution */
/* tell udevd the event has been completed */
r = worker_send_message(manager->worker_watch[WRITE_END]);
if (r < 0)
log_device_warning_errno(dev, r, "Failed to send signal to main daemon, ignoring: %m");
Expand Down

0 comments on commit aa76361

Please sign in to comment.