From dcebb015fb168351039ab0c89f49f485503deeb0 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Fri, 10 Mar 2023 12:25:42 +0100 Subject: [PATCH 1/4] execute: Use log_unit_error_errno() instead of log_error_errno() --- src/core/execute.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 9db5b2cd17..3800c7a38b 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -4198,9 +4198,10 @@ static int get_open_file_fd(Unit *u, const OpenFile *of) { ofd = open(of->path, O_PATH | O_CLOEXEC); if (ofd < 0) - return log_error_errno(errno, "Could not open \"%s\": %m", of->path); + return log_unit_error_errno(u, errno, "Could not open \"%s\": %m", of->path); + if (fstat(ofd, &st) < 0) - return log_error_errno(errno, "Failed to stat %s: %m", of->path); + return log_unit_error_errno(u, errno, "Failed to stat %s: %m", of->path); if (S_ISSOCK(st.st_mode)) { fd = connect_unix_harder(u, of, ofd); @@ -4208,7 +4209,8 @@ static int get_open_file_fd(Unit *u, const OpenFile *of) { return fd; if (FLAGS_SET(of->flags, OPENFILE_READ_ONLY) && shutdown(fd, SHUT_WR) < 0) - return log_error_errno(errno, "Failed to shutdown send for socket %s: %m", of->path); + return log_unit_error_errno(u, errno, "Failed to shutdown send for socket %s: %m", + of->path); log_unit_debug(u, "socket %s opened (fd=%d)", of->path, fd); } else { From 846c9c12e75e832e41a6c21ee818f6726c142214 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 13 Mar 2023 13:03:32 +0100 Subject: [PATCH 2/4] lock-util: Add posix_lock() POSIX locks with the same interface as flock(). --- src/basic/lock-util.c | 27 ++++++++++++++++++++++++--- src/basic/lock-util.h | 8 +++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/basic/lock-util.c b/src/basic/lock-util.c index ba0c23445b..c7a7b68ebc 100644 --- a/src/basic/lock-util.c +++ b/src/basic/lock-util.c @@ -109,12 +109,15 @@ void release_lock_file(LockFile *f) { f->operation = 0; } -int unposix_lock(int fd, int operation) { +static int fcntl_lock(int fd, int operation, bool ofd) { int cmd, type, r; assert(fd >= 0); - cmd = (operation & LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW; + if (ofd) + cmd = (operation & LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW; + else + cmd = (operation & LOCK_NB) ? F_SETLK : F_SETLKW; switch (operation & ~LOCK_NB) { case LOCK_EX: @@ -143,12 +146,30 @@ int unposix_lock(int fd, int operation) { return r; } +int posix_lock(int fd, int operation) { + return fcntl_lock(fd, operation, /*ofd=*/ false); +} + +int unposix_lock(int fd, int operation) { + return fcntl_lock(fd, operation, /*ofd=*/ true); +} + +void posix_unlockpp(int **fd) { + assert(fd); + + if (!*fd || **fd < 0) + return; + + (void) fcntl_lock(**fd, LOCK_UN, /*ofd=*/ false); + *fd = NULL; +} + void unposix_unlockpp(int **fd) { assert(fd); if (!*fd || **fd < 0) return; - (void) unposix_lock(**fd, LOCK_UN); + (void) fcntl_lock(**fd, LOCK_UN, /*ofd=*/ true); *fd = NULL; } diff --git a/src/basic/lock-util.h b/src/basic/lock-util.h index e8d4c24874..6eebd09143 100644 --- a/src/basic/lock-util.h +++ b/src/basic/lock-util.h @@ -13,9 +13,15 @@ void release_lock_file(LockFile *f); #define LOCK_FILE_INIT { .fd = -EBADF, .path = NULL } +/* POSIX locks with the same interface as flock(). */ +int posix_lock(int fd, int operation); +void posix_unlockpp(int **fd); + +#define CLEANUP_POSIX_UNLOCK(fd) \ + _cleanup_(posix_unlockpp) _unused_ int *CONCATENATE(_cleanup_posix_unlock_, UNIQ) = &(fd) + /* Open File Description locks with the same interface as flock(). */ int unposix_lock(int fd, int operation); - void unposix_unlockpp(int **fd); #define CLEANUP_UNPOSIX_UNLOCK(fd) \ From 1fe15cb7a965f02c8050cb8cd28efa43e81792a7 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 13 Mar 2023 13:05:19 +0100 Subject: [PATCH 3/4] dynamic-user: Revert back to using POSIX locks unposix locks are shared between child and parent after fork() which is precisely what we don't want in this case so revert back to POSIX locks which are not shared between parent and child. --- src/core/dynamic-user.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/dynamic-user.c b/src/core/dynamic-user.c index 42936bf567..3824ae747f 100644 --- a/src/core/dynamic-user.c +++ b/src/core/dynamic-user.c @@ -383,11 +383,11 @@ static int dynamic_user_realize( /* Acquire a UID for the user name. This will allocate a UID for the user name if the user doesn't exist * yet. If it already exists its existing UID/GID will be reused. */ - r = unposix_lock(d->storage_socket[0], LOCK_EX); + r = posix_lock(d->storage_socket[0], LOCK_EX); if (r < 0) return r; - CLEANUP_UNPOSIX_UNLOCK(d->storage_socket[0]); + CLEANUP_POSIX_UNLOCK(d->storage_socket[0]); r = dynamic_user_pop(d, &num, &uid_lock_fd); if (r < 0) { @@ -399,7 +399,7 @@ static int dynamic_user_realize( /* OK, nothing stored yet, let's try to find something useful. While we are working on this release the * lock however, so that nobody else blocks on our NSS lookups. */ - r = unposix_lock(d->storage_socket[0], LOCK_UN); + r = posix_lock(d->storage_socket[0], LOCK_UN); if (r < 0) return r; @@ -451,7 +451,7 @@ static int dynamic_user_realize( } /* So, we found a working UID/lock combination. Let's see if we actually still need it. */ - r = unposix_lock(d->storage_socket[0], LOCK_EX); + r = posix_lock(d->storage_socket[0], LOCK_EX); if (r < 0) { unlink_uid_lock(uid_lock_fd, num, d->name); return r; @@ -523,11 +523,11 @@ int dynamic_user_current(DynamicUser *d, uid_t *ret) { /* Get the currently assigned UID for the user, if there's any. This simply pops the data from the * storage socket, and pushes it back in right-away. */ - r = unposix_lock(d->storage_socket[0], LOCK_EX); + r = posix_lock(d->storage_socket[0], LOCK_EX); if (r < 0) return r; - CLEANUP_UNPOSIX_UNLOCK(d->storage_socket[0]); + CLEANUP_POSIX_UNLOCK(d->storage_socket[0]); r = dynamic_user_pop(d, &uid, &lock_fd); if (r < 0) @@ -565,11 +565,11 @@ static int dynamic_user_close(DynamicUser *d) { /* Release the user ID, by releasing the lock on it, and emptying the storage socket. After this the * user is unrealized again, much like it was after it the DynamicUser object was first allocated. */ - r = unposix_lock(d->storage_socket[0], LOCK_EX); + r = posix_lock(d->storage_socket[0], LOCK_EX); if (r < 0) return r; - CLEANUP_UNPOSIX_UNLOCK(d->storage_socket[0]); + CLEANUP_POSIX_UNLOCK(d->storage_socket[0]); r = dynamic_user_pop(d, &uid, &lock_fd); if (r == -EAGAIN) From 13339577c339b470986203fdc988db7a8c8dc87d Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 8 Mar 2023 15:30:19 +0100 Subject: [PATCH 4/4] namespace: Modernize shareable namespace functions --- src/core/namespace.c | 140 +++++++++++++++++++------------------------ src/core/namespace.h | 4 +- 2 files changed, 63 insertions(+), 81 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index 252f5c9d2c..b4bb01bb74 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -24,6 +24,7 @@ #include "glyph-util.h" #include "label.h" #include "list.h" +#include "lock-util.h" #include "loop-util.h" #include "loopback-setup.h" #include "missing_syscall.h" @@ -2847,9 +2848,9 @@ int setup_tmp_dirs(const char *id, char **tmp_dir, char **var_tmp_dir) { return 0; } -int setup_shareable_ns(const int ns_storage_socket[static 2], unsigned long nsflag) { +int setup_shareable_ns(int ns_storage_socket[static 2], unsigned long nsflag) { _cleanup_close_ int ns = -EBADF; - int r, q; + int r; const char *ns_name, *ns_path; assert(ns_storage_socket); @@ -2867,57 +2868,47 @@ int setup_shareable_ns(const int ns_storage_socket[static 2], unsigned long nsfl * * It's a bit crazy, but hey, works great! */ - if (lockf(ns_storage_socket[0], F_LOCK, 0) < 0) + r = posix_lock(ns_storage_socket[0], LOCK_EX); + if (r < 0) + return r; + + CLEANUP_POSIX_UNLOCK(ns_storage_socket[0]); + + ns = receive_one_fd(ns_storage_socket[0], MSG_PEEK|MSG_DONTWAIT); + if (ns >= 0) { + /* Yay, found something, so let's join the namespace */ + r = RET_NERRNO(setns(ns, nsflag)); + if (r < 0) + return r; + + return 0; + } + + if (ns != -EAGAIN) + return ns; + + /* Nothing stored yet, so let's create a new namespace. */ + + if (unshare(nsflag) < 0) return -errno; - ns = receive_one_fd(ns_storage_socket[0], MSG_DONTWAIT); - if (ns == -EAGAIN) { - /* Nothing stored yet, so let's create a new namespace. */ + (void) loopback_setup(); - if (unshare(nsflag) < 0) { - r = -errno; - goto fail; - } + ns_path = strjoina("/proc/self/ns/", ns_name); + ns = open(ns_path, O_RDONLY|O_CLOEXEC|O_NOCTTY); + if (ns < 0) + return -errno; - (void) loopback_setup(); + r = send_one_fd(ns_storage_socket[1], ns, MSG_DONTWAIT); + if (r < 0) + return r; - ns_path = strjoina("/proc/self/ns/", ns_name); - ns = open(ns_path, O_RDONLY|O_CLOEXEC|O_NOCTTY); - if (ns < 0) { - r = -errno; - goto fail; - } - - r = 1; - - } else if (ns < 0) { - r = ns; - goto fail; - - } else { - /* Yay, found something, so let's join the namespace */ - if (setns(ns, nsflag) < 0) { - r = -errno; - goto fail; - } - - r = 0; - } - - q = send_one_fd(ns_storage_socket[1], ns, MSG_DONTWAIT); - if (q < 0) { - r = q; - goto fail; - } - -fail: - (void) lockf(ns_storage_socket[0], F_ULOCK, 0); - return r; + return 1; } -int open_shareable_ns_path(const int ns_storage_socket[static 2], const char *path, unsigned long nsflag) { +int open_shareable_ns_path(int ns_storage_socket[static 2], const char *path, unsigned long nsflag) { _cleanup_close_ int ns = -EBADF; - int q, r; + int r; assert(ns_storage_socket); assert(ns_storage_socket[0] >= 0); @@ -2928,44 +2919,35 @@ int open_shareable_ns_path(const int ns_storage_socket[static 2], const char *pa * it. This is supposed to be called ahead of time, i.e. before setup_shareable_ns() which will * allocate a new anonymous ns if needed. */ - if (lockf(ns_storage_socket[0], F_LOCK, 0) < 0) + r = posix_lock(ns_storage_socket[0], LOCK_EX); + if (r < 0) + return r; + + CLEANUP_POSIX_UNLOCK(ns_storage_socket[0]); + + ns = receive_one_fd(ns_storage_socket[0], MSG_PEEK|MSG_DONTWAIT); + if (ns >= 0) + return 0; + if (ns != -EAGAIN) + return ns; + + /* Nothing stored yet. Open the file from the file system. */ + + ns = open(path, O_RDONLY|O_NOCTTY|O_CLOEXEC); + if (ns < 0) return -errno; - ns = receive_one_fd(ns_storage_socket[0], MSG_DONTWAIT); - if (ns == -EAGAIN) { - /* Nothing stored yet. Open the file from the file system. */ + r = fd_is_ns(ns, nsflag); + if (r == 0) + return -EINVAL; + if (r < 0 && r != -EUCLEAN) /* EUCLEAN: we don't know */ + return r; - ns = open(path, O_RDONLY|O_NOCTTY|O_CLOEXEC); - if (ns < 0) { - r = -errno; - goto fail; - } + r = send_one_fd(ns_storage_socket[1], ns, MSG_DONTWAIT); + if (r < 0) + return r; - r = fd_is_ns(ns, nsflag); - if (r == 0) { /* Not a ns of our type? Refuse early. */ - r = -EINVAL; - goto fail; - } - if (r < 0 && r != -EUCLEAN) /* EUCLEAN: we don't know */ - goto fail; - - r = 1; - - } else if (ns < 0) { - r = ns; - goto fail; - } else - r = 0; /* Already allocated */ - - q = send_one_fd(ns_storage_socket[1], ns, MSG_DONTWAIT); - if (q < 0) { - r = q; - goto fail; - } - -fail: - (void) lockf(ns_storage_socket[0], F_ULOCK, 0); - return r; + return 1; } bool ns_type_supported(NamespaceType type) { diff --git a/src/core/namespace.h b/src/core/namespace.h index 74f78784b6..52ab6c4f2f 100644 --- a/src/core/namespace.h +++ b/src/core/namespace.h @@ -153,8 +153,8 @@ int setup_tmp_dirs( char **tmp_dir, char **var_tmp_dir); -int setup_shareable_ns(const int ns_storage_socket[static 2], unsigned long nsflag); -int open_shareable_ns_path(const int netns_storage_socket[static 2], const char *path, unsigned long nsflag); +int setup_shareable_ns(int ns_storage_socket[static 2], unsigned long nsflag); +int open_shareable_ns_path(int netns_storage_socket[static 2], const char *path, unsigned long nsflag); const char* protect_home_to_string(ProtectHome p) _const_; ProtectHome protect_home_from_string(const char *s) _pure_;