From 25cd49647c83bacaee591afbc9b6fc047be66b58 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Fri, 19 Jan 2018 17:28:38 +0000 Subject: [PATCH 1/4] mount: forbid mount on path with symlinks It was forbidden to create mount units for a symlink. But the reason is that the mount unit needs to know the real path that will appear in /proc/self/mountinfo. The kernel dereferences *all* the symlinks in the path at mount time (I checked this with `mount -c` running under `strace`). This will have no effect on most systems. As recommended by docs, most systems use /etc/fstab, as opposed to native mount unit files. fstab-generator dereferences symlinks for backwards compatibility. A relatively minor issue regarding Time Of Check / Time Of Use also exists here. I can't see how to get rid of it entirely. If we pass an absolute path to mount, the racing process can replace it with a symlink. If we chdir() to the mount point and pass ".", the racing process can move the directory. The latter might potentially be nicer, except that it breaks WorkingDirectory=. I'm not saying the race is relevant to security - I just want to consider how bad the effect is. Currently, it can make the mount unit active (and hence the job return success), despite there never being a matching entry in /proc/self/mountinfo. This wart will be removed in the next commit; i.e. it will make the mount unit fail instead. --- src/core/automount.c | 2 +- src/core/mount.c | 2 +- src/core/unit.c | 14 +++++++++----- src/core/unit.h | 2 +- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/core/automount.c b/src/core/automount.c index 28d5cc3917..c191336c0e 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -578,7 +578,7 @@ static void automount_enter_waiting(Automount *a) { set_clear(a->tokens); - r = unit_fail_if_symlink(UNIT(a), a->where); + r = unit_fail_if_noncanonical(UNIT(a), a->where); if (r < 0) goto fail; diff --git a/src/core/mount.c b/src/core/mount.c index d424e5a3a8..49014a5c3b 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -942,7 +942,7 @@ static void mount_enter_mounting(Mount *m) { assert(m); - r = unit_fail_if_symlink(UNIT(m), m->where); + r = unit_fail_if_noncanonical(UNIT(m), m->where); if (r < 0) goto fail; diff --git a/src/core/unit.c b/src/core/unit.c index d736d274de..308fe3f3e8 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4705,25 +4705,29 @@ void unit_warn_if_dir_nonempty(Unit *u, const char* where) { NULL); } -int unit_fail_if_symlink(Unit *u, const char* where) { +int unit_fail_if_noncanonical(Unit *u, const char* where) { + _cleanup_free_ char *canonical_where; int r; assert(u); assert(where); - r = is_symlink(where); + r = chase_symlinks(where, NULL, CHASE_NONEXISTENT, &canonical_where); if (r < 0) { - log_unit_debug_errno(u, r, "Failed to check symlink %s, ignoring: %m", where); + log_unit_debug_errno(u, r, "Failed to check %s for symlinks, ignoring: %m", where); return 0; } - if (r == 0) + + /* We will happily ignore a trailing slash (or any redundant slashes) */ + if (path_equal(where, canonical_where)) return 0; + /* No need to mention "." or "..", they would already have been rejected by unit_name_from_path() */ log_struct(LOG_ERR, "MESSAGE_ID=" SD_MESSAGE_OVERMOUNTING_STR, LOG_UNIT_ID(u), LOG_UNIT_INVOCATION_ID(u), - LOG_UNIT_MESSAGE(u, "Mount on symlink %s not allowed.", where), + LOG_UNIT_MESSAGE(u, "Mount path %s is not canonical (contains a symlink).", where), "WHERE=%s", where, NULL); diff --git a/src/core/unit.h b/src/core/unit.h index f8a12947cf..f3e991e79e 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -760,7 +760,7 @@ static inline bool unit_supported(Unit *u) { } void unit_warn_if_dir_nonempty(Unit *u, const char* where); -int unit_fail_if_symlink(Unit *u, const char* where); +int unit_fail_if_noncanonical(Unit *u, const char* where); int unit_start_limit_test(Unit *u); From 006aabbd052ce46ec35c8f90afe042f84c81c643 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sat, 20 Jan 2018 20:05:52 +0000 Subject: [PATCH 2/4] mount: mountinfo event is supposed to always arrive before SIGCHLD "Due to the io event priority logic we can be sure the new mountinfo is loaded before we process the SIGCHLD for the mount command." I think this is a reasonable expectation. But if it works, then the other comment must be false: "Note that mount(8) returning and the kernel sending us a mount table change event might happen out-of-order." Therefore we can clean up the code for the latter. If this is working as advertised, then we can make sure that mount units fail if the mount we thought we were creating did not actually appear, due to races or trickery (or because /sbin/mount did something unexpected despite returning EXIT_SUCCESS). Include a specific warning message for this failure. If we give up when the mount point is still mounted after 32 successful calls to /sbin/umount, that seems a fairly similar case. So make that message a LOG_WARN as well (not LOG_DEBUG). Also, this was recently changed to only retry while umount is returning EXIT_SUCCESS; in that case in particular there would be no other messages in the log to suggest what had happened. --- src/core/mount.c | 32 +++++++++++++++++--------------- src/core/mount.h | 3 ++- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 49014a5c3b..a9d81f2aa1 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1280,23 +1280,25 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) { log_unit_full(u, f == MOUNT_SUCCESS ? LOG_DEBUG : LOG_NOTICE, 0, "Mount process exited, code=%s status=%i", sigchld_code_to_string(code), status); - /* Note that mount(8) returning and the kernel sending us a mount table change event might happen - * out-of-order. If an operation succeed we assume the kernel will follow soon too and already change into the - * resulting state. If it fails we check if the kernel still knows about the mount. and change state - * accordingly. */ + /* Note that due to the io event priority logic, we can be sure the new mountinfo is loaded + * before we process the SIGCHLD for the mount command. */ switch (m->state) { case MOUNT_MOUNTING: - case MOUNT_MOUNTING_DONE: + /* Our mount point has not appeared in mountinfo. Something went wrong. */ - if (f == MOUNT_SUCCESS || m->from_proc_self_mountinfo) - /* If /bin/mount returned success, or if we see the mount point in /proc/self/mountinfo we are - * happy. If we see the first condition first, we should see the second condition - * immediately after – or /bin/mount lies to us and is broken. */ - mount_enter_mounted(m, f); - else - mount_enter_dead(m, f); + if (f == MOUNT_SUCCESS) { + /* Either /bin/mount has an unexpected definition of success, + * or someone raced us and we lost. */ + log_unit_warning(UNIT(m), "Mount process finished, but there is no mount."); + f = MOUNT_FAILURE_PROTOCOL; + } + mount_enter_dead(m, f); + break; + + case MOUNT_MOUNTING_DONE: + mount_enter_mounted(m, f); break; case MOUNT_REMOUNTING: @@ -1312,15 +1314,14 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (f == MOUNT_SUCCESS && m->from_proc_self_mountinfo) { /* Still a mount point? If so, let's try again. Most likely there were multiple mount points - * stacked on top of each other. Note that due to the io event priority logic we can be sure - * the new mountinfo is loaded before we process the SIGCHLD for the mount command. */ + * stacked on top of each other. */ if (m->n_retry_umount < RETRY_UMOUNT_MAX) { log_unit_debug(u, "Mount still present, trying again."); m->n_retry_umount++; mount_enter_unmounting(m); } else { - log_unit_debug(u, "Mount still present after %u attempts to unmount, giving up.", m->n_retry_umount); + log_unit_warning(u, "Mount still present after %u attempts to unmount, giving up.", m->n_retry_umount); mount_enter_mounted(m, f); } } else @@ -1951,6 +1952,7 @@ static const char* const mount_result_table[_MOUNT_RESULT_MAX] = { [MOUNT_FAILURE_SIGNAL] = "signal", [MOUNT_FAILURE_CORE_DUMP] = "core-dump", [MOUNT_FAILURE_START_LIMIT_HIT] = "start-limit-hit", + [MOUNT_FAILURE_PROTOCOL] = "protocol", }; DEFINE_STRING_TABLE_LOOKUP(mount_result, MountResult); diff --git a/src/core/mount.h b/src/core/mount.h index 44fe3b889e..1a496def84 100644 --- a/src/core/mount.h +++ b/src/core/mount.h @@ -35,12 +35,13 @@ typedef enum MountExecCommand { typedef enum MountResult { MOUNT_SUCCESS, - MOUNT_FAILURE_RESOURCES, + MOUNT_FAILURE_RESOURCES, /* a bit of a misnomer, just our catch-all error for errnos we didn't expect */ MOUNT_FAILURE_TIMEOUT, MOUNT_FAILURE_EXIT_CODE, MOUNT_FAILURE_SIGNAL, MOUNT_FAILURE_CORE_DUMP, MOUNT_FAILURE_START_LIMIT_HIT, + MOUNT_FAILURE_PROTOCOL, _MOUNT_RESULT_MAX, _MOUNT_RESULT_INVALID = -1 } MountResult; From 570183612189f5dcc4f7a613cd13da43d652b4cf Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Mon, 22 Jan 2018 17:42:25 +0000 Subject: [PATCH 3/4] mount: clarify that umount retries do not (anymore) allow multiple timeouts It _looks_ as if, back when we used to retry unsuccessful calls to umount, this would have inflated the effective timeout. Multiplying it by RETRY_UMOUNT_MAX. Which is set to 32. I'm surprised if it's true: I would have expected it to be noticed during the work on NFS timeouts. But I can't see what would have stopped it. Clarify that I do not expect this to happen anymore. I think each individual umount call is allowed up to the full timeout, but if umount ever exited with a signal status, we would stop retrying. To be extra clear, make sure that we do not retry in the event that umount perversely returned EXIT_SUCCESS after receiving SIGTERM. --- src/core/mount.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index a9d81f2aa1..12f9bb2af6 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1308,13 +1308,12 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) { break; case MOUNT_UNMOUNTING: - case MOUNT_UNMOUNTING_SIGKILL: - case MOUNT_UNMOUNTING_SIGTERM: if (f == MOUNT_SUCCESS && m->from_proc_self_mountinfo) { /* Still a mount point? If so, let's try again. Most likely there were multiple mount points - * stacked on top of each other. */ + * stacked on top of each other. We might exceed the timeout specified by the user overall, + * but we will stop as soon as any one umount times out. */ if (m->n_retry_umount < RETRY_UMOUNT_MAX) { log_unit_debug(u, "Mount still present, trying again."); @@ -1329,6 +1328,11 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) { break; + case MOUNT_UNMOUNTING_SIGKILL: + case MOUNT_UNMOUNTING_SIGTERM: + mount_enter_dead_or_mounted(m, f); + break; + default: assert_not_reached("Uh, control process died at wrong time."); } From b6ba0c164d23e2612ebd14d5d942f918ae955730 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sat, 20 Jan 2018 20:12:09 +0000 Subject: [PATCH 4/4] mount: don't consider activated until /sbin/mount returns So far, we considered mount units activated as soon as the mount appeared. This avoided seeing a difference between mounts started by systemd, and e.g. by running `mount` from a terminal. (`umount` was not handled this way). However in some cases, options passed to `mount` require additional system calls after the mount is successfully created. E.g. the `private` mount option, or the `ro` option on bind mounts. It seems best to wait for mount to finish doing that. E.g. in the `private` case, the current behaviour could theoretically cause non-deterministic results, as child mounts inherit the private/shared propagation setting from their parent. This also avoids a special case in mount_reload(). --- src/core/mount.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 12f9bb2af6..453a83f93d 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -55,7 +55,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter); static const UnitActiveState state_translation_table[_MOUNT_STATE_MAX] = { [MOUNT_DEAD] = UNIT_INACTIVE, [MOUNT_MOUNTING] = UNIT_ACTIVATING, - [MOUNT_MOUNTING_DONE] = UNIT_ACTIVE, + [MOUNT_MOUNTING_DONE] = UNIT_ACTIVATING, [MOUNT_MOUNTED] = UNIT_ACTIVE, [MOUNT_REMOUNTING] = UNIT_RELOADING, [MOUNT_UNMOUNTING] = UNIT_DEACTIVATING, @@ -1135,10 +1135,6 @@ static int mount_reload(Unit *u) { Mount *m = MOUNT(u); assert(m); - - if (m->state == MOUNT_MOUNTING_DONE) /* not yet ready to reload, try again */ - return -EAGAIN; - assert(m->state == MOUNT_MOUNTED); mount_enter_remounting(m);