From 52a1e91e96c8ad18deebf651bbf21ebf72f6a9ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 21 Aug 2023 11:49:14 +0200 Subject: [PATCH 1/5] mount-tool,man: reword comments and error messages We were consistenly using --umount everywhere, but add a comment to make this clearer. Also, reword a few things for clarity. --- man/systemd-mount.xml | 6 +++--- src/mount/mount-tool.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/man/systemd-mount.xml b/man/systemd-mount.xml index 890c950cd0..3463a107da 100644 --- a/man/systemd-mount.xml +++ b/man/systemd-mount.xml @@ -70,9 +70,9 @@ whose name is generated from the file system label. In this mode the block device or image file must exist at the time of invocation of the command, so that it may be probed. If the device is found to be a removable block device (e.g. a USB stick), an automount point is created instead of a regular mount point - (i.e. the option is implied, see below). If the option - is specified, then the argument will be interpreted as the path where the new temporary file system will - be mounted on. + (i.e. the option is implied, see below). If the option + is specified, then the argument is interpreted as the path where the new + temporary file system shall be mounted. If two arguments are specified, the first indicates the mount source (the WHAT) and the second indicates the path to mount it on (the diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index 31abc0685d..a0f1503f70 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -206,7 +206,7 @@ static int parse_argv(int argc, char *argv[]) { { "bind-device", no_argument, NULL, ARG_BIND_DEVICE }, { "list", no_argument, NULL, ARG_LIST }, { "umount", no_argument, NULL, 'u' }, - { "unmount", no_argument, NULL, 'u' }, + { "unmount", no_argument, NULL, 'u' }, /* Compat spelling */ { "collect", no_argument, NULL, 'G' }, { "tmpfs", no_argument, NULL, 'T' }, {}, @@ -407,7 +407,7 @@ static int parse_argv(int argc, char *argv[]) { if (argc > optind+2) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "At most two arguments required."); + "More than two arguments are not allowed."); if (arg_tmpfs) { if (argc <= optind+1) { From 857127f1e303c02b5ab4d5aed53ce0310b67f144 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 21 Aug 2023 11:56:04 +0200 Subject: [PATCH 2/5] mount-tool: reduce scope of iterator variables --- src/mount/mount-tool.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index a0f1503f70..780179b3ed 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -391,15 +391,12 @@ static int parse_argv(int argc, char *argv[]) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "At least one argument required."); - if (arg_transport != BUS_TRANSPORT_LOCAL) { - int i; - - for (i = optind; i < argc; i++) - if (!path_is_absolute(argv[i]) ) + if (arg_transport != BUS_TRANSPORT_LOCAL) + for (int i = optind; i < argc; i++) + if (!path_is_absolute(argv[i])) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Path must be absolute when operating remotely: %s", argv[i]); - } } else { if (optind >= argc) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), @@ -1028,10 +1025,10 @@ static int action_umount( int argc, char **argv) { - int i, r, r2 = 0; + int r, r2 = 0; if (arg_transport != BUS_TRANSPORT_LOCAL) { - for (i = optind; i < argc; i++) { + for (int i = optind; i < argc; i++) { _cleanup_free_ char *p = NULL; p = strdup(argv[i]); @@ -1047,7 +1044,7 @@ static int action_umount( return r2; } - for (i = optind; i < argc; i++) { + for (int i = optind; i < argc; i++) { _cleanup_free_ char *u = NULL, *p = NULL; struct stat st; @@ -1411,7 +1408,6 @@ enum { static int list_devices(void) { _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; _cleanup_(table_unrefp) Table *table = NULL; - unsigned c; int r; r = sd_device_enumerator_new(&e); @@ -1440,7 +1436,7 @@ static int list_devices(void) { table_set_header(table, arg_legend); FOREACH_DEVICE(e, d) { - for (c = 0; c < _COLUMN_MAX; c++) { + for (unsigned c = 0; c < _COLUMN_MAX; c++) { const char *x = NULL; switch (c) { From e0cfdb85f55ba3f0e863e5825ca7ddccc9a77a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 21 Aug 2023 20:25:13 +0200 Subject: [PATCH 3/5] test-umask-util: add test for get_process_umask() It didn't have any test, and here we're setting umask, so we can add the test for free. --- src/test/test-umask-util.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/test/test-umask-util.c b/src/test/test-umask-util.c index bd5bc9cb43..8316dfb44a 100644 --- a/src/test/test-umask-util.c +++ b/src/test/test-umask-util.c @@ -1,11 +1,12 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "process-util.h" #include "tests.h" #include "umask-util.h" int main(int argc, char *argv[]) { size_t n; - mode_t u; + mode_t u, t; test_setup_logging(LOG_DEBUG); @@ -15,19 +16,31 @@ int main(int argc, char *argv[]) { WITH_UMASK(0123) { assert_se(umask(000) == 0123); n++; + + assert_se(get_process_umask(0, &t) == 0); + assert_se(t == 000); } assert_se(n == 1); assert_se(umask(u) == 0111); + assert_se(get_process_umask(getpid_cached(), &t) == 0); + assert_se(t == u); + WITH_UMASK(0135) { assert_se(umask(000) == 0135); n++; + + assert_se(get_process_umask(0, &t) == 0); + assert_se(t == 000); } assert_se(n == 2); assert_se(umask(0111) == u); + assert_se(get_process_umask(0, &t) == 0); + assert_se(t == 0111); + WITH_UMASK(0315) { assert_se(umask(000) == 0315); n++; @@ -37,5 +50,8 @@ int main(int argc, char *argv[]) { assert_se(n == 3); assert_se(umask(u) == 0111); + assert_se(get_process_umask(0, &t) == 0); + assert_se(t == u); + return EXIT_SUCCESS; } From 8980a53f24b5b5d43be947f4f959924d6b91fbd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 21 Aug 2023 20:44:05 +0200 Subject: [PATCH 4/5] basic/umask-util: drop clearing of unused bits in call to umask(2) umask(2) says that the kernel does this exact operation on its own, so we can drop if from our call. --- src/basic/umask-util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/umask-util.h b/src/basic/umask-util.h index 6f0e1cc2b2..00417fa304 100644 --- a/src/basic/umask-util.h +++ b/src/basic/umask-util.h @@ -8,12 +8,12 @@ #include "macro.h" static inline void umaskp(mode_t *u) { - umask(*u & 0777); + umask(*u); } #define _cleanup_umask_ _cleanup_(umaskp) -/* We make use of the fact here that the umask() concept is using only the lower 9 bits of mode_t, although +/* We make use of the fact here that the umask() syscall uses only the lower 9 bits of mode_t, although * mode_t has space for the file type in the bits further up. We simply OR in the file type mask S_IFMT to * distinguish the first and the second iteration of the WITH_UMASK() loop, so that we can run the first one, * and exit on the second. */ From 9269296df05a724c6b121bcbb0b420c8b4088f2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 21 Aug 2023 19:32:21 +0200 Subject: [PATCH 5/5] mount-tool: override mode of --tmpfs mounts to (rwxrwxrwx & ~umask) The kernel default for tmpfs mounts is rwxrwxrwt, i.e. world-writable with the sticky bit set. This makes sense for /tmp and /var/tmp, but less so for other directories. Users will not use systemd-mount to mount /tmp and /var/tmp, so the existing behaviour is surprising. The defaults are overriden to the same defaults we use for "internal tmpfs mounts", i.e. no sticky bit, nosuid, nodev, and limits of size=10%, nr_inodes=400k. --tmpfs is new, so we can modify the behaviour without breaking compatibility. --- man/systemd-mount.xml | 13 +++++++++++-- src/mount/mount-tool.c | 41 ++++++++++++++++++++++++++++++++--------- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/man/systemd-mount.xml b/man/systemd-mount.xml index 3463a107da..dfa4a95cb1 100644 --- a/man/systemd-mount.xml +++ b/man/systemd-mount.xml @@ -287,8 +287,17 @@ - Create and mount a new temporary file system on WHERE, with an - optional NAME that defaults to tmpfs. + Create and mount a new tmpfs file system on + WHERE, with an optional NAME that defaults to + tmpfs. + + The file system is mounted with the top-level directory mode determined by the + umask2 setting + of the caller, i.e. rwxrwxrwx masked by the umask of the caller. This matches + what + mkdir1 + does, but is different from the kernel default of rwxrwxrwxt, i.e. a + world-writable directory with the sticky bit set. diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index 780179b3ed..b167176a9f 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -35,6 +35,7 @@ #include "stat-util.h" #include "strv.h" #include "terminal-util.h" +#include "umask-util.h" #include "unit-def.h" #include "unit-name.h" #include "user-util.h" @@ -506,7 +507,6 @@ static int transient_unit_set_properties(sd_bus_message *m, UnitType t, char **p } static int transient_mount_set_properties(sd_bus_message *m) { - _cleanup_free_ char *options = NULL; int r; assert(m); @@ -527,20 +527,43 @@ static int transient_mount_set_properties(sd_bus_message *m) { return r; } + _cleanup_free_ char *options = NULL; + /* Prepend uid=…,gid=… if arg_uid is set */ if (arg_uid != UID_INVALID) { - r = asprintf(&options, - "uid=" UID_FMT ",gid=" GID_FMT "%s%s", - arg_uid, arg_gid, - arg_mount_options ? "," : "", strempty(arg_mount_options)); + r = strextendf_with_separator(&options, ",", + "uid="UID_FMT",gid="GID_FMT, arg_uid, arg_gid); if (r < 0) - return -ENOMEM; + return r; } - if (options || arg_mount_options) { - log_debug("Using mount options: %s", options ?: arg_mount_options); + /* Override the default for tmpfs mounts. The kernel sets the sticky bit on the root directory by + * default. This makes sense for the case when the user does 'mount -t tmpfs tmpfs /tmp', but less so + * for other directories. + * + * Let's also set some reasonable limits. We use the current umask, to match what a command to create + * directory would use, e.g. mkdir. */ + if (arg_tmpfs) { + mode_t mask; - r = sd_bus_message_append(m, "(sv)", "Options", "s", options ?: arg_mount_options); + r = get_process_umask(0, &mask); + if (r < 0) + return r; + + assert((mask & ~0777) == 0); + r = strextendf_with_separator(&options, ",", + "mode=0%o,nodev,nosuid%s", 0777 & ~mask, NESTED_TMPFS_LIMITS); + if (r < 0) + return r; + } + + if (arg_mount_options) + if (!strextend_with_separator(&options, ",", arg_mount_options)) + return r; + + if (options) { + log_debug("Using mount options: %s", options); + r = sd_bus_message_append(m, "(sv)", "Options", "s", options); if (r < 0) return r; } else