From 54a4d71509c0f3401aa576346754a0781795214a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 25 Oct 2022 21:40:21 +0900 Subject: [PATCH 1/3] core/device: update comment --- src/core/device.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 224fc90835..c79fd08bbb 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -267,7 +267,7 @@ static int device_coldplug(Unit *u) { * 1. MANAGER_IS_RUNNING() == false * 2. enumerate devices: manager_enumerate() -> device_enumerate() * Device.enumerated_found is set. - * 3. deserialize devices: manager_deserialize() -> device_deserialize() + * 3. deserialize devices: manager_deserialize() -> device_deserialize_item() * Device.deserialize_state and Device.deserialized_found are set. * 4. coldplug devices: manager_coldplug() -> device_coldplug() * deserialized properties are copied to the main properties. @@ -282,23 +282,28 @@ static int device_coldplug(Unit *u) { * * - On switch-root, the udev database may be cleared, except for devices with sticky bit, i.e. * OPTIONS="db_persist". Hence, almost no devices are enumerated in the step 2. However, in - * general, we have several serialized devices. So, DEVICE_FOUND_UDEV bit in the deserialized_found - * must be ignored, as udev rules in initrd and the main system are often different. If the - * deserialized state is DEVICE_PLUGGED, we need to downgrade it to DEVICE_TENTATIVE. Unlike the - * other starting mode, MANAGER_IS_SWITCHING_ROOT() is true when device_coldplug() and - * device_catchup() are called. Hence, let's conditionalize the operations by using the - * flag. After switch-root, systemd-udevd will (re-)process all devices, and the Device.found and - * Device.state will be adjusted. + * general, we have several serialized devices. So, DEVICE_FOUND_UDEV bit in the + * Device.deserialized_found must be ignored, as udev rules in initrd and the main system are often + * different. If the deserialized state is DEVICE_PLUGGED, we need to downgrade it to + * DEVICE_TENTATIVE. Unlike the other starting mode, MANAGER_IS_SWITCHING_ROOT() is true when + * device_coldplug() and device_catchup() are called. Hence, let's conditionalize the operations by + * using the flag. After switch-root, systemd-udevd will (re-)process all devices, and the + * Device.found and Device.state will be adjusted. * - * - On reload or reexecute, we can trust enumerated_found, deserialized_found, and deserialized_state. - * Of course, deserialized parameters may be outdated, but the unit state can be adjusted later by - * device_catchup() or uevents. */ + * - On reload or reexecute, we can trust Device.enumerated_found, Device.deserialized_found, and + * Device.deserialized_state. Of course, deserialized parameters may be outdated, but the unit + * state can be adjusted later by device_catchup() or uevents. */ if (MANAGER_IS_SWITCHING_ROOT(m) && !FLAGS_SET(d->enumerated_found, DEVICE_FOUND_UDEV)) { - found &= ~DEVICE_FOUND_UDEV; /* ignore DEVICE_FOUND_UDEV bit */ + + /* The device has not been enumerated. On switching-root, such situation is natural. See the + * above comment. To prevent problematic state transition active → dead → active, let's + * drop the DEVICE_FOUND_UDEV flag and downgrade state to DEVICE_TENTATIVE(activating). See + * issue #12953 and #23208. */ + found &= ~DEVICE_FOUND_UDEV; if (state == DEVICE_PLUGGED) - state = DEVICE_TENTATIVE; /* downgrade state */ + state = DEVICE_TENTATIVE; } if (d->found == found && d->state == state) From 1ea74fca3a3c737f3901bc10d879b7830b3528bf Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 25 Oct 2022 21:41:17 +0900 Subject: [PATCH 2/3] core/device: also serialize/deserialize device syspath The field will be used in later commits. --- src/core/device.c | 13 ++++++++++++- src/core/device.h | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index c79fd08bbb..7e354b2b4a 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -135,6 +135,7 @@ static void device_done(Unit *u) { assert(d); device_unset_sysfs(d); + d->deserialized_sysfs = mfree(d->deserialized_sysfs); d->wants_property = strv_free(d->wants_property); d->path = mfree(d->path); } @@ -392,6 +393,9 @@ static int device_serialize(Unit *u, FILE *f, FDSet *fds) { assert(f); assert(fds); + if (d->sysfs) + (void) serialize_item(f, "sysfs", d->sysfs); + if (d->path) (void) serialize_item(f, "path", d->path); @@ -413,7 +417,14 @@ static int device_deserialize_item(Unit *u, const char *key, const char *value, assert(value); assert(fds); - if (streq(key, "path")) { + if (streq(key, "sysfs")) { + if (!d->deserialized_sysfs) { + d->deserialized_sysfs = strdup(value); + if (!d->deserialized_sysfs) + log_oom_debug(); + } + + } else if (streq(key, "path")) { if (!d->path) { d->path = strdup(value); if (!d->path) diff --git a/src/core/device.h b/src/core/device.h index 7584bc70c4..9dd6fb57c2 100644 --- a/src/core/device.h +++ b/src/core/device.h @@ -20,7 +20,7 @@ typedef enum DeviceFound { struct Device { Unit meta; - char *sysfs; + char *sysfs, *deserialized_sysfs; char *path; /* syspath, device node, alias, or devlink */ /* In order to be able to distinguish dependencies on different device nodes we might end up creating multiple From b6c86ae28149c4abb2f0bd6acab13153382da9e7 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Oct 2022 01:18:05 +0900 Subject: [PATCH 3/3] core/device: verify device syspath on switching root Otherwise, if a device is removed while switching root, then the corresponding .device unit will never go to inactive state. This replaces the code dropped by cf1ac0cfe44997747b0f857a1d0b67cea1298272. Fixes #25106. --- src/core/device.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/core/device.c b/src/core/device.c index 7e354b2b4a..6e07f2745b 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -305,6 +305,19 @@ static int device_coldplug(Unit *u) { found &= ~DEVICE_FOUND_UDEV; if (state == DEVICE_PLUGGED) state = DEVICE_TENTATIVE; + + /* Also check the validity of the device syspath. Without this check, if the device was + * removed while switching root, it would never go to inactive state, as both Device.found + * and Device.enumerated_found do not have the DEVICE_FOUND_UDEV flag, so device_catchup() in + * device_update_found_one() does nothing in most cases. See issue #25106. Note that the + * syspath field is only serialized when systemd is sufficiently new and the device has been + * already processed by udevd. */ + if (d->deserialized_sysfs) { + _cleanup_(sd_device_unrefp) sd_device *dev = NULL; + + if (sd_device_new_from_syspath(&dev, d->deserialized_sysfs) < 0) + state = DEVICE_DEAD; + } } if (d->found == found && d->state == state)