From ef9ca572e1f37c046c39e7710f8031a2d2c09457 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 06:35:22 +0900 Subject: [PATCH 01/10] core/device: use DEVICE_NOT_FOUND --- src/core/device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index d6710262a9..14862d9494 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -727,7 +727,7 @@ static int device_setup_devlink_unit_one(Manager *m, const char *devlink, sd_dev if (sd_device_new_from_devname(&dev, devlink) < 0 || !device_is_ready(dev)) { /* the devlink is already removed or not ready */ - device_update_found_by_name(m, devlink, 0, DEVICE_FOUND_UDEV); + device_update_found_by_name(m, devlink, DEVICE_NOT_FOUND, DEVICE_FOUND_UDEV); *ret = NULL; return 0; /* not ready */ } @@ -1140,7 +1140,7 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * if (action == SD_DEVICE_REMOVE) /* If we get notified that a device was removed by udev, then it's completely gone, hence * unset all found bits */ - device_update_found_by_sysfs(m, sysfs, 0, DEVICE_FOUND_MASK); + device_update_found_by_sysfs(m, sysfs, DEVICE_NOT_FOUND, DEVICE_FOUND_MASK); else if (ready) /* The device is found now, set the udev found bit */ device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); From 1cb89339bcf44d910931655c689d23b752dc8cce Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 09:02:39 +0900 Subject: [PATCH 02/10] core/device: removed devices are not ready --- src/core/device.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 14862d9494..b7fbf8a8e6 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -692,6 +692,9 @@ static bool device_is_ready(sd_device *dev) { assert(dev); + if (device_for_action(dev, SD_DEVICE_REMOVE)) + return false; + r = device_is_renaming(dev); if (r < 0) log_device_warning_errno(dev, r, "Failed to check if device is renaming, assuming device is not renaming: %m"); @@ -1103,18 +1106,16 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * /* A change event can signal that a device is becoming ready, in particular if the device is using * the SYSTEMD_READY logic in udev so we need to reach the else block of the following if, even for * change events */ + ready = device_is_ready(dev); + if (action == SD_DEVICE_REMOVE) { r = swap_process_device_remove(m, dev); if (r < 0) log_device_warning_errno(dev, r, "Failed to process swap device remove event, ignoring: %m"); - ready = false; - (void) device_setup_devlink_units_on_remove(m, dev, &ready_devlinks); } else { - ready = device_is_ready(dev); - if (ready) { device_process_new(m, dev, sysfs); From c072b84c7ed4ee9fee5e2940e28d60740cbfe48a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 09:01:19 +0900 Subject: [PATCH 03/10] core/device: introduce device_by_path() helper function --- src/core/device.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index b7fbf8a8e6..e34141b18e 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -32,6 +32,27 @@ static const UnitActiveState state_translation_table[_DEVICE_STATE_MAX] = { static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata); +static int device_by_path(Manager *m, const char *path, Unit **ret) { + _cleanup_free_ char *e = NULL; + Unit *u; + int r; + + assert(m); + assert(path); + + r = unit_name_from_path(path, ".device", &e); + if (r < 0) + return r; + + u = manager_get_unit(m, e); + if (!u) + return -ENOENT; + + if (ret) + *ret = u; + return 0; +} + static void device_unset_sysfs(Device *d) { Hashmap *devices; Device *first; @@ -214,9 +235,7 @@ static void device_update_found_by_sysfs(Manager *m, const char *sysfs, DeviceFo } static void device_update_found_by_name(Manager *m, const char *path, DeviceFound found, DeviceFound mask) { - _cleanup_free_ char *e = NULL; Unit *u; - int r; assert(m); assert(path); @@ -224,12 +243,7 @@ static void device_update_found_by_name(Manager *m, const char *path, DeviceFoun if (mask == 0) return; - r = unit_name_from_path(path, ".device", &e); - if (r < 0) - return (void) log_debug_errno(r, "Failed to generate unit name from device path, ignoring: %m"); - - u = manager_get_unit(m, e); - if (!u) + if (device_by_path(m, path, &u) < 0) return; device_update_found_one(DEVICE(u), found, mask); @@ -1034,19 +1048,13 @@ static void device_propagate_reload_by_sysfs(Manager *m, const char *sysfs) { } static void device_propagate_reload_by_name(Manager *m, const char *path) { - _cleanup_free_ char *e = NULL; Unit *u; int r; assert(m); assert(path); - r = unit_name_from_path(path, ".device", &e); - if (r < 0) - return (void) log_debug_errno(r, "Failed to generate unit name from device path, ignoring: %m"); - - u = manager_get_unit(m, e); - if (!u) + if (device_by_path(m, path, &u) < 0) return; if (DEVICE(u)->state == DEVICE_DEAD) From 9616f550b89029b3656f9938261575abac280611 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 10:21:30 +0900 Subject: [PATCH 04/10] core/device: introduce device_propagate_reload() --- src/core/device.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index e34141b18e..442efa6ba2 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -1029,22 +1029,29 @@ fail: device_shutdown(m); } +static void device_propagate_reload(Manager *m, Device *d) { + int r; + + assert(m); + assert(d); + + if (d->state == DEVICE_DEAD) + return; + + r = manager_propagate_reload(m, UNIT(d), JOB_REPLACE, NULL); + if (r < 0) + log_unit_warning_errno(UNIT(d), r, "Failed to propagate reload, ignoring: %m"); +} + static void device_propagate_reload_by_sysfs(Manager *m, const char *sysfs) { Device *l; - int r; assert(m); assert(sysfs); l = hashmap_get(m->devices_by_sysfs, sysfs); - LIST_FOREACH(same_sysfs, d, l) { - if (d->state == DEVICE_DEAD) - continue; - - r = manager_propagate_reload(m, UNIT(d), JOB_REPLACE, NULL); - if (r < 0) - log_unit_warning_errno(UNIT(d), r, "Failed to propagate reload, ignoring: %m"); - } + LIST_FOREACH(same_sysfs, d, l) + device_propagate_reload(m, d); } static void device_propagate_reload_by_name(Manager *m, const char *path) { @@ -1057,12 +1064,7 @@ static void device_propagate_reload_by_name(Manager *m, const char *path) { if (device_by_path(m, path, &u) < 0) return; - if (DEVICE(u)->state == DEVICE_DEAD) - return; - - r = manager_propagate_reload(m, u, JOB_REPLACE, NULL); - if (r < 0) - log_unit_warning_errno(u, r, "Failed to propagate reload, ignoring: %m"); + device_propagate_reload(m, DEVICE(u)); } static void device_remove_old_on_move(Manager *m, sd_device *dev) { From 4228306b9d50df9a804859d00e84588a9fc4c4b9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 01:17:27 +0900 Subject: [PATCH 05/10] core/device: always update existing devlink or alias units on uevent Previously, existing device units for devlinks or aliases were not removed unless the main device unit is removed. This makes all existing device units for devlinks and aliases are checked if they are still required, and remove if not necessary anymore. Fixes #24518. --- src/core/device.c | 341 +++++++++++++++++++++------------------------- 1 file changed, 159 insertions(+), 182 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 442efa6ba2..ebe1a934f4 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -620,7 +620,7 @@ static void device_upgrade_mount_deps(Unit *u) { } } -static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool main) { +static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool main, Set **units) { _cleanup_(unit_freep) Unit *new_unit = NULL; _cleanup_free_ char *e = NULL; const char *sysfs = NULL; @@ -697,6 +697,12 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool if (dev && device_is_bound_by_mounts(DEVICE(u), dev)) device_upgrade_mount_deps(u); + if (units) { + r = set_ensure_put(units, NULL, DEVICE(u)); + if (r < 0) + return log_unit_error_errno(u, r, "Failed to store unit: %m"); + } + TAKE_PTR(new_unit); return 0; } @@ -735,153 +741,169 @@ static bool device_is_ready(sd_device *dev) { return r != 0; } -static int device_setup_devlink_unit_one(Manager *m, const char *devlink, sd_device **ret) { +static int device_setup_devlink_unit_one(Manager *m, const char *devlink, Set **ready_units, Set **not_ready_units) { _cleanup_(sd_device_unrefp) sd_device *dev = NULL; - int r; + Unit *u; assert(m); assert(devlink); + assert(ready_units); + assert(not_ready_units); - if (sd_device_new_from_devname(&dev, devlink) < 0 || !device_is_ready(dev)) { - /* the devlink is already removed or not ready */ - device_update_found_by_name(m, devlink, DEVICE_NOT_FOUND, DEVICE_FOUND_UDEV); - *ret = NULL; - return 0; /* not ready */ - } + if (sd_device_new_from_devname(&dev, devlink) >= 0 && device_is_ready(dev)) + return device_setup_unit(m, dev, devlink, /* main = */ false, ready_units); - r = device_setup_unit(m, dev, devlink, /* main = */ false); - if (r < 0) - return log_device_warning_errno(dev, r, "Failed to setup unit for '%s': %m", devlink); + /* the devlink is already removed or not ready */ + if (device_by_path(m, devlink, &u) < 0) + return 0; /* The corresponding .device unit not found. That's fine. */ - *ret = TAKE_PTR(dev); - return 1; /* ready */ + return set_ensure_put(not_ready_units, NULL, DEVICE(u)); } -static int device_setup_devlink_units(Manager *m, sd_device *dev, char ***ret_ready_devlinks) { - _cleanup_strv_free_ char **ready_devlinks = NULL; - const char *devlink, *syspath; - int r; - - assert(m); - assert(dev); - assert(ret_ready_devlinks); - - r = sd_device_get_syspath(dev, &syspath); - if (r < 0) - return r; - - FOREACH_DEVICE_DEVLINK(dev, devlink) { - _cleanup_(sd_device_unrefp) sd_device *assigned = NULL; - const char *s; - - if (PATH_STARTSWITH_SET(devlink, "/dev/block/", "/dev/char/")) - continue; - - if (device_setup_devlink_unit_one(m, devlink, &assigned) <= 0) - continue; - - if (sd_device_get_syspath(assigned, &s) < 0) - continue; - - if (path_equal(s, syspath)) - continue; - - r = strv_extend(&ready_devlinks, devlink); - if (r < 0) - return -ENOMEM; - } - - *ret_ready_devlinks = TAKE_PTR(ready_devlinks); - return 0; -} - -static int device_setup_devlink_units_on_remove(Manager *m, sd_device *dev, char ***ret_ready_devlinks) { - _cleanup_strv_free_ char **ready_devlinks = NULL; - const char *syspath; +static int device_setup_extra_units(Manager *m, sd_device *dev, Set **ready_units, Set **not_ready_units) { + _cleanup_strv_free_ char **aliases = NULL; + const char *devlink, *syspath, *devname = NULL; Device *l; int r; assert(m); assert(dev); - assert(ret_ready_devlinks); + assert(ready_units); + assert(not_ready_units); r = sd_device_get_syspath(dev, &syspath); if (r < 0) return r; - l = hashmap_get(m->devices_by_sysfs, syspath); - LIST_FOREACH(same_sysfs, d, l) { - _cleanup_(sd_device_unrefp) sd_device *assigned = NULL; + (void) sd_device_get_devname(dev, &devname); + + /* devlink units */ + FOREACH_DEVICE_DEVLINK(dev, devlink) { + /* These are a kind of special devlink. They should be always unique, but neither persistent + * nor predictable. Hence, let's refuse them. See also the comments for alias units below. */ + if (PATH_STARTSWITH_SET(devlink, "/dev/block/", "/dev/char/")) + continue; + + (void) device_setup_devlink_unit_one(m, devlink, ready_units, not_ready_units); + } + + if (device_is_ready(dev)) { const char *s; + r = sd_device_get_property_value(dev, "SYSTEMD_ALIAS", &s); + if (r < 0 && r != -ENOENT) + log_device_warning_errno(dev, r, "Failed to get SYSTEMD_ALIAS property, ignoring: %m"); + if (r >= 0) { + r = strv_split_full(&aliases, s, NULL, EXTRACT_UNQUOTE); + if (r < 0) + log_device_warning_errno(dev, r, "Failed to parse SYSTEMD_ALIAS property, ignoring: %m"); + } + } + + /* alias units */ + STRV_FOREACH(alias, aliases) { + if (!path_is_absolute(*alias)) { + log_device_warning(dev, "The alias \"%s\" specified in SYSTEMD_ALIAS is not an absolute path, ignoring.", *alias); + continue; + } + + if (!path_is_normalized(*alias)) { + log_device_warning(dev, "The alias \"%s\" specified in SYSTEMD_ALIAS is not a normalized path, ignoring.", *alias); + continue; + } + + /* Note, even if the devlink is not persistent, LVM expects /dev/block/ symlink units exist. + * To achieve that, they set the path to SYSTEMD_ALIAS. Hence, we cannot refuse aliases start + * with /dev/, unfortunately. */ + + (void) device_setup_unit(m, dev, *alias, /* main = */ false, ready_units); + } + + l = hashmap_get(m->devices_by_sysfs, syspath); + LIST_FOREACH(same_sysfs, d, l) { if (!d->path) continue; - if (!path_startswith(d->path, "/dev/")) - continue; + if (path_equal(d->path, syspath)) + continue; /* This is the main unit. */ - if (device_setup_devlink_unit_one(m, d->path, &assigned) <= 0) - continue; + if (devname && path_equal(d->path, devname)) + continue; /* This is the real device node. */ - if (sd_device_get_syspath(assigned, &s) < 0) - continue; + if (device_has_devlink(dev, d->path)) + continue; /* The devlink was already processed in the above loop. */ - if (path_equal(s, syspath)) - continue; + if (strv_contains(aliases, d->path)) + continue; /* This is already processed in the above, and ready. */ - r = strv_extend(&ready_devlinks, d->path); - if (r < 0) - return -ENOMEM; + if (path_startswith(d->path, "/dev/")) + /* This is a devlink unit. Check existence and update syspath. */ + (void) device_setup_devlink_unit_one(m, d->path, ready_units, not_ready_units); + else + /* This is an alias unit of dropped or not ready device. */ + (void) set_ensure_put(not_ready_units, NULL, d); } - *ret_ready_devlinks = TAKE_PTR(ready_devlinks); return 0; } -static void device_process_new(Manager *m, sd_device *dev, const char *sysfs) { - const char *dn, *alias; +static int device_setup_units(Manager *m, sd_device *dev, Set **ready_units, Set **not_ready_units) { + const char *syspath, *devname = NULL; int r; assert(m); assert(dev); - assert(sysfs); + assert(ready_units); + assert(not_ready_units); - /* Add the main unit named after the sysfs path. If this one fails, don't bother with the rest, as - * this one shall be the main device unit the others just follow. (Compare with how - * device_following() is implemented, see below, which looks for the sysfs device.) */ - if (device_setup_unit(m, dev, sysfs, /* main = */ true) < 0) - return; + r = sd_device_get_syspath(dev, &syspath); + if (r < 0) + return log_device_debug_errno(dev, r, "Couldn't get syspath from device, ignoring: %m"); - /* Add an additional unit for the device node */ - if (sd_device_get_devname(dev, &dn) >= 0) - (void) device_setup_unit(m, dev, dn, /* main = */ false); - - /* Add additional units for all explicitly configured aliases */ - r = sd_device_get_property_value(dev, "SYSTEMD_ALIAS", &alias); - if (r < 0) { - if (r != -ENOENT) - log_device_error_errno(dev, r, "Failed to get SYSTEMD_ALIAS property, ignoring: %m"); - return; - } - - for (;;) { - _cleanup_free_ char *word = NULL; - - r = extract_first_word(&alias, &word, NULL, EXTRACT_UNQUOTE); - if (r == 0) - break; - if (r == -ENOMEM) - return (void) log_oom(); + /* First, process the main (that is, points to the syspath) and (real, not symlink) devnode units. */ + if (device_for_action(dev, SD_DEVICE_REMOVE)) + /* If the device is removed, the main and devnode units units will be removed by + * device_update_found_by_sysfs() in device_dispatch_io(). Hence, it is not necessary to + * store them to not_ready_units, and we have nothing to do here. + * + * Note, still we need to process devlink units below, as a devlink previously points to this + * device may still exist and now point to another device node. That is, do not forget to + * call device_setup_extra_units(). */ + ; + else if (device_is_ready(dev)) { + /* Add the main unit named after the syspath. If this one fails, don't bother with the rest, + * as this one shall be the main device unit the others just follow. (Compare with how + * device_following() is implemented, see below, which looks for the sysfs device.) */ + r = device_setup_unit(m, dev, syspath, /* main = */ true, ready_units); if (r < 0) - return (void) log_device_warning_errno(dev, r, "Failed to parse SYSTEMD_ALIAS property, ignoring: %m"); + return r; - if (!path_is_absolute(word)) - log_device_warning(dev, "SYSTEMD_ALIAS is not an absolute path, ignoring: %s", word); - else if (!path_is_normalized(word)) - log_device_warning(dev, "SYSTEMD_ALIAS is not a normalized path, ignoring: %s", word); - else - (void) device_setup_unit(m, dev, word, /* main = */ false); + /* Add an additional unit for the device node */ + if (sd_device_get_devname(dev, &devname) >= 0) + (void) device_setup_unit(m, dev, devname, /* main = */ false, ready_units); + + } else { + Unit *u; + + /* If the device exists but not ready, then save the units and unset udev bits later. */ + + if (device_by_path(m, syspath, &u) >= 0) { + r = set_ensure_put(not_ready_units, NULL, DEVICE(u)); + if (r < 0) + log_unit_debug_errno(u, r, "Failed to store unit, ignoring: %m"); + } + + if (sd_device_get_devname(dev, &devname) >= 0 && + device_by_path(m, devname, &u) >= 0) { + r = set_ensure_put(not_ready_units, NULL, DEVICE(u)); + if (r < 0) + log_unit_debug_errno(u, r, "Failed to store unit, ignoring: %m"); + } } + + /* Next, add/update additional .device units point to aliases and symlinks. */ + (void) device_setup_extra_units(m, dev, ready_units, not_ready_units); + return 0; } static Unit *device_following(Unit *u) { @@ -999,28 +1021,16 @@ static void device_enumerate(Manager *m) { } FOREACH_DEVICE(e, dev) { - _cleanup_strv_free_ char **ready_devlinks = NULL; - const char *sysfs; - bool ready; + _cleanup_set_free_ Set *ready_units = NULL, *not_ready_units = NULL; + Device *d; - r = sd_device_get_syspath(dev, &sysfs); - if (r < 0) { - log_device_debug_errno(dev, r, "Couldn't get syspath from device, ignoring: %m"); + if (device_setup_units(m, dev, &ready_units, ¬_ready_units) < 0) continue; - } - ready = device_is_ready(dev); - if (ready) - device_process_new(m, dev, sysfs); - - /* Add additional units for all symlinks */ - (void) device_setup_devlink_units(m, dev, &ready_devlinks); - - if (ready) - device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); - - STRV_FOREACH(devlink, ready_devlinks) - device_update_found_by_name(m, *devlink, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); + SET_FOREACH(d, ready_units) + device_update_found_one(d, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); + SET_FOREACH(d, not_ready_units) + device_update_found_one(d, DEVICE_NOT_FOUND, DEVICE_FOUND_UDEV); } return; @@ -1043,30 +1053,6 @@ static void device_propagate_reload(Manager *m, Device *d) { log_unit_warning_errno(UNIT(d), r, "Failed to propagate reload, ignoring: %m"); } -static void device_propagate_reload_by_sysfs(Manager *m, const char *sysfs) { - Device *l; - - assert(m); - assert(sysfs); - - l = hashmap_get(m->devices_by_sysfs, sysfs); - LIST_FOREACH(same_sysfs, d, l) - device_propagate_reload(m, d); -} - -static void device_propagate_reload_by_name(Manager *m, const char *path) { - Unit *u; - int r; - - assert(m); - assert(path); - - if (device_by_path(m, path, &u) < 0) - return; - - device_propagate_reload(m, DEVICE(u)); -} - static void device_remove_old_on_move(Manager *m, sd_device *dev) { _cleanup_free_ char *syspath_old = NULL; const char *devpath_old; @@ -1087,11 +1073,12 @@ static void device_remove_old_on_move(Manager *m, sd_device *dev) { } static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata) { - _cleanup_strv_free_ char **ready_devlinks = NULL; + _cleanup_set_free_ Set *ready_units = NULL, *not_ready_units = NULL; Manager *m = ASSERT_PTR(userdata); sd_device_action_t action; const char *sysfs; bool ready; + Device *d; int r; assert(dev); @@ -1118,50 +1105,40 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * * change events */ ready = device_is_ready(dev); + (void) device_setup_units(m, dev, &ready_units, ¬_ready_units); + if (action == SD_DEVICE_REMOVE) { r = swap_process_device_remove(m, dev); if (r < 0) log_device_warning_errno(dev, r, "Failed to process swap device remove event, ignoring: %m"); - - (void) device_setup_devlink_units_on_remove(m, dev, &ready_devlinks); - - } else { - if (ready) { - device_process_new(m, dev, sysfs); - - r = swap_process_device_new(m, dev); - if (r < 0) - log_device_warning_errno(dev, r, "Failed to process swap device new event, ignoring: %m"); - } - - /* Add additional units for all symlinks */ - (void) device_setup_devlink_units(m, dev, &ready_devlinks); + } else if (ready) { + r = swap_process_device_new(m, dev); + if (r < 0) + log_device_warning_errno(dev, r, "Failed to process swap device new event, ignoring: %m"); } - if (!IN_SET(action, SD_DEVICE_ADD, SD_DEVICE_REMOVE, SD_DEVICE_MOVE)) { - device_propagate_reload_by_sysfs(m, sysfs); + if (!IN_SET(action, SD_DEVICE_ADD, SD_DEVICE_REMOVE, SD_DEVICE_MOVE)) + SET_FOREACH(d, ready_units) + device_propagate_reload(m, d); - STRV_FOREACH(devlink, ready_devlinks) - device_propagate_reload_by_name(m, *devlink); - } - - if (ready || !strv_isempty(ready_devlinks)) + if (!set_isempty(ready_units)) manager_dispatch_load_queue(m); if (action == SD_DEVICE_REMOVE) /* If we get notified that a device was removed by udev, then it's completely gone, hence - * unset all found bits */ + * unset all found bits. Note this affects all .device units still point to the removed + * device. */ device_update_found_by_sysfs(m, sysfs, DEVICE_NOT_FOUND, DEVICE_FOUND_MASK); - else if (ready) - /* The device is found now, set the udev found bit */ - device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); - else - /* The device is nominally around, but not ready for us. Hence unset the udev bit, but leave - * the rest around. */ - device_update_found_by_sysfs(m, sysfs, DEVICE_NOT_FOUND, DEVICE_FOUND_UDEV); - STRV_FOREACH(devlink, ready_devlinks) - device_update_found_by_name(m, *devlink, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); + /* These devices are found and ready now, set the udev found bit. Note, this is also necessary to do + * on remove uevent, as some devlinks may be updated and now point to other device nodes. */ + SET_FOREACH(d, ready_units) + device_update_found_one(d, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); + + /* These devices may be nominally around, but not ready for us. Hence unset the udev bit, but leave + * the rest around. This may be redundant for remove uevent, but should be harmless. */ + SET_FOREACH(d, not_ready_units) + device_update_found_one(d, DEVICE_NOT_FOUND, DEVICE_FOUND_UDEV); return 0; } @@ -1205,7 +1182,7 @@ void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFo return; } - (void) device_setup_unit(m, dev, node, /* main = */ false); /* 'dev' may be NULL. */ + (void) device_setup_unit(m, dev, node, /* main = */ false, NULL); /* 'dev' may be NULL. */ } /* Update the device unit's state, should it exist */ From c352110adc82bc837da275b94e6ba245fc7d5469 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Sep 2022 03:13:46 +0900 Subject: [PATCH 06/10] core/device: refuse alias with ".." --- src/core/device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index ebe1a934f4..21dc5890fc 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -807,8 +807,8 @@ static int device_setup_extra_units(Manager *m, sd_device *dev, Set **ready_unit continue; } - if (!path_is_normalized(*alias)) { - log_device_warning(dev, "The alias \"%s\" specified in SYSTEMD_ALIAS is not a normalized path, ignoring.", *alias); + if (!path_is_safe(*alias)) { + log_device_warning(dev, "The alias \"%s\" specified in SYSTEMD_ALIAS is not safe, ignoring.", *alias); continue; } From a4cb8afb8ff82ed55ca3667e1ccee9f3757a4a20 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Sep 2022 03:30:11 +0900 Subject: [PATCH 07/10] core/device: check that no unit is ready and not simultaneously This should not happen, just for safety. --- src/core/device.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/device.c b/src/core/device.c index 21dc5890fc..b3cf77d50c 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -903,6 +903,13 @@ static int device_setup_units(Manager *m, sd_device *dev, Set **ready_units, Set /* Next, add/update additional .device units point to aliases and symlinks. */ (void) device_setup_extra_units(m, dev, ready_units, not_ready_units); + + /* Safety check: no unit should be in ready_units and not_ready_units simultaneously. */ + Unit *u; + SET_FOREACH(u, *not_ready_units) + if (set_remove(*ready_units, u)) + log_unit_error(u, "Cannot active and deactive simultaneously. Deactivating."); + return 0; } From 9c126b2c92c220fb7e87591d10a4f87e1d2d6f12 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 03:48:02 +0900 Subject: [PATCH 08/10] test-64-udev-storage: check device units --- test/units/testsuite-64.sh | 116 +++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/test/units/testsuite-64.sh b/test/units/testsuite-64.sh index 9a2540ad0c..dccb531242 100755 --- a/test/units/testsuite-64.sh +++ b/test/units/testsuite-64.sh @@ -83,6 +83,93 @@ helper_check_udev_watch() {( done < <(find /run/udev/watch -type l) )} +check_device_unit() {( + set +x + + local log_level link links path syspath unit + + log_level="${1?}" + path="${2?}" + unit=$(systemd-escape --path --suffix=device "$path") + + syspath=$(systemctl show --value --property SysFSPath "$unit" 2>/dev/null) + if [[ -z "$syspath" ]]; then + [[ "$log_level" == 1 ]] && echo >&2 "ERROR: $unit not found." + return 1 + fi + + if [[ ! -L "$path" ]]; then + if [[ ! -d "$syspath" ]]; then + [[ "$log_level" == 1 ]] && echo >&2 "ERROR: $unit exists for $syspath but it does not exist." + return 1 + fi + return 0 + fi + + if [[ ! -b "$path" && ! -c "$path" ]]; then + [[ "$log_level" == 1 ]] && echo >&2 "ERROR: invalid file type $path" + return 1 + fi + + read -r -a links < <(udevadm info -q symlink "$syspath" 2>/dev/null) + for link in "${links[@]}"; do + if [[ "/dev/$link" == "$path" ]]; then # DEVLINKS= given by -q symlink are relative to /dev + return 0 + fi + done + + read -r -a links < <(udevadm info "$syspath" | sed -ne '/SYSTEMD_ALIAS=/ { s/^E: SYSTEMD_ALIAS=//; p }' 2>/dev/null) + for link in "${links[@]}"; do + if [[ "$link" == "$path" ]]; then # SYSTEMD_ALIAS= are absolute + return 0 + fi + done + + [[ "$log_level" == 1 ]] && echo >&2 "ERROR: $unit exists for $syspath but it does not have the corresponding DEVLINKS or SYSTEMD_ALIAS." + return 1 +)} + +check_device_units() {( + set +x + + local log_level path paths + + log_level="${1?}" + shift + paths=("$@") + + for path in "${paths[@]}"; do + if ! check_device_unit "$log_level" "$path"; then + return 1 + fi + done + + while read -r unit _; do + path=$(systemd-escape --path --unescape "$unit") + if ! check_device_unit "$log_level" "$path"; then + return 1 + fi + done < <(systemctl list-units --all --type=device --no-legend dev-* | awk '{ print $1 }' | sed -e 's/\.device$//') + + return 0 +)} + +helper_check_device_units() {( + set +x + + local i + + for ((i = 0; i < 20; i++)); do + (( i == 0 )) || sleep .5 + + if check_device_units 0 "$@"; then + return 0 + fi + done + + check_device_units 1 "$@" +)} + testcase_megasas2_basic() { lsblk -S [[ "$(lsblk --scsi --noheadings | wc -l)" -ge 128 ]] @@ -160,6 +247,7 @@ EOF "/dev/disk/by-uuid/deadbeef-dead-dead-beef-111111111111" ) udevadm wait --settle --timeout=30 "${part_links[@]}" + helper_check_device_units "${part_links[@]}" # Choose a random symlink to the failover data partition each time, for # a better coverage @@ -191,6 +279,7 @@ EOF # Make sure all symlinks are still valid udevadm wait --settle --timeout=30 "${part_links[@]}" + helper_check_device_units "${part_links[@]}" done multipath -l "$path" @@ -287,6 +376,7 @@ EOF fi done + helper_check_device_units rm -f "$rule" "$partscript" udevadm control --reload @@ -317,16 +407,19 @@ testcase_lvm_basic() { mkfs.ext4 -L mylvpart1 "/dev/$vgroup/mypart1" udevadm wait --settle --timeout="$timeout" "/dev/disk/by-label/mylvpart1" helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units # Disable the VG and check symlinks... lvm vgchange -an "$vgroup" udevadm wait --settle --timeout="$timeout" --removed "/dev/$vgroup" "/dev/disk/by-label/mylvpart1" helper_check_device_symlinks "/dev/disk" + helper_check_device_units # reenable the VG and check the symlinks again if all LVs are properly activated lvm vgchange -ay "$vgroup" udevadm wait --settle --timeout="$timeout" "/dev/$vgroup/mypart1" "/dev/$vgroup/mypart2" "/dev/disk/by-label/mylvpart1" helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units # Same as above, but now with more "stress" [[ -n "${ASAN_OPTIONS:-}" ]] && iterations=10 || iterations=50 @@ -337,6 +430,7 @@ testcase_lvm_basic() { if ((i % 5 == 0)); then udevadm wait --settle --timeout="$timeout" "/dev/$vgroup/mypart1" "/dev/$vgroup/mypart2" "/dev/disk/by-label/mylvpart1" helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units fi done @@ -345,6 +439,7 @@ testcase_lvm_basic() { udevadm wait --settle --timeout="$timeout" --removed "/dev/$vgroup/mypart1" udevadm wait --timeout=0 "/dev/$vgroup/mypart2" helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units # Create & remove LVs in a loop, i.e. with more "stress" [[ -n "${ASAN_OPTIONS:-}" ]] && iterations=8 || iterations=16 @@ -364,6 +459,7 @@ testcase_lvm_basic() { udevadm wait --settle --timeout="$timeout" --removed "/dev/$vgroup/looppart$part" done helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units fi done } @@ -383,6 +479,7 @@ testcase_btrfs_basic() { udevadm wait --settle --timeout=30 "${devices[0]}" "/dev/disk/by-uuid/$uuid" "/dev/disk/by-label/$label" btrfs filesystem show helper_check_device_symlinks + helper_check_device_units echo "Multiple devices: using partitions, data: single, metadata: raid1" uuid="deadbeef-dead-dead-beef-000000000001" @@ -400,6 +497,7 @@ EOF udevadm wait --settle --timeout=30 "/dev/disk/by-uuid/$uuid" "/dev/disk/by-label/$label" btrfs filesystem show helper_check_device_symlinks + helper_check_device_units wipefs -a -f "${devices[0]}" udevadm wait --settle --timeout=30 --removed /dev/disk/by-partlabel/diskpart{1..4} @@ -415,6 +513,7 @@ EOF udevadm wait --settle --timeout=30 "/dev/disk/by-uuid/$uuid" "/dev/disk/by-label/$label" btrfs filesystem show helper_check_device_symlinks + helper_check_device_units echo "Multiple devices: using LUKS encrypted disks, data: raid1, metadata: raid1, mixed mode" uuid="deadbeef-dead-dead-beef-000000000003" @@ -441,6 +540,7 @@ EOF systemctl start "systemd-cryptsetup@encbtrfs$i" done helper_check_device_symlinks + helper_check_device_units # Check if we have all necessary DM devices ls -l /dev/mapper/encbtrfs{0..3} # Create a multi-device btrfs filesystem on the LUKS devices @@ -453,6 +553,7 @@ EOF udevadm wait --settle --timeout=30 "/dev/disk/by-uuid/$uuid" "/dev/disk/by-label/$label" btrfs filesystem show helper_check_device_symlinks + helper_check_device_units # Mount it and write some data to it we can compare later mount -t btrfs /dev/mapper/encbtrfs0 "$mpoint" echo "hello there" >"$mpoint/test" @@ -461,6 +562,7 @@ EOF systemctl stop systemd-cryptsetup@encbtrfs{0..3} udevadm wait --settle --timeout=30 --removed "/dev/disk/by-uuid/$uuid" helper_check_device_symlinks + helper_check_device_units # Add the mount point to /etc/fstab and check if the device can be put together # automagically. The source device is the DM name of the first LUKS device # (from /etc/crypttab). We have to specify all LUKS devices manually, as @@ -477,6 +579,7 @@ EOF udevadm wait --settle --timeout=30 "/dev/disk/by-uuid/$uuid" "/dev/disk/by-label/$label" btrfs filesystem show helper_check_device_symlinks + helper_check_device_units grep "hello there" "$mpoint/test" # Cleanup systemctl stop "${mpoint##*/}.mount" @@ -525,6 +628,7 @@ testcase_iscsi_lvm() { iscsiadm --mode node --targetname "$target_name" --portal "$target_ip:$target_port" --login udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" helper_check_device_symlinks + helper_check_device_units # Cleanup iscsiadm --mode node --targetname "$target_name" --portal "$target_ip:$target_port" --logout tgtadm --lld iscsi --op delete --mode target --tid=1 @@ -560,6 +664,7 @@ testcase_iscsi_lvm() { iscsiadm --mode node --targetname "$target_name" --portal "$target_ip:$target_port" --login udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" helper_check_device_symlinks + helper_check_device_units # Add all iSCSI devices into a LVM volume group, create two logical volumes, # and check if necessary symlinks exist (and are valid) lvm pvcreate -y "${expected_symlinks[@]}" @@ -574,6 +679,7 @@ testcase_iscsi_lvm() { mkfs.ext4 -L mylvpart1 "/dev/$vgroup/mypart1" udevadm wait --settle --timeout=30 "/dev/disk/by-label/mylvpart1" helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units # Disconnect the iSCSI devices and check all the symlinks iscsiadm --mode node --targetname "$target_name" --portal "$target_ip:$target_port" --logout # "Reset" the DM state, since we yanked the backing storage from under the LVM, @@ -582,11 +688,13 @@ testcase_iscsi_lvm() { # The LVM and iSCSI related symlinks should be gone udevadm wait --settle --timeout=30 --removed "/dev/$vgroup" "/dev/disk/by-label/mylvpart1" "${expected_symlinks[@]}" helper_check_device_symlinks "/dev/disk" + helper_check_device_units # Reconnect the iSCSI devices and check if everything get detected correctly iscsiadm --mode discoverydb --type sendtargets --portal "$target_ip" --discover iscsiadm --mode node --targetname "$target_name" --portal "$target_ip:$target_port" --login udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" "/dev/$vgroup/mypart1" "/dev/$vgroup/mypart2" "/dev/disk/by-label/mylvpart1" helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units # Cleanup iscsiadm --mode node --targetname "$target_name" --portal "$target_ip:$target_port" --logout tgtadm --lld iscsi --op delete --mode target --tid=2 @@ -687,6 +795,7 @@ testcase_mdadm_basic() { udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" done helper_check_device_symlinks + helper_check_device_units # Cleanup mdadm -v --stop "$raid_dev" udevadm wait --settle --timeout=30 --removed "${expected_symlinks[@]}" @@ -715,9 +824,11 @@ testcase_mdadm_basic() { udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" done helper_check_device_symlinks + helper_check_device_units # Cleanup mdadm -v --stop "$raid_dev" udevadm wait --settle --timeout=30 --removed "${expected_symlinks[@]}" + helper_check_device_units echo "Mirror + parity raid (RAID 10) + multiple partitions" raid_name="mdmirpar" @@ -763,10 +874,12 @@ EOF udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" done helper_check_device_symlinks + helper_check_device_units # Cleanup mdadm -v --stop "$raid_dev" # Check if all expected symlinks were removed after the cleanup udevadm wait --settle --timeout=30 --removed "${expected_symlinks[@]}" + helper_check_device_units } testcase_mdadm_lvm() { @@ -811,15 +924,18 @@ testcase_mdadm_lvm() { mdadm -v --stop "$raid_dev" udevadm wait --settle --timeout=30 --removed "${expected_symlinks[@]}" helper_check_device_symlinks + helper_check_device_units # Reassemble it and check if all required symlinks exist mdadm --assemble "$raid_dev" --name "$raid_name" -v udevadm wait --settle --timeout=30 "${expected_symlinks[@]}" helper_check_device_symlinks + helper_check_device_units # Cleanup lvm vgchange -an "$vgroup" mdadm -v --stop "$raid_dev" # Check if all expected symlinks were removed after the cleanup udevadm wait --settle --timeout=30 --removed "${expected_symlinks[@]}" + helper_check_device_units } : >/failed From ede0fe02714d079ddf1223f3ea814a2805d1b89d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 04:12:25 +0900 Subject: [PATCH 09/10] test-64-storage: add test for renaming lvm volume --- test/units/testsuite-64.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/units/testsuite-64.sh b/test/units/testsuite-64.sh index dccb531242..1bf894fa32 100755 --- a/test/units/testsuite-64.sh +++ b/test/units/testsuite-64.sh @@ -409,6 +409,22 @@ testcase_lvm_basic() { helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" helper_check_device_units + # Rename partitions (see issue #24518) + lvm lvrename "/dev/$vgroup/mypart1" renamed1 + lvm lvrename "/dev/$vgroup/mypart2" renamed2 + udevadm wait --settle --timeout="$timeout" --removed "/dev/$vgroup/mypart1" "/dev/$vgroup/mypart2" + udevadm wait --settle --timeout="$timeout" "/dev/$vgroup/renamed1" "/dev/$vgroup/renamed2" + helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units + + # Rename them back + lvm lvrename "/dev/$vgroup/renamed1" mypart1 + lvm lvrename "/dev/$vgroup/renamed2" mypart2 + udevadm wait --settle --timeout="$timeout" --removed "/dev/$vgroup/renamed1" "/dev/$vgroup/renamed2" + udevadm wait --settle --timeout="$timeout" "/dev/$vgroup/mypart1" "/dev/$vgroup/mypart2" + helper_check_device_symlinks "/dev/disk" "/dev/$vgroup" + helper_check_device_units + # Disable the VG and check symlinks... lvm vgchange -an "$vgroup" udevadm wait --settle --timeout="$timeout" --removed "/dev/$vgroup" "/dev/disk/by-label/mylvpart1" From fb227a3de1e709d3887ae8b5df38835335a5127d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 1 Sep 2022 04:40:21 +0900 Subject: [PATCH 10/10] test-17-udev: test that device units for nonexistent devlink are removed For issue #24518. --- test/units/testsuite-17.08.sh | 72 +++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100755 test/units/testsuite-17.08.sh diff --git a/test/units/testsuite-17.08.sh b/test/units/testsuite-17.08.sh new file mode 100755 index 0000000000..f740b337f7 --- /dev/null +++ b/test/units/testsuite-17.08.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -ex +set -o pipefail + +# shellcheck source=test/units/assert.sh +. "$(dirname "$0")"/assert.sh + +# This is a test for issue #24518. + +mkdir -p /run/udev/rules.d/ +cat >/run/udev/rules.d/50-testsuite.rules <