From 6688b72d32f450d243a47ab960dc131ebe666e67 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 15 May 2022 05:40:34 +0900 Subject: [PATCH 1/6] core/mount: use set_put_strdup_full() --- src/core/mount.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index d22e7dec59..8aaa9e966b 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1984,7 +1984,7 @@ static int drain_libmount(Manager *m) { } static int mount_process_proc_self_mountinfo(Manager *m) { - _cleanup_set_free_free_ Set *around = NULL, *gone = NULL; + _cleanup_set_free_ Set *around = NULL, *gone = NULL; const char *what; int r; @@ -2015,13 +2015,10 @@ static int mount_process_proc_self_mountinfo(Manager *m) { * existed. */ if (mount->from_proc_self_mountinfo && - mount->parameters_proc_self_mountinfo.what) { - + mount->parameters_proc_self_mountinfo.what) /* Remember that this device might just have disappeared */ - if (set_ensure_allocated(&gone, &path_hash_ops) < 0 || - set_put_strdup(&gone, mount->parameters_proc_self_mountinfo.what) < 0) + if (set_put_strdup_full(&gone, &path_hash_ops_free, mount->parameters_proc_self_mountinfo.what) < 0) log_oom(); /* we don't care too much about OOM here... */ - } mount->from_proc_self_mountinfo = false; assert_se(update_parameters_proc_self_mountinfo(mount, NULL, NULL, NULL) >= 0); @@ -2079,13 +2076,10 @@ static int mount_process_proc_self_mountinfo(Manager *m) { if (mount_is_mounted(mount) && mount->from_proc_self_mountinfo && - mount->parameters_proc_self_mountinfo.what) { + mount->parameters_proc_self_mountinfo.what) /* Track devices currently used */ - - if (set_ensure_allocated(&around, &path_hash_ops) < 0 || - set_put_strdup(&around, mount->parameters_proc_self_mountinfo.what) < 0) + if (set_put_strdup_full(&around, &path_hash_ops_free, mount->parameters_proc_self_mountinfo.what) < 0) log_oom(); - } /* Reset the flags for later calls */ mount->proc_flags = 0; From 03e52e8cc3f4bca180afe7a4a2d2b2f585c70235 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 13 May 2022 11:40:55 +0900 Subject: [PATCH 2/6] core: unit_name_from_path() does not return -ENAMETOOLONG anymore Follow-ups for a7fb1f2eae3314c28d451822302283a7ab1bc1c0 and 1d0727e76fd5e9a07cc9991ec9a10ea1d78a99c7. --- src/core/mount.c | 31 ++++--------------------------- src/core/swap.c | 3 +-- src/core/unit.c | 2 +- 3 files changed, 6 insertions(+), 30 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 8aaa9e966b..4263610e82 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1717,37 +1717,14 @@ static int mount_setup_unit( if (!is_path(where)) return 0; - /* Mount unit names have to be (like all other unit names) short enough to fit into file names. This - * means there's a good chance that overly long mount point paths after mangling them to look like a - * unit name would result in unit names we don't actually consider valid. This should be OK however - * as such long mount point paths should not happen on regular systems — and if they appear - * nonetheless they are generally synthesized by software, and thus managed by that other - * software. Having such long names just means you cannot use systemd to manage those specific mount - * points, which should be an OK restriction to make. After all we don't have to be able to manage - * all mount points in the world — as long as we don't choke on them when we encounter them. */ r = unit_name_from_path(where, ".mount", &e); - if (r < 0) { - static RateLimit rate_limit = { /* Let's log about this at warning level at most once every - * 5s. Given that we generate this whenever we read the file - * otherwise we probably shouldn't flood the logs with - * this */ - .interval = 5 * USEC_PER_SEC, - .burst = 1, - }; - - if (r == -ENAMETOOLONG) - return log_struct_errno( - ratelimit_below(&rate_limit) ? LOG_WARNING : LOG_DEBUG, r, - "MESSAGE_ID=" SD_MESSAGE_MOUNT_POINT_PATH_NOT_SUITABLE_STR, - "MOUNT_POINT=%s", where, - LOG_MESSAGE("Mount point path '%s' too long to fit into unit name, ignoring mount point.", where)); - + if (r < 0) return log_struct_errno( - ratelimit_below(&rate_limit) ? LOG_WARNING : LOG_DEBUG, r, + LOG_WARNING, r, "MESSAGE_ID=" SD_MESSAGE_MOUNT_POINT_PATH_NOT_SUITABLE_STR, "MOUNT_POINT=%s", where, - LOG_MESSAGE("Failed to generate valid unit name from mount point path '%s', ignoring mount point: %m", where)); - } + LOG_MESSAGE("Failed to generate valid unit name from mount point path '%s', ignoring mount point: %m", + where)); u = manager_get_unit(m, e); if (u) diff --git a/src/core/swap.c b/src/core/swap.c index 222c76cf79..2d1190d018 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1437,8 +1437,7 @@ int swap_process_device_new(Manager *m, sd_device *dev) { int q; q = unit_name_from_path(devlink, ".swap", &n); - if (IN_SET(q, -EINVAL, -ENAMETOOLONG)) /* If name too long or otherwise not convertible to - * unit name, we can't manage it */ + if (q == -EINVAL) /* If the name is not convertible to unit name, we can't manage it */ continue; if (q < 0) return q; diff --git a/src/core/unit.c b/src/core/unit.c index a082741bf8..07c83c90a4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1519,7 +1519,7 @@ static int unit_add_mount_dependencies(Unit *u) { Unit *m; r = unit_name_from_path(prefix, ".mount", &p); - if (IN_SET(r, -EINVAL, -ENAMETOOLONG)) + if (r == -EINVAL) continue; /* If the path cannot be converted to a mount unit name, then it's * not manageable as a unit by systemd, and hence we don't need a * dependency on it. Let's thus silently ignore the issue. */ From b9e6d23dfc93b0eab8762c6fbe4fddc9eeb0c04f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 13 May 2022 12:30:04 +0900 Subject: [PATCH 3/6] core/mount: set Mount.from_proc_self_mountinfo flag before adding default dependencies When the function mount_setup_new_unit() is called, the corresponding .mount unit file is not loaded. Hence, Mount.from_fragment is false. To add default dependencies to e.g. .device units, it is necessary that the Mount.from_proc_self_mountinfo flag is set. However, the flag was not set even the unit has information from /proc/self/mountinfo. Partially fixes #19983. --- src/core/mount.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 4263610e82..229b3de4fd 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1600,15 +1600,15 @@ static int mount_setup_new_unit( if (r < 0) return r; - r = mount_add_non_exec_dependencies(MOUNT(u)); - if (r < 0) - return r; - /* This unit was generated because /proc/self/mountinfo reported it. Remember this, so that by the * time we load the unit file for it (and thus add in extra deps right after) we know what source to * attributes the deps to. */ MOUNT(u)->from_proc_self_mountinfo = true; + r = mount_add_non_exec_dependencies(MOUNT(u)); + if (r < 0) + return r; + /* We have only allocated the stub now, let's enqueue this unit for loading now, so that everything * else is loaded in now. */ unit_add_to_load_queue(u); From 0fe06168b319064a84fca05109ac3eaaeea499c2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 15 May 2022 05:49:01 +0900 Subject: [PATCH 4/6] core/mount: also remove default deps from /proc/self/mountinfo when it is updated The dependencies tagged with UNIT_DEPENDENCY_MOUNTINFO_DEFAULT depend on both /proc/self/mountinfo and corresponding .mount unit file. Hence, if some information from mountinfo is updated, e.g. device mounted on the path, we need to update the dependencies. --- src/core/mount.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 229b3de4fd..71da69cbb5 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -909,7 +909,7 @@ static void mount_enter_dead(Mount *m, MountResult f) { dynamic_creds_destroy(&m->dynamic_creds); /* Any dependencies based on /proc/self/mountinfo are now stale */ - unit_remove_dependencies(UNIT(m), UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT); + unit_remove_dependencies(UNIT(m), UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT | UNIT_DEPENDENCY_MOUNTINFO_DEFAULT); } static void mount_enter_mounted(Mount *m, MountResult f) { @@ -1675,7 +1675,7 @@ static int mount_setup_existing_unit( /* If things changed, then make sure that all deps are regenerated. Let's * first remove all automatic deps, and then add in the new ones. */ - unit_remove_dependencies(u, UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT); + unit_remove_dependencies(u, UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT | UNIT_DEPENDENCY_MOUNTINFO_DEFAULT); r = mount_add_non_exec_dependencies(MOUNT(u)); if (r < 0) From 49267b1b147047c294e882b7ae741f8732d36d05 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 15 May 2022 06:31:17 +0900 Subject: [PATCH 5/6] core/mount: make device deps from /proc/self/mountinfo and .mount unit file exclusive Before this commit, when a .mount unit file is loaded, the device dependencies from the unit file are automatically added. If a device that is different from which specified in .mount unit file is mounted on the path, then the device dependencies about the device are also added. Hence, the unit has device dependencies about two (or more) different devices. Hence, if one of the devices are unplugged, even if another one is mounted on the path, then the path is unmounted. This commit makes device dependencies from two different sources exclusive, and always regenerate the dependencies when one of the information is updated. Fixes #19983 and #23552. --- src/core/mount.c | 57 ++++++++++++++++++++++----------------- src/core/unit-serialize.c | 1 + src/core/unit.h | 10 ++++--- 3 files changed, 40 insertions(+), 28 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 71da69cbb5..9dc04cd07e 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -347,7 +347,6 @@ static int mount_add_mount_dependencies(Mount *m) { } static int mount_add_device_dependencies(Mount *m) { - UnitDependencyMask mask; MountParameters *p; UnitDependency dep; int r; @@ -396,17 +395,14 @@ static int mount_add_device_dependencies(Mount *m) { * suddenly. */ dep = mount_is_bound_to_device(m) ? UNIT_BINDS_TO : UNIT_REQUIRES; - /* We always use 'what' from /proc/self/mountinfo if mounted */ - mask = m->from_proc_self_mountinfo ? UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT : UNIT_DEPENDENCY_FILE; - - r = unit_add_node_dependency(UNIT(m), p->what, dep, mask); + r = unit_add_node_dependency(UNIT(m), p->what, dep, UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; if (r > 0) log_unit_trace(UNIT(m), "Added %s dependency on %s", unit_dependency_to_string(dep), p->what); if (mount_propagate_stop(m)) { - r = unit_add_node_dependency(UNIT(m), p->what, UNIT_STOP_PROPAGATED_FROM, mask); + r = unit_add_node_dependency(UNIT(m), p->what, UNIT_STOP_PROPAGATED_FROM, UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; if (r > 0) @@ -414,11 +410,11 @@ static int mount_add_device_dependencies(Mount *m) { unit_dependency_to_string(UNIT_STOP_PROPAGATED_FROM), p->what); } - r = unit_add_blockdev_dependency(UNIT(m), p->what, mask); + r = unit_add_blockdev_dependency(UNIT(m), p->what, UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r > 0) log_unit_trace(UNIT(m), "Added %s dependency on %s", unit_dependency_to_string(UNIT_AFTER), p->what); - return r; + return 0; } static int mount_add_quota_dependencies(Mount *m) { @@ -474,8 +470,7 @@ static bool mount_is_extrinsic(Unit *u) { static int mount_add_default_ordering_dependencies( Mount *m, - MountParameters *p, - UnitDependencyMask mask) { + MountParameters *p) { const char *after, *before, *e; int r; @@ -501,23 +496,25 @@ static int mount_add_default_ordering_dependencies( } if (!mount_is_nofail(m)) { - r = unit_add_dependency_by_name(UNIT(m), UNIT_BEFORE, before, true, mask); + r = unit_add_dependency_by_name(UNIT(m), UNIT_BEFORE, before, true, + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; } if (after) { - r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, after, true, mask); + r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, after, true, + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; } return unit_add_two_dependencies_by_name(UNIT(m), UNIT_BEFORE, UNIT_CONFLICTS, - SPECIAL_UMOUNT_TARGET, true, mask); + SPECIAL_UMOUNT_TARGET, true, + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); } static int mount_add_default_dependencies(Mount *m) { - UnitDependencyMask mask; MountParameters *p; int r; @@ -537,9 +534,7 @@ static int mount_add_default_dependencies(Mount *m) { if (!p) return 0; - mask = m->from_fragment ? UNIT_DEPENDENCY_FILE : UNIT_DEPENDENCY_MOUNTINFO_DEFAULT; - - r = mount_add_default_ordering_dependencies(m, p, mask); + r = mount_add_default_ordering_dependencies(m, p); if (r < 0) return r; @@ -549,7 +544,8 @@ static int mount_add_default_dependencies(Mount *m) { * network.target, so that they are shut down only after this mount unit is * stopped. */ - r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_NETWORK_TARGET, true, mask); + r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_NETWORK_TARGET, true, + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; @@ -558,14 +554,17 @@ static int mount_add_default_dependencies(Mount *m) { * mounting network file systems, and whose purpose it is to delay this until the * network is "up". */ - r = unit_add_two_dependencies_by_name(UNIT(m), UNIT_WANTS, UNIT_AFTER, SPECIAL_NETWORK_ONLINE_TARGET, true, mask); + r = unit_add_two_dependencies_by_name(UNIT(m), UNIT_WANTS, UNIT_AFTER, + SPECIAL_NETWORK_ONLINE_TARGET, true, + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; } /* If this is a tmpfs mount then we have to unmount it before we try to deactivate swaps */ if (streq_ptr(p->fstype, "tmpfs")) { - r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_SWAP_TARGET, true, mask); + r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_SWAP_TARGET, true, + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); if (r < 0) return r; } @@ -607,8 +606,18 @@ static int mount_verify(Mount *m) { static int mount_add_non_exec_dependencies(Mount *m) { int r; + assert(m); + /* Any dependencies based on /proc/self/mountinfo may be now stale. */ + unit_remove_dependencies(UNIT(m), + UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT | + UNIT_DEPENDENCY_MOUNTINFO_DEFAULT | + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE); + + if (!m->where) + return 0; + /* Adds in all dependencies directly responsible for ordering the mount, as opposed to dependencies * resulting from the ExecContext and such. */ @@ -908,8 +917,9 @@ static void mount_enter_dead(Mount *m, MountResult f) { dynamic_creds_destroy(&m->dynamic_creds); - /* Any dependencies based on /proc/self/mountinfo are now stale */ - unit_remove_dependencies(UNIT(m), UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT | UNIT_DEPENDENCY_MOUNTINFO_DEFAULT); + /* Any dependencies based on /proc/self/mountinfo are now stale. Let's re-generate dependencies from + * .mount unit. */ + (void) mount_add_non_exec_dependencies(m); } static void mount_enter_mounted(Mount *m, MountResult f) { @@ -1674,9 +1684,6 @@ static int mount_setup_existing_unit( if (FLAGS_SET(flags, MOUNT_PROC_JUST_CHANGED)) { /* If things changed, then make sure that all deps are regenerated. Let's * first remove all automatic deps, and then add in the new ones. */ - - unit_remove_dependencies(u, UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT | UNIT_DEPENDENCY_MOUNTINFO_DEFAULT); - r = mount_add_non_exec_dependencies(MOUNT(u)); if (r < 0) return r; diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index 3897bdee7c..797bf6c5ce 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -592,6 +592,7 @@ static void print_unit_dependency_mask(FILE *f, const char *kind, UnitDependency { UNIT_DEPENDENCY_PATH, "path" }, { UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT, "mountinfo-implicit" }, { UNIT_DEPENDENCY_MOUNTINFO_DEFAULT, "mountinfo-default" }, + { UNIT_DEPENDENCY_MOUNTINFO_OR_FILE, "mountinfo-or-file" }, { UNIT_DEPENDENCY_PROC_SWAP, "proc-swap" }, { UNIT_DEPENDENCY_SLICE_PROPERTY, "slice-property" }, }; diff --git a/src/core/unit.h b/src/core/unit.h index 33884fc991..4dabbb9a3c 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -86,13 +86,17 @@ typedef enum UnitDependencyMask { * DefaultDependencies= and thus also involving configuration from UNIT_DEPENDENCY_FILE sources */ UNIT_DEPENDENCY_MOUNTINFO_DEFAULT = 1 << 6, + /* A dependency created because of data read from /proc/self/mountinfo, but fallback to unit configuration + * sources */ + UNIT_DEPENDENCY_MOUNTINFO_OR_FILE = 1 << 7, + /* A dependency created because of data read from /proc/swaps and no other configuration source */ - UNIT_DEPENDENCY_PROC_SWAP = 1 << 7, + UNIT_DEPENDENCY_PROC_SWAP = 1 << 8, /* A dependency for units in slices assigned by directly setting Slice= */ - UNIT_DEPENDENCY_SLICE_PROPERTY = 1 << 8, + UNIT_DEPENDENCY_SLICE_PROPERTY = 1 << 9, - _UNIT_DEPENDENCY_MASK_FULL = (1 << 9) - 1, + _UNIT_DEPENDENCY_MASK_FULL = (1 << 10) - 1, } UnitDependencyMask; /* The Unit's dependencies[] hashmaps use this structure as value. It has the same size as a void pointer, and thus can From 413f20b4ac328c239952472102771b430fdc1a2a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 20 Jul 2022 16:56:25 +0900 Subject: [PATCH 6/6] test: add test case for mount unit dependencies --- test/units/testsuite-60.sh | 152 +++++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/test/units/testsuite-60.sh b/test/units/testsuite-60.sh index 3c391c8efa..c221b19c36 100755 --- a/test/units/testsuite-60.sh +++ b/test/units/testsuite-60.sh @@ -3,6 +3,155 @@ set -eux set -o pipefail +# shellcheck source=test/units/assert.sh +. "$(dirname "$0")"/assert.sh + +teardown_test_dependencies() ( + set +eux + + if mountpoint /tmp/deptest; then + umount /tmp/deptest + fi + + if [[ -n "${LOOP}" ]]; then + losetup -d "${LOOP}" || : + fi + if [[ -n "${LOOP_0}" ]]; then + losetup -d "${LOOP_0}" || : + fi + if [[ -n "${LOOP_1}" ]]; then + losetup -d "${LOOP_1}" || : + fi + + rm -f /tmp/testsuite-60-dependencies-0.img + rm -f /tmp/testsuite-60-dependencies-1.img + + rm -f /run/systemd/system/tmp-deptest.mount + systemctl daemon-reload + + return 0 +) + +setup_loop() { + truncate -s 30m "/tmp/testsuite-60-dependencies-${1?}.img" + sfdisk --wipe=always "/tmp/testsuite-60-dependencies-${1?}.img" </run/systemd/system/tmp-deptest.mount <