From 56a061f508ec0c0331f8f258c5b4055ff0cc4968 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 19 Dec 2019 16:43:39 +0100 Subject: [PATCH 01/11] update TODO --- TODO | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TODO b/TODO index ce37869ce1..ed5e048d9a 100644 --- a/TODO +++ b/TODO @@ -24,6 +24,9 @@ Features: * honour specifiers in unit files that resolve to some very basic /etc/os-release data, such as ID, VERSION_ID, BUILD_ID, VARIANT_ID. +* cryptsetup: reimplement the mkswap/mke2fs in cryptsetup-generator to use + systemd-makefs.service instead. + * socket units: allow creating a udev monitor socket with ListenDevices= or so, with matches, then actviate app thorugh that passing socket oveer From 33a4c983428239894f282a77212f088758ee136e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 19 Dec 2019 13:14:03 +0100 Subject: [PATCH 02/11] fstab-generator: line break a bit more systematically --- src/fstab-generator/fstab-generator.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index e0f1076326..a75a74bb3c 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -174,8 +174,13 @@ static bool mount_in_initrd(struct mntent *me) { streq(me->mnt_dir, "/usr"); } -static int write_timeout(FILE *f, const char *where, const char *opts, - const char *filter, const char *variable) { +static int write_timeout( + FILE *f, + const char *where, + const char *opts, + const char *filter, + const char *variable) { + _cleanup_free_ char *timeout = NULL; char timespan[FORMAT_TIMESPAN_MAX]; usec_t u; @@ -208,8 +213,12 @@ static int write_mount_timeout(FILE *f, const char *where, const char *opts) { "x-systemd.mount-timeout\0", "TimeoutSec"); } -static int write_dependency(FILE *f, const char *opts, - const char *filter, const char *format) { +static int write_dependency( + FILE *f, + const char *opts, + const char *filter, + const char *format) { + _cleanup_strv_free_ char **names = NULL, **units = NULL; _cleanup_free_ char *res = NULL; char **s; @@ -230,6 +239,7 @@ static int write_dependency(FILE *f, const char *opts, r = unit_name_mangle_with_suffix(*s, "as dependency", 0, ".mount", &x); if (r < 0) return log_error_errno(r, "Failed to generate unit name: %m"); + r = strv_consume(&units, x); if (r < 0) return log_oom(); @@ -249,7 +259,8 @@ static int write_dependency(FILE *f, const char *opts, } static int write_after(FILE *f, const char *opts) { - return write_dependency(f, opts, "x-systemd.after", "After=%1$s\n"); + return write_dependency(f, opts, + "x-systemd.after", "After=%1$s\n"); } static int write_requires_after(FILE *f, const char *opts) { From 49685fb31480cd1f1428d4ac8ccfa9950f1bb77d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 19 Dec 2019 18:22:26 +0100 Subject: [PATCH 03/11] cryptsetup-generator: break overly long line --- src/cryptsetup/cryptsetup-generator.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c index b87abe9ece..eaa3af6298 100644 --- a/src/cryptsetup/cryptsetup-generator.c +++ b/src/cryptsetup/cryptsetup-generator.c @@ -99,7 +99,14 @@ static int split_keyspec(const char *keyspec, char **ret_keyfile, char **ret_key return 0; } -static int generate_keydev_mount(const char *name, const char *keydev, const char *keydev_timeout, bool canfail, char **unit, char **mount) { +static int generate_keydev_mount( + const char *name, + const char *keydev, + const char *keydev_timeout, + bool canfail, + char **unit, + char **mount) { + _cleanup_free_ char *u = NULL, *where = NULL, *name_escaped = NULL, *device_unit = NULL; _cleanup_fclose_ FILE *f = NULL; int r; From 6bbd539e5e7226ff1e2574e42c8bc821b68cfa19 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 19 Dec 2019 13:17:31 +0100 Subject: [PATCH 04/11] cryptsetup-generator: order after cryptsetup-pre.target unconditionally --- src/cryptsetup/cryptsetup-generator.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c index eaa3af6298..02b2cae5c0 100644 --- a/src/cryptsetup/cryptsetup-generator.c +++ b/src/cryptsetup/cryptsetup-generator.c @@ -299,9 +299,11 @@ static int create_disk( "SourcePath=%s\n" "DefaultDependencies=no\n" "IgnoreOnIsolate=true\n" - "After=%s\n", - arg_crypttab, - netdev ? "remote-fs-pre.target" : "cryptsetup-pre.target"); + "After=cryptsetup-pre.target\n", + arg_crypttab); + + if (netdev) + fprintf(f, "After=remote-fs-pre.target\n"); /* If initrd takes care of attaching the disk then it should also detach it during shutdown. */ if (!attach_in_initrd) From a7e885587949b6793ccf389505f3c436315fa653 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 19 Dec 2019 17:38:55 +0100 Subject: [PATCH 05/11] units: introduce blockdev@.target for properly ordering mounts/swaps against cryptsetup Let's hook it into both cryptsetup-generator and gpt-auto-generator with a shared implementation in generator.c Fixes: #8472 --- src/cryptsetup/cryptsetup-generator.c | 58 +++--------- src/fstab-generator/fstab-generator.c | 26 ++++-- src/gpt-auto-generator/gpt-auto-generator.c | 63 ++++++------- src/shared/generator.c | 97 +++++++++++++++++++++ src/shared/generator.h | 15 ++++ units/blockdev@.target | 13 +++ units/meson.build | 1 + 7 files changed, 188 insertions(+), 85 deletions(-) create mode 100644 units/blockdev@.target diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c index 02b2cae5c0..1deab765fb 100644 --- a/src/cryptsetup/cryptsetup-generator.c +++ b/src/cryptsetup/cryptsetup-generator.c @@ -230,8 +230,8 @@ static int create_disk( const char *options) { _cleanup_free_ char *n = NULL, *d = NULL, *u = NULL, *e = NULL, - *keydev_mount = NULL, *keyfile_timeout_value = NULL, *password_escaped = NULL, - *filtered = NULL, *u_escaped = NULL, *filtered_escaped = NULL, *name_escaped = NULL, *header_path = NULL; + *keydev_mount = NULL, *keyfile_timeout_value = NULL, + *filtered = NULL, *u_escaped = NULL, *name_escaped = NULL, *header_path = NULL, *password_buffer = NULL; _cleanup_fclose_ FILE *f = NULL; const char *dmname; bool noauto, nofail, tmp, swap, netdev, attach_in_initrd; @@ -292,15 +292,9 @@ static int create_disk( if (r < 0) return r; - fprintf(f, - "[Unit]\n" - "Description=Cryptography Setup for %%I\n" - "Documentation=man:crypttab(5) man:systemd-cryptsetup-generator(8) man:systemd-cryptsetup@.service(8)\n" - "SourcePath=%s\n" - "DefaultDependencies=no\n" - "IgnoreOnIsolate=true\n" - "After=cryptsetup-pre.target\n", - arg_crypttab); + r = generator_write_cryptsetup_unit_section(f, arg_crypttab); + if (r < 0) + return r; if (netdev) fprintf(f, "After=remote-fs-pre.target\n"); @@ -309,24 +303,18 @@ static int create_disk( if (!attach_in_initrd) fprintf(f, "Conflicts=umount.target\n"); - if (password) { - password_escaped = specifier_escape(password); - if (!password_escaped) - return log_oom(); - } - if (keydev) { - _cleanup_free_ char *unit = NULL, *p = NULL; + _cleanup_free_ char *unit = NULL; r = generate_keydev_mount(name, keydev, keyfile_timeout_value, keyfile_can_timeout > 0, &unit, &keydev_mount); if (r < 0) return log_error_errno(r, "Failed to generate keydev mount unit: %m"); - p = path_join(keydev_mount, password_escaped); - if (!p) + password_buffer = path_join(keydev_mount, password); + if (!password_buffer) return log_oom(); - free_and_replace(password_escaped, p); + password = password_buffer; fprintf(f, "After=%s\n", unit); if (keyfile_can_timeout > 0) @@ -353,17 +341,13 @@ static int create_disk( return r; } - if (path_startswith(u, "/dev/")) { + if (path_startswith(u, "/dev/")) fprintf(f, "BindsTo=%s\n" "After=%s\n" "Before=umount.target\n", d, d); - - if (swap) - fputs("Before=dev-mapper-%i.swap\n", - f); - } else + else /* For loopback devices, add systemd-tmpfiles-setup-dev.service dependency to ensure that loopback support is available in the kernel (/dev/loop-control needs to exist) */ @@ -377,23 +361,9 @@ static int create_disk( if (r < 0) log_warning_errno(r, "Failed to write device timeout drop-in: %m"); - if (filtered) { - filtered_escaped = specifier_escape(filtered); - if (!filtered_escaped) - return log_oom(); - } - - fprintf(f, - "\n[Service]\n" - "Type=oneshot\n" - "RemainAfterExit=yes\n" - "TimeoutSec=0\n" /* the binary handles timeouts anyway */ - "KeyringMode=shared\n" /* make sure we can share cached keys among instances */ - "OOMScoreAdjust=500\n" /* unlocking can allocate a lot of memory if Argon2 is used */ - "ExecStart=" SYSTEMD_CRYPTSETUP_PATH " attach '%s' '%s' '%s' '%s'\n" - "ExecStop=" SYSTEMD_CRYPTSETUP_PATH " detach '%s'\n", - name_escaped, u_escaped, strempty(password_escaped), strempty(filtered_escaped), - name_escaped); + r = generator_write_cryptsetup_service_section(f, name, u, password, filtered); + if (r < 0) + return r; if (tmp) fprintf(f, diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index a75a74bb3c..5a0a871759 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -118,11 +118,18 @@ static int add_swap( fprintf(f, "[Unit]\n" - "SourcePath=%s\n" - "Documentation=man:fstab(5) man:systemd-fstab-generator(8)\n\n" - "[Swap]\n", + "Documentation=man:fstab(5) man:systemd-fstab-generator(8)\n" + "SourcePath=%s\n", fstab_path()); + r = generator_write_blockdev_dependency(f, what); + if (r < 0) + return r; + + fprintf(f, + "\n" + "[Swap]\n"); + r = write_what(f, what); if (r < 0) return r; @@ -374,8 +381,8 @@ static int add_mount( fprintf(f, "[Unit]\n" - "SourcePath=%s\n" - "Documentation=man:fstab(5) man:systemd-fstab-generator(8)\n", + "Documentation=man:fstab(5) man:systemd-fstab-generator(8)\n" + "SourcePath=%s\n", source); /* All mounts under /sysroot need to happen later, at initrd-fs.target time. IOW, it's not @@ -422,7 +429,14 @@ static int add_mount( return r; } - fprintf(f, "\n[Mount]\n"); + r = generator_write_blockdev_dependency(f, what); + if (r < 0) + return r; + + fprintf(f, + "\n" + "[Mount]\n"); + if (original_where) fprintf(f, "# Canonicalized from %s\n", original_where); diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c index e03cdbd5c0..2067eeaf89 100644 --- a/src/gpt-auto-generator/gpt-auto-generator.c +++ b/src/gpt-auto-generator/gpt-auto-generator.c @@ -105,9 +105,8 @@ static int open_parent_block_device(dev_t devnum, int *ret_fd) { } static int add_cryptsetup(const char *id, const char *what, bool rw, bool require, char **device) { - _cleanup_free_ char *e = NULL, *n = NULL, *d = NULL, *id_escaped = NULL, *what_escaped = NULL; + _cleanup_free_ char *e = NULL, *n = NULL, *d = NULL; _cleanup_fclose_ FILE *f = NULL; - const char *p; int r; assert(id); @@ -125,44 +124,28 @@ static int add_cryptsetup(const char *id, const char *what, bool rw, bool requir if (r < 0) return log_error_errno(r, "Failed to generate unit name: %m"); - id_escaped = specifier_escape(id); - if (!id_escaped) - return log_oom(); + r = generator_open_unit_file(arg_dest, NULL, n, &f); + if (r < 0) + return r; - what_escaped = specifier_escape(what); - if (!what_escaped) - return log_oom(); - - p = prefix_roota(arg_dest, n); - f = fopen(p, "wxe"); - if (!f) - return log_error_errno(errno, "Failed to create unit file %s: %m", p); + r = generator_write_cryptsetup_unit_section(f, NULL); + if (r < 0) + return r; fprintf(f, - "# Automatically generated by systemd-gpt-auto-generator\n\n" - "[Unit]\n" - "Description=Cryptography Setup for %%I\n" - "Documentation=man:systemd-gpt-auto-generator(8) man:systemd-cryptsetup@.service(8)\n" - "DefaultDependencies=no\n" - "Conflicts=umount.target\n" - "BindsTo=dev-mapper-%%i.device %s\n" "Before=umount.target cryptsetup.target\n" - "After=%s\n" - "IgnoreOnIsolate=true\n" - "[Service]\n" - "Type=oneshot\n" - "RemainAfterExit=yes\n" - "TimeoutSec=0\n" /* the binary handles timeouts anyway */ - "KeyringMode=shared\n" /* make sure we can share cached keys among instances */ - "ExecStart=" SYSTEMD_CRYPTSETUP_PATH " attach '%s' '%s' '' '%s'\n" - "ExecStop=" SYSTEMD_CRYPTSETUP_PATH " detach '%s'\n", - d, d, - id_escaped, what_escaped, rw ? "" : "read-only", - id_escaped); + "Conflicts=umount.target\n" + "BindsTo=%s\n" + "After=%s\n", + d, d); + + r = generator_write_cryptsetup_service_section(f, id, what, NULL, rw ? NULL : "read-only"); + if (r < 0) + return r; r = fflush_and_check(f); if (r < 0) - return log_error_errno(r, "Failed to write file %s: %m", p); + return log_error_errno(r, "Failed to write file %s: %m", n); r = generator_add_symlink(arg_dest, d, "wants", n); if (r < 0) @@ -227,7 +210,6 @@ static int add_mount( log_debug("Adding %s: %s fstype=%s", where, what, fstype ?: "(any)"); if (streq_ptr(fstype, "crypto_LUKS")) { - r = add_cryptsetup(id, what, rw, true, &crypto_what); if (r < 0) return r; @@ -262,6 +244,10 @@ static int add_mount( if (r < 0) return r; + r = generator_write_blockdev_dependency(f, what); + if (r < 0) + return r; + fprintf(f, "\n" "[Mount]\n" @@ -370,7 +356,14 @@ static int add_swap(const char *path) { "# Automatically generated by systemd-gpt-auto-generator\n\n" "[Unit]\n" "Description=Swap Partition\n" - "Documentation=man:systemd-gpt-auto-generator(8)\n\n" + "Documentation=man:systemd-gpt-auto-generator(8)\n"); + + r = generator_write_blockdev_dependency(f, path); + if (r < 0) + return r; + + fprintf(f, + "\n" "[Swap]\n" "What=%s\n", path); diff --git a/src/shared/generator.c b/src/shared/generator.c index 06e1ab8031..48667c93a2 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -513,6 +513,103 @@ int generator_enable_remount_fs_service(const char *dir) { SYSTEM_DATA_UNIT_PATH "/" SPECIAL_REMOUNT_FS_SERVICE); } +int generator_write_blockdev_dependency( + FILE *f, + const char *what) { + + _cleanup_free_ char *escaped = NULL; + int r; + + assert(f); + assert(what); + + if (!path_startswith(what, "/dev/")) + return 0; + + r = unit_name_path_escape(what, &escaped); + if (r < 0) + return log_error_errno(r, "Failed to escape device node path %s: %m", what); + + fprintf(f, + "After=blockdev@%s.target\n", + escaped); + + return 0; +} + +int generator_write_cryptsetup_unit_section( + FILE *f, + const char *source) { + + assert(f); + + fprintf(f, + "[Unit]\n" + "Description=Cryptography Setup for %%I\n" + "Documentation=man:crypttab(5) man:systemd-cryptsetup-generator(8) man:systemd-cryptsetup@.service(8)\n"); + + if (source) + fprintf(f, "SourcePath=%s\n", source); + + fprintf(f, + "DefaultDependencies=no\n" + "IgnoreOnIsolate=true\n" + "After=cryptsetup-pre.target\n" + "Before=blockdev@dev-mapper-%%i.target\n" + "Wants=blockdev@dev-mapper-%%i.target\n"); + + return 0; +} + +int generator_write_cryptsetup_service_section( + FILE *f, + const char *name, + const char *what, + const char *password, + const char *options) { + + _cleanup_free_ char *name_escaped = NULL, *what_escaped = NULL, *password_escaped = NULL, *options_escaped = NULL; + + assert(f); + assert(name); + assert(what); + + name_escaped = specifier_escape(name); + if (!name_escaped) + return log_oom(); + + what_escaped = specifier_escape(what); + if (!what_escaped) + return log_oom(); + + if (password) { + password_escaped = specifier_escape(password); + if (!password_escaped) + return log_oom(); + } + + if (options) { + options_escaped = specifier_escape(options); + if (!options_escaped) + return log_oom(); + } + + fprintf(f, + "\n" + "[Service]\n" + "Type=oneshot\n" + "RemainAfterExit=yes\n" + "TimeoutSec=0\n" /* The binary handles timeouts on its own */ + "KeyringMode=shared\n" /* Make sure we can share cached keys among instances */ + "OOMScoreAdjust=500\n" /* Unlocking can allocate a lot of memory if Argon2 is used */ + "ExecStart=" SYSTEMD_CRYPTSETUP_PATH " attach '%s' '%s' '%s' '%s'\n" + "ExecStop=" SYSTEMD_CRYPTSETUP_PATH " detach '%s'\n", + name_escaped, what_escaped, strempty(password_escaped), strempty(options_escaped), + name_escaped); + + return 0; +} + void log_setup_generator(void) { log_set_prohibit_ipc(true); log_setup_service(); diff --git a/src/shared/generator.h b/src/shared/generator.h index fa002a9114..579e291fe8 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -27,6 +27,21 @@ int generator_write_timeouts( const char *opts, char **filtered); +int generator_write_blockdev_dependency( + FILE *f, + const char *what); + +int generator_write_cryptsetup_unit_section( + FILE *f, + const char *source); + +int generator_write_cryptsetup_service_section( + FILE *f, + const char *name, + const char *what, + const char *password, + const char *options); + int generator_write_device_deps( const char *dir, const char *what, diff --git a/units/blockdev@.target b/units/blockdev@.target new file mode 100644 index 0000000000..22a9a5bb2b --- /dev/null +++ b/units/blockdev@.target @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: LGPL-2.1+ +# +# This file is part of systemd. +# +# systemd is free software; you can redistribute it and/or modify it +# under the terms of the GNU Lesser General Public License as published by +# the Free Software Foundation; either version 2.1 of the License, or +# (at your option) any later version. + +[Unit] +Description=Block Device Preparation for %f +Documentation=man:systemd.special(7) +StopWhenUnneeded=yes diff --git a/units/meson.build b/units/meson.build index f7653c920c..27742044c5 100644 --- a/units/meson.build +++ b/units/meson.build @@ -2,6 +2,7 @@ units = [ ['basic.target', ''], + ['blockdev@.target', ''], ['bluetooth.target', ''], ['boot-complete.target', ''], ['cryptsetup-pre.target', 'HAVE_LIBCRYPTSETUP'], From 219f3cd94106033f4d7081fbb0891439197b6052 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Jan 2020 18:08:27 +0100 Subject: [PATCH 06/11] core: drop _pure_ from static functions For static functions the compiler should figure this out on its own. --- 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 668c4d7e89..cf19af10b1 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -217,7 +217,7 @@ static void mount_done(Unit *u) { m->timer_event_source = sd_event_source_unref(m->timer_event_source); } -_pure_ static MountParameters* get_mount_parameters_fragment(Mount *m) { +static MountParameters* get_mount_parameters_fragment(Mount *m) { assert(m); if (m->from_fragment) @@ -226,7 +226,7 @@ _pure_ static MountParameters* get_mount_parameters_fragment(Mount *m) { return NULL; } -_pure_ static MountParameters* get_mount_parameters(Mount *m) { +static MountParameters* get_mount_parameters(Mount *m) { assert(m); if (m->from_proc_self_mountinfo) From 5de0acf40d32e18e55712a045e708d60fcf2c9a8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Jan 2020 18:09:09 +0100 Subject: [PATCH 07/11] core: let's be defensive, /dev/nfs is also a special mount source, filter it out --- src/core/mount.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index cf19af10b1..0964eb1864 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -342,10 +342,9 @@ static int mount_add_device_dependencies(Mount *m) { if (!is_device_path(p->what)) return 0; - /* /dev/root is a really weird thing, it's not a real device, - * but just a path the kernel exports for the root file system - * specified on the kernel command line. Ignore it here. */ - if (path_equal(p->what, "/dev/root")) + /* /dev/root is a really weird thing, it's not a real device, but just a path the kernel exports for + * the root file system specified on the kernel command line. Ignore it here. */ + if (PATH_IN_SET(p->what, "/dev/root", "/dev/nfs")) return 0; if (path_equal(m->where, "/")) From 61f9cf4e4c488adf47419a4a81a315196fc01d43 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Jan 2020 18:16:05 +0100 Subject: [PATCH 08/11] swap: generate automatic dependencies also for /proc/swaps devices This catches up with the logic we do for mounts: we create deps based on /proc/swaps now too, with the right flags set. --- src/core/swap.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/core/swap.c b/src/core/swap.c index 473b3483ae..4155112f41 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -184,21 +184,39 @@ static int swap_arm_timer(Swap *s, usec_t usec) { return 0; } +static SwapParameters* swap_get_parameters(Swap *s) { + assert(s); + + if (s->from_proc_swaps) + return &s->parameters_proc_swaps; + + if (s->from_fragment) + return &s->parameters_fragment; + + return NULL; +} + static int swap_add_device_dependencies(Swap *s) { + UnitDependencyMask mask; + SwapParameters *p; + assert(s); if (!s->what) return 0; - if (!s->from_fragment) + p = swap_get_parameters(s); + if (!p) return 0; - if (is_device_path(s->what)) - return unit_add_node_dependency(UNIT(s), s->what, UNIT_BINDS_TO, UNIT_DEPENDENCY_FILE); + mask = s->from_proc_swaps ? UNIT_DEPENDENCY_PROC_SWAP : UNIT_DEPENDENCY_FILE; - /* File based swap devices need to be ordered after systemd-remount-fs.service, - * since they might need a writable file system. */ - return unit_add_dependency_by_name(UNIT(s), UNIT_AFTER, SPECIAL_REMOUNT_FS_SERVICE, true, UNIT_DEPENDENCY_FILE); + if (is_device_path(p->what)) + return unit_add_node_dependency(UNIT(s), p->what, UNIT_BINDS_TO, mask); + + /* File based swap devices need to be ordered after systemd-remount-fs.service, since they might need + * a writable file system. */ + return unit_add_dependency_by_name(UNIT(s), UNIT_AFTER, SPECIAL_REMOUNT_FS_SERVICE, true, mask); } static int swap_add_default_dependencies(Swap *s) { From e3e6f996894f0eea0e766b4194922f5c7235fb01 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Jan 2020 18:16:53 +0100 Subject: [PATCH 09/11] =?UTF-8?q?core:=20downgrade=20swap=20=E2=86=92=20de?= =?UTF-8?q?vice=20dep=20to=20Requires=3D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This catches up with 9d06297e262966de71095debd1537fc223f940a3 and adapts the change made to swap units. We generally don't want to react a-posteriori to swap devices disappearing, bad things will happen anyway. --- src/core/swap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/swap.c b/src/core/swap.c index 4155112f41..713d785618 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -212,7 +212,7 @@ static int swap_add_device_dependencies(Swap *s) { mask = s->from_proc_swaps ? UNIT_DEPENDENCY_PROC_SWAP : UNIT_DEPENDENCY_FILE; if (is_device_path(p->what)) - return unit_add_node_dependency(UNIT(s), p->what, UNIT_BINDS_TO, mask); + return unit_add_node_dependency(UNIT(s), p->what, UNIT_REQUIRES, mask); /* File based swap devices need to be ordered after systemd-remount-fs.service, since they might need * a writable file system. */ From 44b0d1fd597d7034a995feb385b41a6820f4aac0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Jan 2020 18:19:08 +0100 Subject: [PATCH 10/11] core: add implicit ordering dep on blockdev@.target from all mount units This way we shuld be able to order mounts properly against their backing services in case complex storage is used (i.e. LUKS), even if the device path used for mounting the devices is different from the expected device node of the backing service. Specifically, if we have a LUKS device /dev/mapper/foo that is mounted by this name all is trivial as the relationship can be established a priori easily. But if it is mounted via a /dev/disk/by-uuid/ symlink or similar we only can relate the device node generated to the one mounted at the moment the device is actually established. That's because the UUID of the fs is stored inside the encrypted volume and thus not knowable until the volume is set up. This patch tries to improve on this situation: a implicit After=blockdev@.target dependency is generated for all mounts, based on the data from /proc/self/mountinfo, which should be the actual device node, with all symlinks resolved. This means that as soon as the mount is established the ordering via blockdev@.target will work, and that means during shutdown it is honoured, which is what we are looking for. Note that specifying /etc/fstab entries via UUID= for LUKS devices still sucks and shouldn't be done, because it means we cannot know which LUKS device to activate to make an fs appear, and that means unless the volume is set up at boot anyway we can't really handle things automatically when putting together transactions that need the mount. --- src/core/mount.c | 11 +++++------ src/core/swap.c | 10 ++++++++-- src/core/unit.c | 32 +++++++++++++++++++++++++++++--- src/core/unit.h | 1 + 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 0964eb1864..1c4aefd734 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -350,11 +350,10 @@ static int mount_add_device_dependencies(Mount *m) { if (path_equal(m->where, "/")) return 0; - /* Mount units from /proc/self/mountinfo are not bound to devices - * by default since they're subject to races when devices are - * unplugged. But the user can still force this dep with an - * appropriate option (or udev property) so the mount units are - * automatically stopped when the device disappears suddenly. */ + /* Mount units from /proc/self/mountinfo are not bound to devices by default since they're subject to + * races when devices are unplugged. But the user can still force this dep with an appropriate option + * (or udev property) so the mount units are automatically stopped when the device disappears + * suddenly. */ dep = mount_is_bound_to_device(m) ? UNIT_BINDS_TO : UNIT_REQUIRES; /* We always use 'what' from /proc/self/mountinfo if mounted */ @@ -364,7 +363,7 @@ static int mount_add_device_dependencies(Mount *m) { if (r < 0) return r; - return 0; + return unit_add_blockdev_dependency(UNIT(m), p->what, mask); } static int mount_add_quota_dependencies(Mount *m) { diff --git a/src/core/swap.c b/src/core/swap.c index 713d785618..fd2fa557db 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -199,6 +199,7 @@ static SwapParameters* swap_get_parameters(Swap *s) { static int swap_add_device_dependencies(Swap *s) { UnitDependencyMask mask; SwapParameters *p; + int r; assert(s); @@ -211,8 +212,13 @@ static int swap_add_device_dependencies(Swap *s) { mask = s->from_proc_swaps ? UNIT_DEPENDENCY_PROC_SWAP : UNIT_DEPENDENCY_FILE; - if (is_device_path(p->what)) - return unit_add_node_dependency(UNIT(s), p->what, UNIT_REQUIRES, mask); + if (is_device_path(p->what)) { + r = unit_add_node_dependency(UNIT(s), p->what, UNIT_REQUIRES, mask); + if (r < 0) + return r; + + return unit_add_blockdev_dependency(UNIT(s), p->what, mask); + } /* File based swap devices need to be ordered after systemd-remount-fs.service, since they might need * a writable file system. */ diff --git a/src/core/unit.c b/src/core/unit.c index c629a1a9ce..9e95857d9a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3846,8 +3846,8 @@ int unit_deserialize_skip(FILE *f) { } int unit_add_node_dependency(Unit *u, const char *what, UnitDependency dep, UnitDependencyMask mask) { - Unit *device; _cleanup_free_ char *e = NULL; + Unit *device; int r; assert(u); @@ -3859,8 +3859,7 @@ int unit_add_node_dependency(Unit *u, const char *what, UnitDependency dep, Unit if (!is_device_path(what)) return 0; - /* When device units aren't supported (such as in a - * container), don't create dependencies on them. */ + /* When device units aren't supported (such as in a container), don't create dependencies on them. */ if (!unit_type_supported(UNIT_DEVICE)) return 0; @@ -3880,6 +3879,33 @@ int unit_add_node_dependency(Unit *u, const char *what, UnitDependency dep, Unit device, true, mask); } +int unit_add_blockdev_dependency(Unit *u, const char *what, UnitDependencyMask mask) { + _cleanup_free_ char *escaped = NULL, *target = NULL; + int r; + + assert(u); + + if (isempty(what)) + return 0; + + if (!path_startswith(what, "/dev/")) + return 0; + + /* If we don't support devices, then also don't bother with blockdev@.target */ + if (!unit_type_supported(UNIT_DEVICE)) + return 0; + + r = unit_name_path_escape(what, &escaped); + if (r < 0) + return r; + + r = unit_name_build("blockdev", escaped, ".target", &target); + if (r < 0) + return r; + + return unit_add_dependency_by_name(u, UNIT_AFTER, target, true, mask); +} + int unit_coldplug(Unit *u) { int r = 0, q; char **i; diff --git a/src/core/unit.h b/src/core/unit.h index 68de900b0d..38d681dfb8 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -743,6 +743,7 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds); int unit_deserialize_skip(FILE *f); int unit_add_node_dependency(Unit *u, const char *what, UnitDependency d, UnitDependencyMask mask); +int unit_add_blockdev_dependency(Unit *u, const char *what, UnitDependencyMask mask); int unit_coldplug(Unit *u); void unit_catchup(Unit *u); From 68bda079fd089e070ac8c93d5eb324e8cfc391d0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Jan 2020 20:22:15 +0100 Subject: [PATCH 11/11] man: document blockdev@.target --- man/systemd.special.xml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/man/systemd.special.xml b/man/systemd.special.xml index c839af1842..f393283a1d 100644 --- a/man/systemd.special.xml +++ b/man/systemd.special.xml @@ -26,6 +26,7 @@ cryptsetup-pre.target, cryptsetup.target, ctrl-alt-del.target, + blockdev@.target, boot-complete.target, default.target, emergency.target, @@ -845,6 +846,23 @@ not useful as only unit within a transaction. + + blockdev@.target + This template unit may be used to order mount units and other consumers of block + devices against services that synthesize these block devices. This is intended to be used to order + storage services (such as + systemd-cryptsetup@.service5) + that allocate and manage a virtual block device against mount units and other consumers of + it. Specifically, the storage services are supposed to be orderd before an instance of + blockdev@.target, and the mount unit (or other consuming unit, such as a swap + unit) after it. The ordering is particular relevant during shutdown, as it ensures that the mount + is deactivated first and the service backing the mount only deactivated after that completed. The + blockdev@.target instance should be pulled in via a + dependency of the storage daemon and thus generally not be part of any transaction unless a storage + daemon is used. The instance name for instances of this template unit is supposed to be the + properly escaped bock device node path, e.g. blockdev@dev-mapper-foobar.target + for a storage device /dev/mapper/foobar. + cryptsetup-pre.target