diff --git a/src/core/mount.c b/src/core/mount.c index d22e7dec59..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); + /* 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) { @@ -1600,15 +1610,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); @@ -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); - r = mount_add_non_exec_dependencies(MOUNT(u)); if (r < 0) return r; @@ -1717,37 +1724,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) @@ -1984,7 +1968,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 +1999,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 +2060,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; 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-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.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. */ 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 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 <