From df7ee95913c70088263c768723dc5572982221f9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 6 Mar 2021 16:09:23 +0900 Subject: [PATCH 01/16] udev: do not store inotify fd in a global variable When manager_exit() or manager_free() is called, the global variable in udev-watch.c is not set '-1'. Of course, that is safe, as the event source for the inotify fd is unref()ed in manager_exit() and manager_free(). But let's not store fd globally. --- src/test/test-udev.c | 2 +- src/udev/udev-event.c | 20 +++++++++++-------- src/udev/udev-event.h | 34 +++++++++++++++++++------------ src/udev/udev-watch.c | 44 +++++++++++------------------------------ src/udev/udev-watch.h | 7 +++---- src/udev/udevadm-test.c | 2 +- src/udev/udevd.c | 37 ++++++++++++++++++++-------------- 7 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/test/test-udev.c b/src/test/test-udev.c index 6bb8a9e4fc..c2c63cc679 100644 --- a/src/test/test-udev.c +++ b/src/test/test-udev.c @@ -130,7 +130,7 @@ static int run(int argc, char *argv[]) { } } - udev_event_execute_rules(event, 3 * USEC_PER_SEC, SIGKILL, NULL, rules); + udev_event_execute_rules(event, -1, 3 * USEC_PER_SEC, SIGKILL, NULL, rules); udev_event_execute_run(event, 3 * USEC_PER_SEC, SIGKILL); return 0; diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 12597194a9..82cdd99b7a 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -913,6 +913,7 @@ static int update_devnode(UdevEvent *event) { static void event_execute_rules_on_remove( UdevEvent *event, + int inotify_fd, usec_t timeout_usec, int timeout_signal, Hashmap *properties_list, @@ -934,7 +935,7 @@ static void event_execute_rules_on_remove( log_device_debug_errno(dev, r, "Failed to delete database under /run/udev/data/, ignoring: %m"); if (sd_device_get_devnum(dev, NULL) >= 0) - (void) udev_watch_end(dev); + (void) udev_watch_end(inotify_fd, dev); (void) udev_rules_apply_to_event(rules, event, timeout_usec, timeout_signal, properties_list); @@ -971,11 +972,14 @@ static int copy_all_tags(sd_device *d, sd_device *s) { return 0; } -int udev_event_execute_rules(UdevEvent *event, - usec_t timeout_usec, - int timeout_signal, - Hashmap *properties_list, - UdevRules *rules) { +int udev_event_execute_rules( + UdevEvent *event, + int inotify_fd, /* This may be negative */ + usec_t timeout_usec, + int timeout_signal, + Hashmap *properties_list, + UdevRules *rules) { + const char *subsystem; sd_device_action_t action; sd_device *dev; @@ -995,7 +999,7 @@ int udev_event_execute_rules(UdevEvent *event, return log_device_error_errno(dev, r, "Failed to get ACTION: %m"); if (action == SD_DEVICE_REMOVE) { - event_execute_rules_on_remove(event, timeout_usec, timeout_signal, properties_list, rules); + event_execute_rules_on_remove(event, inotify_fd, timeout_usec, timeout_signal, properties_list, rules); return 0; } @@ -1009,7 +1013,7 @@ int udev_event_execute_rules(UdevEvent *event, if (sd_device_get_devnum(dev, NULL) >= 0) /* Disable watch during event processing. */ - (void) udev_watch_end(event->dev_db_clone); + (void) udev_watch_end(inotify_fd, event->dev_db_clone); if (action == SD_DEVICE_MOVE) { r = udev_event_on_move(event->dev); diff --git a/src/udev/udev-event.h b/src/udev/udev-event.h index ecbe957b4f..d90cc10a2d 100644 --- a/src/udev/udev-event.h +++ b/src/udev/udev-event.h @@ -51,20 +51,28 @@ UdevEvent *udev_event_new(sd_device *dev, usec_t exec_delay_usec, sd_netlink *rt UdevEvent *udev_event_free(UdevEvent *event); DEFINE_TRIVIAL_CLEANUP_FUNC(UdevEvent*, udev_event_free); -size_t udev_event_apply_format(UdevEvent *event, - const char *src, char *dest, size_t size, - bool replace_whitespace); +size_t udev_event_apply_format( + UdevEvent *event, + const char *src, + char *dest, + size_t size, + bool replace_whitespace); int udev_check_format(const char *value, size_t *offset, const char **hint); -int udev_event_spawn(UdevEvent *event, - usec_t timeout_usec, - int timeout_signal, - bool accept_failure, - const char *cmd, char *result, size_t ressize); -int udev_event_execute_rules(UdevEvent *event, - usec_t timeout_usec, - int timeout_signal, - Hashmap *properties_list, - UdevRules *rules); +int udev_event_spawn( + UdevEvent *event, + usec_t timeout_usec, + int timeout_signal, + bool accept_failure, + const char *cmd, + char *result, + size_t ressize); +int udev_event_execute_rules( + UdevEvent *event, + int inotify_fd, + usec_t timeout_usec, + int timeout_signal, + Hashmap *properties_list, + UdevRules *rules); void udev_event_execute_run(UdevEvent *event, usec_t timeout_usec, int timeout_signal); static inline usec_t udev_warn_timeout(usec_t timeout_usec) { diff --git a/src/udev/udev-watch.c b/src/udev/udev-watch.c index 8656fb0076..e90fcd1bcc 100644 --- a/src/udev/udev-watch.c +++ b/src/udev/udev-watch.c @@ -16,28 +16,13 @@ #include "stdio-util.h" #include "udev-watch.h" -static int inotify_fd = -1; - -/* inotify descriptor, will be shared with rules directory; - * set to cloexec since we need our children to be able to add - * watches for us. */ -int udev_watch_init(void) { - inotify_fd = inotify_init1(IN_CLOEXEC); - if (inotify_fd < 0) - return -errno; - - return inotify_fd; -} - /* Move any old watches directory out of the way, and then restore the watches. */ -int udev_watch_restore(void) { +int udev_watch_restore(int inotify_fd) { struct dirent *ent; DIR *dir; int r; - if (inotify_fd < 0) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid inotify descriptor."); + assert(inotify_fd >= 0); if (rename("/run/udev/watch", "/run/udev/watch.old") < 0) { if (errno != ENOENT) @@ -70,7 +55,7 @@ int udev_watch_restore(void) { } log_device_debug(dev, "Restoring old watch"); - (void) udev_watch_begin(dev); + (void) udev_watch_begin(inotify_fd, dev); unlink: (void) unlinkat(dirfd(dir), ent->d_name, 0); } @@ -81,14 +66,13 @@ unlink: return 0; } -int udev_watch_begin(sd_device *dev) { +int udev_watch_begin(int inotify_fd, sd_device *dev) { char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; const char *devnode, *id_filename; int wd, r; - if (inotify_fd < 0) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid inotify descriptor."); + assert(inotify_fd >= 0); + assert(dev); r = sd_device_get_devname(dev, &devnode); if (r < 0) @@ -118,12 +102,15 @@ int udev_watch_begin(sd_device *dev) { return 0; } -int udev_watch_end(sd_device *dev) { +int udev_watch_end(int inotify_fd, sd_device *dev) { char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; int wd, r; + assert(dev); + + /* This may be called by 'udevadm test'. In that case, inotify_fd is not initialized. */ if (inotify_fd < 0) - return 0; /* Nothing to do. */ + return 0; r = device_get_watch_handle(dev, &wd); if (r == -ENOENT) @@ -147,16 +134,9 @@ int udev_watch_lookup(int wd, sd_device **ret) { _cleanup_free_ char *device = NULL; int r; + assert(wd >= 0); assert(ret); - if (inotify_fd < 0) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid inotify descriptor."); - - if (wd < 0) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid watch handle."); - xsprintf(filename, "/run/udev/watch/%d", wd); r = readlink_malloc(filename, &device); if (r == -ENOENT) diff --git a/src/udev/udev-watch.h b/src/udev/udev-watch.h index a15fa2767e..f5c55794c4 100644 --- a/src/udev/udev-watch.h +++ b/src/udev/udev-watch.h @@ -3,8 +3,7 @@ #include "sd-device.h" -int udev_watch_init(void); -int udev_watch_restore(void); -int udev_watch_begin(sd_device *dev); -int udev_watch_end(sd_device *dev); +int udev_watch_restore(int inotify_fd); +int udev_watch_begin(int inotify_fd, sd_device *dev); +int udev_watch_end(int inotify_fd, sd_device *dev); int udev_watch_lookup(int wd, sd_device **ret); diff --git a/src/udev/udevadm-test.c b/src/udev/udevadm-test.c index 7a30e25135..fbac719fa0 100644 --- a/src/udev/udevadm-test.c +++ b/src/udev/udevadm-test.c @@ -141,7 +141,7 @@ int test_main(int argc, char *argv[], void *userdata) { assert_se(sigfillset(&mask) >= 0); assert_se(sigprocmask(SIG_SETMASK, &mask, &sigmask_orig) >= 0); - udev_event_execute_rules(event, 60 * USEC_PER_SEC, SIGKILL, NULL, rules); + udev_event_execute_rules(event, -1, 60 * USEC_PER_SEC, SIGKILL, NULL, rules); FOREACH_DEVICE_PROPERTY(dev, key, value) printf("%s=%s\n", key, value); diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 2c702d0388..299f89566f 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -92,10 +92,12 @@ typedef struct Manager { sd_device_monitor *monitor; struct udev_ctrl *ctrl; - int fd_inotify; int worker_watch[2]; + /* used by udev-watch */ + int inotify_fd; sd_event_source *inotify_event; + sd_event_source *kill_workers_event; usec_t last_usec; @@ -304,7 +306,7 @@ static Manager* manager_free(Manager *manager) { hashmap_free_free_free(manager->properties); udev_rules_free(manager->rules); - safe_close(manager->fd_inotify); + safe_close(manager->inotify_fd); safe_close_pair(manager->worker_watch); return mfree(manager); @@ -473,7 +475,7 @@ static int worker_process_device(Manager *manager, sd_device *dev) { * degree — they go hand-in-hand after all. */ log_device_debug(dev, "Block device is currently locked, installing watch to wait until the lock is released."); - (void) udev_watch_begin(dev); + (void) udev_watch_begin(manager->inotify_fd, dev); /* Now the watch is installed, let's lock the device again, maybe in the meantime things changed */ r = worker_lock_block_device(dev, &fd_lock); @@ -484,7 +486,13 @@ static int worker_process_device(Manager *manager, sd_device *dev) { (void) worker_mark_block_device_read_only(dev); /* apply rules, create node, symlinks */ - r = udev_event_execute_rules(udev_event, arg_event_timeout_usec, arg_timeout_signal, manager->properties, manager->rules); + r = udev_event_execute_rules( + udev_event, + manager->inotify_fd, + arg_event_timeout_usec, + arg_timeout_signal, + manager->properties, + manager->rules); if (r < 0) return r; @@ -496,12 +504,12 @@ static int worker_process_device(Manager *manager, sd_device *dev) { /* apply/restore/end inotify watch */ if (udev_event->inotify_watch) { - (void) udev_watch_begin(dev); + (void) udev_watch_begin(manager->inotify_fd, dev); r = device_update_db(dev); if (r < 0) return log_device_debug_errno(dev, r, "Failed to update database under /run/udev/data/: %m"); } else - (void) udev_watch_end(dev); + (void) udev_watch_end(manager->inotify_fd, dev); log_device_uevent(dev, "Device processed"); return 0; @@ -872,7 +880,7 @@ static void manager_exit(Manager *manager) { manager->ctrl = udev_ctrl_unref(manager->ctrl); manager->inotify_event = sd_event_source_unref(manager->inotify_event); - manager->fd_inotify = safe_close(manager->fd_inotify); + manager->inotify_fd = safe_close(manager->inotify_fd); manager->monitor = sd_device_monitor_unref(manager->monitor); @@ -1309,7 +1317,7 @@ static int on_inotify(sd_event_source *s, int fd, uint32_t revents, void *userda if (e->mask & IN_CLOSE_WRITE) synthesize_change(dev); else if (e->mask & IN_IGNORED) - udev_watch_end(dev); + udev_watch_end(manager->inotify_fd, dev); } return 1; @@ -1669,7 +1677,7 @@ static int manager_new(Manager **ret, int fd_ctrl, int fd_uevent, const char *cg return log_oom(); *manager = (Manager) { - .fd_inotify = -1, + .inotify_fd = -1, .worker_watch = { -1, -1 }, .cgroup = cgroup, }; @@ -1722,12 +1730,11 @@ static int main_loop(Manager *manager) { if (r < 0) return log_error_errno(r, "Failed to enable SO_PASSCRED: %m"); - r = udev_watch_init(); - if (r < 0) - return log_error_errno(r, "Failed to create inotify descriptor: %m"); - manager->fd_inotify = r; + manager->inotify_fd = inotify_init1(IN_CLOEXEC); + if (manager->inotify_fd < 0) + return log_error_errno(errno, "Failed to create inotify descriptor: %m"); - udev_watch_restore(); + udev_watch_restore(manager->inotify_fd); /* block and listen to all signals on signalfd */ assert_se(sigprocmask_many(SIG_BLOCK, NULL, SIGTERM, SIGINT, SIGHUP, SIGCHLD, -1) >= 0); @@ -1772,7 +1779,7 @@ static int main_loop(Manager *manager) { if (r < 0) return log_error_errno(r, "Failed to set IDLE event priority for udev control event source: %m"); - r = sd_event_add_io(manager->event, &manager->inotify_event, manager->fd_inotify, EPOLLIN, on_inotify, manager); + r = sd_event_add_io(manager->event, &manager->inotify_event, manager->inotify_fd, EPOLLIN, on_inotify, manager); if (r < 0) return log_error_errno(r, "Failed to create inotify event source: %m"); From 99e0c96f31a1b6e72cc12393be9f89f7d30d26aa Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 6 Mar 2021 16:51:26 +0900 Subject: [PATCH 02/16] sd-device: drop doubled empty lines --- src/libsystemd/sd-device/sd-device.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 4b7f22e207..8ef5aa55bf 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1296,7 +1296,6 @@ int device_get_id_filename(sd_device *device, const char **ret) { if (!subsystem) return -EINVAL; - if (streq(subsystem, "drivers")) /* the 'drivers' pseudo-subsystem is special, and needs the real subsystem * encoded as well */ From fa5d8899aafea4cc0e22ddfdcf568ae5743bebdb Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 6 Mar 2021 16:57:51 +0900 Subject: [PATCH 03/16] udev: drop unused variable --- src/udev/udev-event.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 82cdd99b7a..6bd95f25fb 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -980,7 +980,6 @@ int udev_event_execute_rules( Hashmap *properties_list, UdevRules *rules) { - const char *subsystem; sd_device_action_t action; sd_device *dev; int r; @@ -990,10 +989,6 @@ int udev_event_execute_rules( dev = event->dev; - r = sd_device_get_subsystem(dev, &subsystem); - if (r < 0) - return log_device_error_errno(dev, r, "Failed to get subsystem: %m"); - r = sd_device_get_action(dev, &action); if (r < 0) return log_device_error_errno(dev, r, "Failed to get ACTION: %m"); From 75b4984638750025c286809931f55ea50cc8289e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 6 Mar 2021 21:16:59 +0900 Subject: [PATCH 04/16] udev: shorten code a bit --- src/udev/udev-event.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 6bd95f25fb..40a4130f8f 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -1047,11 +1047,7 @@ int udev_event_execute_rules( /* Yes, we run update_devnode() twice, because in the first invocation, that is before update of udev database, * it could happen that two contenders are replacing each other's symlink. Hence we run it again to make sure * symlinks point to devices that claim them with the highest priority. */ - r = update_devnode(event); - if (r < 0) - return r; - - return 0; + return update_devnode(event); } void udev_event_execute_run(UdevEvent *event, usec_t timeout_usec, int timeout_signal) { From 4cac2260c8ef6a324c95461888c02db32f6c1eb2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 6 Mar 2021 17:02:11 +0900 Subject: [PATCH 05/16] udev: also propagate error in udev_rules_apply_to_event() on remove event When udev rules are not applied correctly, then run program lists is not perfect. So, udev_event_execute_run() later in worker_process_device() should not be called. --- src/udev/udev-event.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 40a4130f8f..2e82be6fb3 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -911,7 +911,7 @@ static int update_devnode(UdevEvent *event) { return udev_node_add(dev, apply_mac, event->mode, event->uid, event->gid, event->seclabel_list); } -static void event_execute_rules_on_remove( +static int event_execute_rules_on_remove( UdevEvent *event, int inotify_fd, usec_t timeout_usec, @@ -937,10 +937,12 @@ static void event_execute_rules_on_remove( if (sd_device_get_devnum(dev, NULL) >= 0) (void) udev_watch_end(inotify_fd, dev); - (void) udev_rules_apply_to_event(rules, event, timeout_usec, timeout_signal, properties_list); + r = udev_rules_apply_to_event(rules, event, timeout_usec, timeout_signal, properties_list); if (sd_device_get_devnum(dev, NULL) >= 0) (void) udev_node_remove(dev); + + return r; } static int udev_event_on_move(sd_device *dev) { @@ -993,10 +995,8 @@ int udev_event_execute_rules( if (r < 0) return log_device_error_errno(dev, r, "Failed to get ACTION: %m"); - if (action == SD_DEVICE_REMOVE) { - event_execute_rules_on_remove(event, inotify_fd, timeout_usec, timeout_signal, properties_list, rules); - return 0; - } + if (action == SD_DEVICE_REMOVE) + return event_execute_rules_on_remove(event, inotify_fd, timeout_usec, timeout_signal, properties_list, rules); r = device_clone_with_db(dev, &event->dev_db_clone); if (r < 0) From 0d77bc4a18efa21022fe90cf8d6051a7b5b039f3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 6 Mar 2021 20:45:17 +0900 Subject: [PATCH 06/16] udev: refuse to enable inotify watch on remove event Some udev rule may erroneously set inotify watch on remove event. For safety, silently ignore such an inotify watch enablement. This also moves inotify watch enablement code to udev-event.c. --- src/udev/udev-event.c | 31 +++++++++++++++++++++++++++++++ src/udev/udev-event.h | 1 + src/udev/udevd.c | 11 +++-------- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 2e82be6fb3..f4dbcef959 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -1082,3 +1082,34 @@ void udev_event_execute_run(UdevEvent *event, usec_t timeout_usec, int timeout_s } } } + +int udev_event_process_inotify_watch(UdevEvent *event, int inotify_fd) { + sd_device *dev; + int r; + + assert(event); + assert(inotify_fd >= 0); + + dev = event->dev; + + assert(dev); + + if (device_for_action(dev, SD_DEVICE_REMOVE)) + return 0; + + if (sd_device_get_devname(dev, NULL) < 0) + return 0; + + if (!event->inotify_watch) { + (void) udev_watch_end(inotify_fd, dev); + return 0; + } + + (void) udev_watch_begin(inotify_fd, dev); + + r = device_update_db(dev); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to update database under /run/udev/data/: %m"); + + return 0; +} diff --git a/src/udev/udev-event.h b/src/udev/udev-event.h index d90cc10a2d..2067909fde 100644 --- a/src/udev/udev-event.h +++ b/src/udev/udev-event.h @@ -74,6 +74,7 @@ int udev_event_execute_rules( Hashmap *properties_list, UdevRules *rules); void udev_event_execute_run(UdevEvent *event, usec_t timeout_usec, int timeout_signal); +int udev_event_process_inotify_watch(UdevEvent *event, int inotify_fd); static inline usec_t udev_warn_timeout(usec_t timeout_usec) { return DIV_ROUND_UP(timeout_usec, 3); diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 299f89566f..9689e15b1c 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -502,14 +502,9 @@ static int worker_process_device(Manager *manager, sd_device *dev) { /* in case rtnl was initialized */ manager->rtnl = sd_netlink_ref(udev_event->rtnl); - /* apply/restore/end inotify watch */ - if (udev_event->inotify_watch) { - (void) udev_watch_begin(manager->inotify_fd, dev); - r = device_update_db(dev); - if (r < 0) - return log_device_debug_errno(dev, r, "Failed to update database under /run/udev/data/: %m"); - } else - (void) udev_watch_end(manager->inotify_fd, dev); + r = udev_event_process_inotify_watch(udev_event, manager->inotify_fd); + if (r < 0) + return r; log_device_uevent(dev, "Device processed"); return 0; From fe7323815d90452a663120a59bca6f9c23971941 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Mar 2021 15:07:51 +0900 Subject: [PATCH 07/16] sd-device: rename device_get_id_filename() -> device_get_device_id() We have sd_device_new_from_device_id(), which takes device ID generated from device_get_id_filename(). For consistency, let's rename the function. --- src/libsystemd/sd-device/device-internal.h | 2 +- src/libsystemd/sd-device/device-private.c | 6 ++-- src/libsystemd/sd-device/device-private.h | 2 +- src/libsystemd/sd-device/sd-device.c | 12 ++++---- src/udev/udev-node.c | 34 +++++++++++----------- src/udev/udev-watch.c | 22 +++++++------- 6 files changed, 39 insertions(+), 39 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index c7f9b59087..6776ab3aa7 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -67,7 +67,7 @@ struct sd_device { char *driver_subsystem; /* only set for the 'drivers' subsystem */ char *driver; - char *id_filename; + char *device_id; usec_t usec_initialized; diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 7a7204b145..cac7b98518 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -761,7 +761,7 @@ static int device_tag(sd_device *device, const char *tag, bool add) { assert(device); assert(tag); - r = device_get_id_filename(device, &id); + r = device_get_device_id(device, &id); if (r < 0) return r; @@ -846,7 +846,7 @@ int device_update_db(sd_device *device) { has_info = device_has_info(device); - r = device_get_id_filename(device, &id); + r = device_get_device_id(device, &id); if (r < 0) return r; @@ -950,7 +950,7 @@ int device_delete_db(sd_device *device) { assert(device); - r = device_get_id_filename(device, &id); + r = device_get_device_id(device, &id); if (r < 0) return r; diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index ec76f772e5..8eafc7ef74 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -13,7 +13,7 @@ int device_new_from_nulstr(sd_device **ret, uint8_t *nulstr, size_t len); int device_new_from_strv(sd_device **ret, char **strv); -int device_get_id_filename(sd_device *device, const char **ret); +int device_get_device_id(sd_device *device, const char **ret); int device_get_devlink_priority(sd_device *device, int *priority); int device_get_watch_handle(sd_device *device, int *handle); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 8ef5aa55bf..decf9ae1d1 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -62,7 +62,7 @@ static sd_device *device_free(sd_device *device) { free(device->subsystem); free(device->driver_subsystem); free(device->driver); - free(device->id_filename); + free(device->device_id); free(device->properties_strv); free(device->properties_nulstr); @@ -1255,11 +1255,11 @@ static int handle_db_line(sd_device *device, char key, const char *value) { return 0; } -int device_get_id_filename(sd_device *device, const char **ret) { +int device_get_device_id(sd_device *device, const char **ret) { assert(device); assert(ret); - if (!device->id_filename) { + if (!device->device_id) { _cleanup_free_ char *id = NULL; const char *subsystem; dev_t devnum; @@ -1306,10 +1306,10 @@ int device_get_id_filename(sd_device *device, const char **ret) { return -ENOMEM; } - device->id_filename = TAKE_PTR(id); + device->device_id = TAKE_PTR(id); } - *ret = device->id_filename; + *ret = device->device_id; return 0; } @@ -1406,7 +1406,7 @@ int device_read_db_internal(sd_device *device, bool force) { if (device->db_loaded || (!force && device->sealed)) return 0; - r = device_get_id_filename(device, &id); + r = device_get_device_id(device, &id); if (r < 0) return r; diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index 96963a4343..5dc205e364 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -30,7 +30,7 @@ static int node_symlink(sd_device *dev, const char *node, const char *slink) { _cleanup_free_ char *slink_dirname = NULL, *target = NULL; - const char *id_filename, *slink_tmp; + const char *id, *slink_tmp; struct stat stats; int r; @@ -81,10 +81,10 @@ static int node_symlink(sd_device *dev, const char *node, const char *slink) { } log_device_debug(dev, "Atomically replace '%s'", slink); - r = device_get_id_filename(dev, &id_filename); + r = device_get_device_id(dev, &id); if (r < 0) - return log_device_error_errno(dev, r, "Failed to get id_filename: %m"); - slink_tmp = strjoina(slink, ".tmp-", id_filename); + return log_device_error_errno(dev, r, "Failed to get device id: %m"); + slink_tmp = strjoina(slink, ".tmp-", id); (void) unlink(slink_tmp); do { r = mkdir_parents_label(slink_tmp, 0755); @@ -147,7 +147,7 @@ static int link_find_prioritized(sd_device *dev, bool add, const char *stackdir, FOREACH_DIRENT_ALL(dent, dir, break) { _cleanup_(sd_device_unrefp) sd_device *dev_db = NULL; - const char *devnode, *id_filename; + const char *devnode, *id; int db_prio = 0; if (dent->d_name[0] == '\0') @@ -157,11 +157,11 @@ static int link_find_prioritized(sd_device *dev, bool add, const char *stackdir, log_device_debug(dev, "Found '%s' claiming '%s'", dent->d_name, stackdir); - if (device_get_id_filename(dev, &id_filename) < 0) + if (device_get_device_id(dev, &id) < 0) continue; /* did we find ourself? */ - if (streq(dent->d_name, id_filename)) + if (streq(dent->d_name, id)) continue; if (sd_device_new_from_device_id(&dev_db, dent->d_name) < 0) @@ -229,21 +229,21 @@ static size_t escape_path(const char *src, char *dest, size_t size) { static int link_update(sd_device *dev, const char *slink, bool add) { _cleanup_free_ char *filename = NULL, *dirname = NULL; char name_enc[PATH_MAX]; - const char *id_filename; + const char *id; int i, r, retries; assert(dev); assert(slink); - r = device_get_id_filename(dev, &id_filename); + r = device_get_device_id(dev, &id); if (r < 0) - return log_device_debug_errno(dev, r, "Failed to get id_filename: %m"); + return log_device_debug_errno(dev, r, "Failed to get device id: %m"); escape_path(slink + STRLEN("/dev"), name_enc, sizeof(name_enc)); dirname = path_join("/run/udev/links/", name_enc); if (!dirname) return log_oom(); - filename = path_join(dirname, id_filename); + filename = path_join(dirname, id); if (!filename) return log_oom(); @@ -348,7 +348,7 @@ int udev_node_update_old_links(sd_device *dev, sd_device *dev_old) { static int node_permissions_apply(sd_device *dev, bool apply_mac, mode_t mode, uid_t uid, gid_t gid, OrderedHashmap *seclabel_list) { - const char *devnode, *subsystem, *id_filename = NULL; + const char *devnode, *subsystem, *id = NULL; bool apply_mode, apply_uid, apply_gid; _cleanup_close_ int node_fd = -1; struct stat stats; @@ -366,7 +366,7 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, r = sd_device_get_devnum(dev, &devnum); if (r < 0) return log_device_debug_errno(dev, r, "Failed to get devnum: %m"); - (void) device_get_id_filename(dev, &id_filename); + (void) device_get_device_id(dev, &id); if (streq(subsystem, "block")) mode |= S_IFBLK; @@ -388,7 +388,7 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, if ((mode != MODE_INVALID && (stats.st_mode & S_IFMT) != (mode & S_IFMT)) || stats.st_rdev != devnum) { log_device_debug(dev, "Found node '%s' with non-matching devnum %s, skipping handling.", - devnode, id_filename); + devnode, strna(id)); return 0; /* We might process a device that already got replaced by the time we have a look * at it, handle this gracefully and step away. */ } @@ -509,10 +509,10 @@ int udev_node_add(sd_device *dev, bool apply, return log_device_debug_errno(dev, r, "Failed to get devnode: %m"); if (DEBUG_LOGGING) { - const char *id_filename = NULL; + const char *id = NULL; - (void) device_get_id_filename(dev, &id_filename); - log_device_debug(dev, "Handling device node '%s', devnum=%s", devnode, strnull(id_filename)); + (void) device_get_device_id(dev, &id); + log_device_debug(dev, "Handling device node '%s', devnum=%s", devnode, strna(id)); } r = node_permissions_apply(dev, apply, mode, uid, gid, seclabel_list); diff --git a/src/udev/udev-watch.c b/src/udev/udev-watch.c index e90fcd1bcc..dca843e3f1 100644 --- a/src/udev/udev-watch.c +++ b/src/udev/udev-watch.c @@ -37,20 +37,20 @@ int udev_watch_restore(int inotify_fd) { FOREACH_DIRENT_ALL(ent, dir, break) { _cleanup_(sd_device_unrefp) sd_device *dev = NULL; - _cleanup_free_ char *device = NULL; + _cleanup_free_ char *id = NULL; if (ent->d_name[0] == '.') continue; - r = readlinkat_malloc(dirfd(dir), ent->d_name, &device); + r = readlinkat_malloc(dirfd(dir), ent->d_name, &id); if (r < 0) { log_debug_errno(r, "Failed to read link '/run/udev/watch.old/%s', ignoring: %m", ent->d_name); goto unlink; } - r = sd_device_new_from_device_id(&dev, device); + r = sd_device_new_from_device_id(&dev, id); if (r < 0) { - log_debug_errno(r, "Failed to create sd_device object for '%s', ignoring: %m", device); + log_debug_errno(r, "Failed to create sd_device object for '%s', ignoring: %m", id); goto unlink; } @@ -68,7 +68,7 @@ unlink: int udev_watch_begin(int inotify_fd, sd_device *dev) { char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; - const char *devnode, *id_filename; + const char *devnode, *id; int wd, r; assert(inotify_fd >= 0); @@ -92,11 +92,11 @@ int udev_watch_begin(int inotify_fd, sd_device *dev) { return log_device_error_errno(dev, r, "Failed to create parent directory of '%s': %m", filename); (void) unlink(filename); - r = device_get_id_filename(dev, &id_filename); + r = device_get_device_id(dev, &id); if (r < 0) return log_device_error_errno(dev, r, "Failed to get device id-filename: %m"); - if (symlink(id_filename, filename) < 0) + if (symlink(id, filename) < 0) return log_device_error_errno(dev, errno, "Failed to create symlink %s: %m", filename); return 0; @@ -131,24 +131,24 @@ int udev_watch_end(int inotify_fd, sd_device *dev) { int udev_watch_lookup(int wd, sd_device **ret) { char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; - _cleanup_free_ char *device = NULL; + _cleanup_free_ char *id = NULL; int r; assert(wd >= 0); assert(ret); xsprintf(filename, "/run/udev/watch/%d", wd); - r = readlink_malloc(filename, &device); + r = readlink_malloc(filename, &id); if (r == -ENOENT) return 0; if (r < 0) return log_debug_errno(r, "Failed to read link '%s': %m", filename); - r = sd_device_new_from_device_id(ret, device); + r = sd_device_new_from_device_id(ret, id); if (r == -ENODEV) return 0; if (r < 0) - return log_debug_errno(r, "Failed to create sd_device object for '%s': %m", device); + return log_debug_errno(r, "Failed to create sd_device object for '%s': %m", id); return 1; } From db2bad4368517062aaf4debbba89af1bb377b9a5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 9 Mar 2021 16:11:52 +0900 Subject: [PATCH 08/16] sd-device: check the validity of device id The device id is used as filename under /run/udev/data or /run/udev/watch. --- src/libsystemd/sd-device/sd-device.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index decf9ae1d1..233bb6df5d 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1306,6 +1306,9 @@ int device_get_device_id(sd_device *device, const char **ret) { return -ENOMEM; } + if (!filename_is_valid(id)) + return -EINVAL; + device->device_id = TAKE_PTR(id); } From ff7a8d2938b24cb7ca7b69900395ecf837a43a23 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Mar 2021 15:24:15 +0900 Subject: [PATCH 09/16] sd-device: minor optimization for sd_device_new_from_device_id() --- src/libsystemd/sd-device/sd-device.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 233bb6df5d..fbb5debff4 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -623,7 +623,7 @@ _public_ int sd_device_new_from_device_id(sd_device **ret, const char *id) { struct ifreq ifr = {}; int ifindex; - r = ifr.ifr_ifindex = parse_ifindex(&id[1]); + r = ifr.ifr_ifindex = parse_ifindex(id + 1); if (r < 0) return r; @@ -652,15 +652,14 @@ _public_ int sd_device_new_from_device_id(sd_device **ret, const char *id) { } case '+': { - char subsys[PATH_MAX]; - char *sysname; + char subsys[NAME_MAX+1]; /* NAME_MAX does not include the trailing NUL. */ + const char *sysname; - (void) strscpy(subsys, sizeof(subsys), id + 1); - sysname = strchr(subsys, ':'); + sysname = strchr(id + 1, ':'); if (!sysname) return -EINVAL; - sysname[0] = '\0'; + (void) strnscpy(subsys, sizeof(subsys), id + 1, sysname - id - 1); sysname++; return sd_device_new_from_subsystem_sysname(ret, subsys, sysname); From 6e25642f8e33d6f304fcf68efd6517d29fc23e5c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 9 Mar 2021 14:31:39 +0900 Subject: [PATCH 10/16] sd-device: drop sysname_set flag When device_set_sysname() succeeds, sysname is always set. --- src/libsystemd/sd-device/device-internal.h | 1 - src/libsystemd/sd-device/sd-device.c | 13 +++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index 6776ab3aa7..73412db5de 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -84,7 +84,6 @@ struct sd_device { bool property_tags_outdated:1; /* need to update TAGS= or CURRENT_TAGS= property */ bool property_devlinks_outdated:1; /* need to update DEVLINKS= property */ bool properties_buf_outdated:1; /* need to reread hashmap */ - bool sysname_set:1; /* don't reread sysname */ bool subsystem_set:1; /* don't reread subsystem */ bool driver_subsystem_set:1; /* don't reread subsystem */ bool driver_set:1; /* don't reread driver */ diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index fbb5debff4..c51d6bffb0 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -986,7 +986,7 @@ _public_ int sd_device_get_devname(sd_device *device, const char **devname) { return 0; } -static int device_set_sysname(sd_device *device) { +static int device_set_sysname_and_sysnum(sd_device *device) { _cleanup_free_ char *sysname = NULL; const char *sysnum = NULL; const char *pos; @@ -1023,7 +1023,6 @@ static int device_set_sysname(sd_device *device) { if (len == 0) sysnum = NULL; - device->sysname_set = true; device->sysnum = sysnum; return free_and_replace(device->sysname, sysname); } @@ -1033,14 +1032,12 @@ _public_ int sd_device_get_sysname(sd_device *device, const char **ret) { assert_return(device, -EINVAL); - if (!device->sysname_set) { - r = device_set_sysname(device); + if (!device->sysname) { + r = device_set_sysname_and_sysnum(device); if (r < 0) return r; } - assert_return(device->sysname, -ENOENT); - if (ret) *ret = device->sysname; return 0; @@ -1051,8 +1048,8 @@ _public_ int sd_device_get_sysnum(sd_device *device, const char **ret) { assert_return(device, -EINVAL); - if (!device->sysname_set) { - r = device_set_sysname(device); + if (!device->sysname) { + r = device_set_sysname_and_sysnum(device); if (r < 0) return r; } From 4956f220a12ba9c00dcdfdc2273e98b71ff33bae Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 9 Mar 2021 15:27:35 +0900 Subject: [PATCH 11/16] sd-device: cleanup sd_device_get_subsystem() --- src/libsystemd/sd-device/device-internal.h | 1 - src/libsystemd/sd-device/sd-device.c | 83 ++++++++++++---------- 2 files changed, 44 insertions(+), 40 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index 73412db5de..9844608157 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -85,7 +85,6 @@ struct sd_device { bool property_devlinks_outdated:1; /* need to update DEVLINKS= property */ bool properties_buf_outdated:1; /* need to reread hashmap */ bool subsystem_set:1; /* don't reread subsystem */ - bool driver_subsystem_set:1; /* don't reread subsystem */ bool driver_set:1; /* don't reread driver */ bool uevent_loaded:1; /* don't reread uevent */ bool db_loaded; /* don't reread db */ diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index c51d6bffb0..06ef3698a5 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -751,15 +751,34 @@ int device_set_subsystem(sd_device *device, const char *_subsystem) { return free_and_replace(device->subsystem, subsystem); } -static int device_set_drivers_subsystem(sd_device *device, const char *_subsystem) { +static int device_set_drivers_subsystem(sd_device *device) { _cleanup_free_ char *subsystem = NULL; + const char *syspath, *drivers, *p; int r; assert(device); - assert(_subsystem); - assert(*_subsystem); - subsystem = strdup(_subsystem); + r = sd_device_get_syspath(device, &syspath); + if (r < 0) + return r; + + drivers = strstr(syspath, "/drivers/"); + if (!drivers) + return -EINVAL; + + for (p = drivers - 1; p >= syspath; p--) + if (*p == '/') + break; + + if (p <= syspath) + /* syspath does not start with /sys/ ?? */ + return -EINVAL; + p++; + if (p >= drivers) + /* refuse duplicated slashes */ + return -EINVAL; + + subsystem = strndup(p, drivers - p); if (!subsystem) return -ENOMEM; @@ -771,60 +790,46 @@ static int device_set_drivers_subsystem(sd_device *device, const char *_subsyste } _public_ int sd_device_get_subsystem(sd_device *device, const char **ret) { - const char *syspath, *drivers = NULL; int r; assert_return(device, -EINVAL); - r = sd_device_get_syspath(device, &syspath); - if (r < 0) - return r; - if (!device->subsystem_set) { _cleanup_free_ char *subsystem = NULL; + const char *syspath; char *path; + r = sd_device_get_syspath(device, &syspath); + if (r < 0) + return r; + /* read 'subsystem' link */ path = strjoina(syspath, "/subsystem"); r = readlink_value(path, &subsystem); - if (r >= 0) + if (r < 0 && r != -ENOENT) + return log_device_debug_errno(device, r, + "sd-device: Failed to read subsystem for %s: %m", + device->devpath); + + if (subsystem) r = device_set_subsystem(device, subsystem); /* use implicit names */ else if (path_startswith(device->devpath, "/module/")) r = device_set_subsystem(device, "module"); - else if (!(drivers = strstr(syspath, "/drivers/")) && - PATH_STARTSWITH_SET(device->devpath, "/subsystem/", + else if (strstr(syspath, "/drivers/")) + r = device_set_drivers_subsystem(device); + else if (PATH_STARTSWITH_SET(device->devpath, "/subsystem/", "/class/", "/bus/")) r = device_set_subsystem(device, "subsystem"); - if (r < 0 && r != -ENOENT) - return log_device_debug_errno(device, r, "sd-device: Failed to set subsystem for %s: %m", device->devpath); - - device->subsystem_set = true; - } else if (!device->driver_subsystem_set) - drivers = strstr(syspath, "/drivers/"); - - if (!device->driver_subsystem_set) { - if (drivers) { - _cleanup_free_ char *subpath = NULL; - - subpath = strndup(syspath, drivers - syspath); - if (!subpath) - r = -ENOMEM; - else { - const char *subsys; - - subsys = strrchr(subpath, '/'); - if (!subsys) - r = -EINVAL; - else - r = device_set_drivers_subsystem(device, subsys + 1); - } - if (r < 0 && r != -ENOENT) - return log_device_debug_errno(device, r, "sd-device: Failed to set subsystem for driver %s: %m", device->devpath); + else { + device->subsystem_set = true; + r = 0; } - - device->driver_subsystem_set = true; + if (r < 0) + return log_device_debug_errno(device, r, + "sd-device: Failed to set subsystem for %s: %m", + device->devpath); } if (!device->subsystem) From 5e0d05106747d2ee25ce19a51ed8a5669ea93bf9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 11 Mar 2021 19:41:52 +0900 Subject: [PATCH 12/16] udev: make udev_watch_end() noop when device does not have devname --- src/udev/udev-event.c | 11 +++-------- src/udev/udev-watch.c | 3 +++ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index f4dbcef959..c613916d18 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -934,8 +934,7 @@ static int event_execute_rules_on_remove( if (r < 0) log_device_debug_errno(dev, r, "Failed to delete database under /run/udev/data/, ignoring: %m"); - if (sd_device_get_devnum(dev, NULL) >= 0) - (void) udev_watch_end(inotify_fd, dev); + (void) udev_watch_end(inotify_fd, dev); r = udev_rules_apply_to_event(rules, event, timeout_usec, timeout_signal, properties_list); @@ -1006,9 +1005,8 @@ int udev_event_execute_rules( if (r < 0) log_device_warning_errno(dev, r, "Failed to copy all tags from old database entry, ignoring: %m"); - if (sd_device_get_devnum(dev, NULL) >= 0) - /* Disable watch during event processing. */ - (void) udev_watch_end(inotify_fd, event->dev_db_clone); + /* Disable watch during event processing. */ + (void) udev_watch_end(inotify_fd, event->dev_db_clone); if (action == SD_DEVICE_MOVE) { r = udev_event_on_move(event->dev); @@ -1097,9 +1095,6 @@ int udev_event_process_inotify_watch(UdevEvent *event, int inotify_fd) { if (device_for_action(dev, SD_DEVICE_REMOVE)) return 0; - if (sd_device_get_devname(dev, NULL) < 0) - return 0; - if (!event->inotify_watch) { (void) udev_watch_end(inotify_fd, dev); return 0; diff --git a/src/udev/udev-watch.c b/src/udev/udev-watch.c index dca843e3f1..f5085107be 100644 --- a/src/udev/udev-watch.c +++ b/src/udev/udev-watch.c @@ -112,6 +112,9 @@ int udev_watch_end(int inotify_fd, sd_device *dev) { if (inotify_fd < 0) return 0; + if (sd_device_get_devname(dev, NULL) < 0) + return 0; + r = device_get_watch_handle(dev, &wd); if (r == -ENOENT) return 0; From e7f781e473f5119bf9246208a6de9f6b76a39c5d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Mar 2021 15:35:33 +0900 Subject: [PATCH 13/16] udev,sd_device: also save map from device ID to watch handle in /run/udev/watch Previously, watch handle is saved in the udev databse. But in most cases, the handle saved in the database is not updated. Especially, when udevd is restarted, the inotify watch is restarted, but the database is not updated. Moreover, it is not necessary to save watch handle in the database, as the handle is only take a effect during udevd is running, and the value is meaningless when udevd is restarted. So, this makes the opposite map from device ID to watch handle is saved in /run/udev/watch as a symbolic link, and the handle not saved in the database anymore. Fixes #18525. --- src/libsystemd/sd-device/device-private.c | 139 +++++++++++++++++++--- src/libsystemd/sd-device/device-private.h | 8 +- src/libsystemd/sd-device/sd-device.c | 8 +- src/udev/udev-event.c | 19 +-- src/udev/udev-watch.c | 107 ++++++----------- src/udev/udev-watch.h | 1 - src/udev/udevd.c | 12 +- 7 files changed, 184 insertions(+), 110 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index cac7b98518..e612dbe0a0 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -20,6 +20,7 @@ #include "parse-util.h" #include "path-util.h" #include "set.h" +#include "stdio-util.h" #include "string-table.h" #include "string-util.h" #include "strv.h" @@ -568,28 +569,142 @@ int device_get_devlink_priority(sd_device *device, int *priority) { return 0; } -int device_get_watch_handle(sd_device *device, int *handle) { +int device_get_watch_handle(sd_device *device) { + char path_wd[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; + _cleanup_free_ char *buf = NULL; + const char *id, *path_id; + int wd, r; + + assert(device); + + if (device->watch_handle >= 0) + return device->watch_handle; + + r = device_get_device_id(device, &id); + if (r < 0) + return r; + + path_id = strjoina("/run/udev/watch/", id); + r = readlink_malloc(path_id, &buf); + if (r < 0) + return r; + + r = safe_atoi(buf, &wd); + if (r < 0) + return r; + + if (wd < 0) + return -EBADF; + + buf = mfree(buf); + xsprintf(path_wd, "/run/udev/watch/%d", wd); + r = readlink_malloc(path_wd, &buf); + if (r < 0) + return r; + + if (!streq(buf, id)) + return -EBADF; + + return device->watch_handle = wd; +} + +static void device_remove_watch_handle(sd_device *device) { + const char *id; + int wd; + + assert(device); + + /* First, remove the symlink from handle to device id. */ + wd = device_get_watch_handle(device); + if (wd >= 0) { + char path_wd[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; + + xsprintf(path_wd, "/run/udev/watch/%d", wd); + if (unlink(path_wd) < 0 && errno != ENOENT) + log_device_debug_errno(device, errno, + "sd-device: failed to remove %s, ignoring: %m", + path_wd); + } + + /* Next, remove the symlink from device id to handle. */ + if (device_get_device_id(device, &id) >= 0) { + const char *path_id; + + path_id = strjoina("/run/udev/watch/", id); + if (unlink(path_id) < 0 && errno != ENOENT) + log_device_debug_errno(device, errno, + "sd-device: failed to remove %s, ignoring: %m", + path_id); + } + + device->watch_handle = -1; +} + +int device_set_watch_handle(sd_device *device, int wd) { + char path_wd[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; + const char *id, *path_id; int r; assert(device); - r = device_read_db(device); + if (wd >= 0 && wd == device->watch_handle) + return 0; + + device_remove_watch_handle(device); + + if (wd < 0) + /* negative wd means that the caller requests to clear saved watch handle. */ + return 0; + + r = device_get_device_id(device, &id); if (r < 0) return r; - if (device->watch_handle < 0) - return -ENOENT; + path_id = strjoina("/run/udev/watch/", id); + xsprintf(path_wd, "/run/udev/watch/%d", wd); - if (handle) - *handle = device->watch_handle; + r = mkdir_parents(path_wd, 0755); + if (r < 0) + return r; + + if (symlink(id, path_wd) < 0) + return -errno; + + if (symlink(path_wd + STRLEN("/run/udev/watch/"), path_id) < 0) { + r = -errno; + if (unlink(path_wd) < 0 && errno != ENOENT) + log_device_debug_errno(device, errno, + "sd-device: failed to remove %s, ignoring: %m", + path_wd); + return r; + } + + device->watch_handle = wd; return 0; } -void device_set_watch_handle(sd_device *device, int handle) { - assert(device); +int device_new_from_watch_handle_at(sd_device **ret, int dirfd, int wd) { + char path_wd[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; + _cleanup_free_ char *id = NULL; + int r; - device->watch_handle = handle; + assert(ret); + + if (wd < 0) + return -EBADF; + + if (dirfd >= 0) { + xsprintf(path_wd, "%d", wd); + r = readlinkat_malloc(dirfd, path_wd, &id); + } else { + xsprintf(path_wd, "/run/udev/watch/%d", wd); + r = readlink_malloc(path_wd, &id); + } + if (r < 0) + return r; + + return sd_device_new_from_device_id(ret, id); } int device_rename(sd_device *device, const char *name) { @@ -822,9 +937,6 @@ static bool device_has_info(sd_device *device) { if (!set_isempty(device->current_tags)) return true; - if (device->watch_handle >= 0) - return true; - return false; } @@ -899,9 +1011,6 @@ int device_update_db(sd_device *device) { if (device->devlink_priority != 0) fprintf(f, "L:%i\n", device->devlink_priority); - - if (device->watch_handle >= 0) - fprintf(f, "W:%i\n", device->watch_handle); } if (device->usec_initialized > 0) diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 8eafc7ef74..fe268d7f2f 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -12,18 +12,22 @@ int device_new_from_nulstr(sd_device **ret, uint8_t *nulstr, size_t len); int device_new_from_strv(sd_device **ret, char **strv); +int device_new_from_watch_handle_at(sd_device **ret, int dirfd, int wd); +static inline int device_new_from_watch_handle(sd_device **ret, int wd) { + return device_new_from_watch_handle_at(ret, -1, wd); +} int device_get_device_id(sd_device *device, const char **ret); int device_get_devlink_priority(sd_device *device, int *priority); -int device_get_watch_handle(sd_device *device, int *handle); +int device_get_watch_handle(sd_device *device); int device_get_devnode_mode(sd_device *device, mode_t *mode); int device_get_devnode_uid(sd_device *device, uid_t *uid); int device_get_devnode_gid(sd_device *device, gid_t *gid); void device_seal(sd_device *device); void device_set_is_initialized(sd_device *device); -void device_set_watch_handle(sd_device *device, int fd); +int device_set_watch_handle(sd_device *device, int wd); void device_set_db_persist(sd_device *device); void device_set_devlink_priority(sd_device *device, int priority); int device_ensure_usec_initialized(sd_device *device, sd_device *device_old); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 06ef3698a5..f2e8c2112c 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1238,10 +1238,10 @@ static int handle_db_line(sd_device *device, char key, const char *value) { break; case 'W': - r = safe_atoi(value, &device->watch_handle); - if (r < 0) - return r; - + /* Deprecated. Previously, watch handle is both saved in database and /run/udev/watch. + * However, the handle saved in database may not be updated when the handle is updated + * or removed. Moreover, it is not necessary to store the handle within the database, + * as its value becomes meaningless when udevd is restarted. */ break; case 'V': r = safe_atou(value, &device->database_version); diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index c613916d18..f40b3120ca 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -997,6 +997,9 @@ int udev_event_execute_rules( if (action == SD_DEVICE_REMOVE) return event_execute_rules_on_remove(event, inotify_fd, timeout_usec, timeout_signal, properties_list, rules); + /* Disable watch during event processing. */ + (void) udev_watch_end(inotify_fd, event->dev); + r = device_clone_with_db(dev, &event->dev_db_clone); if (r < 0) return log_device_debug_errno(dev, r, "Failed to clone sd_device object: %m"); @@ -1005,9 +1008,6 @@ int udev_event_execute_rules( if (r < 0) log_device_warning_errno(dev, r, "Failed to copy all tags from old database entry, ignoring: %m"); - /* Disable watch during event processing. */ - (void) udev_watch_end(inotify_fd, event->dev_db_clone); - if (action == SD_DEVICE_MOVE) { r = udev_event_on_move(event->dev); if (r < 0) @@ -1083,7 +1083,6 @@ void udev_event_execute_run(UdevEvent *event, usec_t timeout_usec, int timeout_s int udev_event_process_inotify_watch(UdevEvent *event, int inotify_fd) { sd_device *dev; - int r; assert(event); assert(inotify_fd >= 0); @@ -1095,16 +1094,10 @@ int udev_event_process_inotify_watch(UdevEvent *event, int inotify_fd) { if (device_for_action(dev, SD_DEVICE_REMOVE)) return 0; - if (!event->inotify_watch) { + if (event->inotify_watch) + (void) udev_watch_begin(inotify_fd, dev); + else (void) udev_watch_end(inotify_fd, dev); - return 0; - } - - (void) udev_watch_begin(inotify_fd, dev); - - r = device_update_db(dev); - if (r < 0) - return log_device_debug_errno(dev, r, "Failed to update database under /run/udev/data/: %m"); return 0; } diff --git a/src/udev/udev-watch.c b/src/udev/udev-watch.c index f5085107be..7f15c87870 100644 --- a/src/udev/udev-watch.c +++ b/src/udev/udev-watch.c @@ -5,52 +5,55 @@ */ #include -#include #include "alloc-util.h" #include "device-private.h" #include "device-util.h" #include "dirent-util.h" #include "fs-util.h" -#include "mkdir.h" -#include "stdio-util.h" +#include "parse-util.h" #include "udev-watch.h" -/* Move any old watches directory out of the way, and then restore the watches. */ int udev_watch_restore(int inotify_fd) { struct dirent *ent; DIR *dir; int r; + /* Move any old watches directory out of the way, and then restore the watches. */ + assert(inotify_fd >= 0); if (rename("/run/udev/watch", "/run/udev/watch.old") < 0) { if (errno != ENOENT) - return log_warning_errno(errno, "Failed to move watches directory /run/udev/watch. Old watches will not be restored: %m"); + return log_warning_errno(errno, "Failed to move watches directory /run/udev/watch. " + "Old watches will not be restored: %m"); return 0; } dir = opendir("/run/udev/watch.old"); if (!dir) - return log_warning_errno(errno, "Failed to open old watches directory /run/udev/watch.old. Old watches will not be restored: %m"); + return log_warning_errno(errno, "Failed to open old watches directory /run/udev/watch.old. " + "Old watches will not be restored: %m"); FOREACH_DIRENT_ALL(ent, dir, break) { _cleanup_(sd_device_unrefp) sd_device *dev = NULL; - _cleanup_free_ char *id = NULL; + int wd; if (ent->d_name[0] == '.') continue; - r = readlinkat_malloc(dirfd(dir), ent->d_name, &id); - if (r < 0) { - log_debug_errno(r, "Failed to read link '/run/udev/watch.old/%s', ignoring: %m", ent->d_name); - goto unlink; - } + /* For backward compatibility, read symlink from watch handle to device id, and ignore + * the opposite direction symlink. */ - r = sd_device_new_from_device_id(&dev, id); + if (safe_atoi(ent->d_name, &wd) < 0) + goto unlink; + + r = device_new_from_watch_handle_at(&dev, dirfd(dir), wd); if (r < 0) { - log_debug_errno(r, "Failed to create sd_device object for '%s', ignoring: %m", id); + log_full_errno(r == -ENODEV ? LOG_DEBUG : LOG_WARNING, r, + "Failed to create sd_device object from saved watch handle '%s', ignoring: %m", + ent->d_name); goto unlink; } @@ -67,8 +70,7 @@ unlink: } int udev_watch_begin(int inotify_fd, sd_device *dev) { - char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; - const char *devnode, *id; + const char *devnode; int wd, r; assert(inotify_fd >= 0); @@ -76,35 +78,27 @@ int udev_watch_begin(int inotify_fd, sd_device *dev) { r = sd_device_get_devname(dev, &devnode); if (r < 0) - return log_device_error_errno(dev, r, "Failed to get device name: %m"); + return log_device_debug_errno(dev, r, "Failed to get device name: %m"); log_device_debug(dev, "Adding watch on '%s'", devnode); wd = inotify_add_watch(inotify_fd, devnode, IN_CLOSE_WRITE); - if (wd < 0) - return log_device_full_errno(dev, errno == ENOENT ? LOG_DEBUG : LOG_ERR, errno, - "Failed to add device '%s' to watch: %m", devnode); + if (wd < 0) { + r = log_device_full_errno(dev, errno == ENOENT ? LOG_DEBUG : LOG_WARNING, + errno, "Failed to add device '%s' to watch: %m", devnode); - device_set_watch_handle(dev, wd); + (void) device_set_watch_handle(dev, -1); + return r; + } - xsprintf(filename, "/run/udev/watch/%d", wd); - r = mkdir_parents(filename, 0755); + r = device_set_watch_handle(dev, wd); if (r < 0) - return log_device_error_errno(dev, r, "Failed to create parent directory of '%s': %m", filename); - (void) unlink(filename); - - r = device_get_device_id(dev, &id); - if (r < 0) - return log_device_error_errno(dev, r, "Failed to get device id-filename: %m"); - - if (symlink(id, filename) < 0) - return log_device_error_errno(dev, errno, "Failed to create symlink %s: %m", filename); + return log_device_warning_errno(dev, r, "Failed to save watch handle in /run/udev/watch: %m"); return 0; } int udev_watch_end(int inotify_fd, sd_device *dev) { - char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; - int wd, r; + int wd; assert(dev); @@ -115,43 +109,14 @@ int udev_watch_end(int inotify_fd, sd_device *dev) { if (sd_device_get_devname(dev, NULL) < 0) return 0; - r = device_get_watch_handle(dev, &wd); - if (r == -ENOENT) - return 0; - if (r < 0) - return log_device_debug_errno(dev, r, "Failed to get watch handle, ignoring: %m"); - - log_device_debug(dev, "Removing watch"); - (void) inotify_rm_watch(inotify_fd, wd); - - xsprintf(filename, "/run/udev/watch/%d", wd); - (void) unlink(filename); - - device_set_watch_handle(dev, -1); + wd = device_get_watch_handle(dev); + if (wd < 0) + log_device_debug_errno(dev, wd, "Failed to get watch handle, ignoring: %m"); + else { + log_device_debug(dev, "Removing watch"); + (void) inotify_rm_watch(inotify_fd, wd); + } + (void) device_set_watch_handle(dev, -1); return 0; } - -int udev_watch_lookup(int wd, sd_device **ret) { - char filename[STRLEN("/run/udev/watch/") + DECIMAL_STR_MAX(int)]; - _cleanup_free_ char *id = NULL; - int r; - - assert(wd >= 0); - assert(ret); - - xsprintf(filename, "/run/udev/watch/%d", wd); - r = readlink_malloc(filename, &id); - if (r == -ENOENT) - return 0; - if (r < 0) - return log_debug_errno(r, "Failed to read link '%s': %m", filename); - - r = sd_device_new_from_device_id(ret, id); - if (r == -ENODEV) - return 0; - if (r < 0) - return log_debug_errno(r, "Failed to create sd_device object for '%s': %m", id); - - return 1; -} diff --git a/src/udev/udev-watch.h b/src/udev/udev-watch.h index f5c55794c4..d211d99f49 100644 --- a/src/udev/udev-watch.h +++ b/src/udev/udev-watch.h @@ -6,4 +6,3 @@ int udev_watch_restore(int inotify_fd); int udev_watch_begin(int inotify_fd, sd_device *dev); int udev_watch_end(int inotify_fd, sd_device *dev); -int udev_watch_lookup(int wd, sd_device **ret); diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 9689e15b1c..5574abce9d 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1302,17 +1302,21 @@ static int on_inotify(sd_event_source *s, int fd, uint32_t revents, void *userda _cleanup_(sd_device_unrefp) sd_device *dev = NULL; const char *devnode; - if (udev_watch_lookup(e->wd, &dev) <= 0) + r = device_new_from_watch_handle(&dev, e->wd); + if (r < 0) { + log_debug_errno(r, "Failed to create sd_device object from watch handle, ignoring: %m"); continue; + } if (sd_device_get_devname(dev, &devnode) < 0) continue; log_device_debug(dev, "Inotify event: %x for %s", e->mask, devnode); if (e->mask & IN_CLOSE_WRITE) - synthesize_change(dev); - else if (e->mask & IN_IGNORED) - udev_watch_end(manager->inotify_fd, dev); + (void) synthesize_change(dev); + + /* Do not handle IN_IGNORED here. It should be handled by worker in 'remove' uevent; + * udev_event_execute_rules() -> event_execute_rules_on_remove() -> udev_watch_end(). */ } return 1; From e1e56f3443eff430f080fd908435286aa6c5dc62 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 11 Mar 2021 00:33:03 +0900 Subject: [PATCH 14/16] test: run udev tests after currently queued events are finished --- test/units/testsuite-17.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/units/testsuite-17.sh b/test/units/testsuite-17.sh index 65d0802969..92ff7b56a6 100755 --- a/test/units/testsuite-17.sh +++ b/test/units/testsuite-17.sh @@ -5,6 +5,8 @@ set -o pipefail : >/failed +udevadm settle + for t in "${0%.sh}".*.sh; do echo "Running $t"; ./"$t" done From e63916ab29ec7133e4f8a466fb8ca7814e1e258c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Mar 2021 14:18:07 +0900 Subject: [PATCH 15/16] test: generate debugging logs for udev tests --- test/units/testsuite-17.01.sh | 2 ++ test/units/testsuite-17.02.sh | 1 + test/units/testsuite-17.03.sh | 5 ++++- test/units/testsuite-17.04.sh | 1 + 4 files changed, 8 insertions(+), 1 deletion(-) diff --git a/test/units/testsuite-17.01.sh b/test/units/testsuite-17.01.sh index 8be27d9507..3b099fc2a4 100755 --- a/test/units/testsuite-17.01.sh +++ b/test/units/testsuite-17.01.sh @@ -20,6 +20,7 @@ while : ; do done cat >/run/udev/rules.d/50-testsuite.rules </run/udev/rules.d/50-testsuite.rules </run/udev/rules.d/50-testsuite.rules <"${test_rule}" + cat >"${test_rule}" <>/etc/udev/udev.conf echo "timeout_signal=SIGABRT" >>/etc/udev/udev.conf diff --git a/test/units/testsuite-17.04.sh b/test/units/testsuite-17.04.sh index b33d47ef6d..fbe8971732 100755 --- a/test/units/testsuite-17.04.sh +++ b/test/units/testsuite-17.04.sh @@ -9,6 +9,7 @@ test ! -f /run/udev/tags/changed/c1:3 udevadm info /dev/null | grep -E 'E: (TAGS|CURRENT_TAGS)=.*:(added|changed):' && exit 1 cat >/run/udev/rules.d/50-testsuite.rules < Date: Mon, 8 Mar 2021 05:21:45 +0900 Subject: [PATCH 16/16] test: add a simple test for udev watch --- test/TEST-17-UDEV/test.sh | 2 +- test/units/testsuite-17.06.sh | 75 +++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100755 test/units/testsuite-17.06.sh diff --git a/test/TEST-17-UDEV/test.sh b/test/TEST-17-UDEV/test.sh index 43119d4c10..446f4a83db 100755 --- a/test/TEST-17-UDEV/test.sh +++ b/test/TEST-17-UDEV/test.sh @@ -8,7 +8,7 @@ TEST_NO_NSPAWN=1 # shellcheck source=test/test-functions . "${TEST_BASE_DIR:?}/test-functions" -QEMU_TIMEOUT=500 +QEMU_TIMEOUT=800 test_append_files() { ( diff --git a/test/units/testsuite-17.06.sh b/test/units/testsuite-17.06.sh new file mode 100755 index 0000000000..68c1f7b298 --- /dev/null +++ b/test/units/testsuite-17.06.sh @@ -0,0 +1,75 @@ +#!/bin/bash +set -ex +set -o pipefail + +# tests for udev watch + +function check_validity() { + local f ID_OR_HANDLE + + for f in /run/udev/watch/*; do + ID_OR_HANDLE=$(readlink $f) + test -L /run/udev/watch/${ID_OR_HANDLE} + test $(readlink /run/udev/watch/${ID_OR_HANDLE}) = $(basename $f) + done +} + +function check() { + local i j + + for ((i=0;i<2;i++)); do + systemctl restart systemd-udevd.service + udevadm control --ping + udevadm settle + sleep 1 + check_validity + + for ((j=0;j<2;j++)); do + udevadm trigger -w --action add --subsystem-match=block + udevadm settle + sleep 1 + check_validity + done + + for ((j=0;j<2;j++)); do + udevadm trigger -w --action change --subsystem-match=block + udevadm settle + sleep 1 + check_validity + done + done +} + +mkdir -p /run/udev/rules.d/ + +cat >/run/udev/rules.d/00-debug.rules </run/udev/rules.d/50-testsuite.rules </run/udev/rules.d/50-testsuite.rules <