From e55db9e792c4db14f70c21ba16fc5445c62efd46 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Fri, 5 Apr 2024 12:14:18 +0200 Subject: [PATCH 1/4] log: fix comment --- src/basic/log.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/log.h b/src/basic/log.h index fc80902c45..6df164e06f 100644 --- a/src/basic/log.h +++ b/src/basic/log.h @@ -32,7 +32,7 @@ typedef enum LogTarget{ } LogTarget; /* This log level disables logging completely. It can only be passed to log_set_max_level() and cannot be - * used a regular log level. */ + * used as a regular log level. */ #define LOG_NULL (LOG_EMERG - 1) /* Note to readers: << and >> have lower precedence (are evaluated earlier) than & and | */ From a9805f8ca9c1561e373355fe7175579b31e1c08c Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Fri, 5 Apr 2024 12:18:58 +0200 Subject: [PATCH 2/4] test: make test-fd-util more lenient when using fd_move_above_stdio() On s390x this test fails when the SUT uses the z90crypt kernel module, as it's an another FD the test doesn't account for: /* test_rearrange_stdio */ Successfully forked off 'rearrange' as PID 57293. test_rearrange_stdio: r=0 /proc/57293/fd: total 0 lrwx------. 1 root root 64 Apr 5 06:18 0 -> /dev/pts/0 lrwx------. 1 root root 64 Apr 5 06:18 1 -> /dev/pts/0 lrwx------. 1 root root 64 Apr 5 06:18 2 -> /dev/pts/0 lrwx------. 1 root root 64 Apr 5 06:18 3 -> /dev/z90crypt rearrange terminated by signal ABRT. Debugging this was pain, since the child process didn't log anything once we closed stdout/stderr (for obvious reasons). Let's fix both issues by switching logging to kmsg once we close stdin/stdout/stderr, and also by making the test work fine when there are some extra FDs in the child's environment. --- src/test/test-fd-util.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c index cd4ce83597..f05cbbee6e 100644 --- a/src/test/test-fd-util.c +++ b/src/test/test-fd-util.c @@ -138,6 +138,7 @@ TEST(rearrange_stdio) { if (r == 0) { _cleanup_free_ char *path = NULL; + int pipe_read_fd, pair[2]; char buffer[10]; /* Child */ @@ -145,6 +146,10 @@ TEST(rearrange_stdio) { safe_close(STDERR_FILENO); /* Let's close an fd < 2, to make it more interesting */ assert_se(rearrange_stdio(-EBADF, -EBADF, -EBADF) >= 0); + /* Reconfigure logging after rearranging stdout/stderr, so we still log to somewhere if the + * following tests fail, making it slightly less annoying to debug */ + log_set_target(LOG_TARGET_KMSG); + log_open(); assert_se(fd_get_path(STDIN_FILENO, &path) >= 0); assert_se(path_equal(path, "/dev/null")); @@ -162,13 +167,12 @@ TEST(rearrange_stdio) { safe_close(STDOUT_FILENO); safe_close(STDERR_FILENO); - { - int pair[2]; - assert_se(pipe(pair) >= 0); - assert_se(pair[0] == 0); - assert_se(pair[1] == 1); - assert_se(fd_move_above_stdio(0) == 3); - } + assert_se(pipe(pair) >= 0); + assert_se(pair[0] == 0); + assert_se(pair[1] == 1); + pipe_read_fd = fd_move_above_stdio(0); + assert_se(pipe_read_fd >= 3); + assert_se(open("/dev/full", O_WRONLY|O_CLOEXEC) == 0); assert_se(acquire_data_fd("foobar") == 2); @@ -176,7 +180,7 @@ TEST(rearrange_stdio) { assert_se(write(1, "x", 1) < 0 && errno == ENOSPC); assert_se(write(2, "z", 1) == 1); - assert_se(read(3, buffer, sizeof(buffer)) == 1); + assert_se(read(pipe_read_fd, buffer, sizeof(buffer)) == 1); assert_se(buffer[0] == 'z'); assert_se(read(0, buffer, sizeof(buffer)) == 6); assert_se(memcmp(buffer, "foobar", 6) == 0); @@ -184,7 +188,7 @@ TEST(rearrange_stdio) { assert_se(rearrange_stdio(-EBADF, 1, 2) >= 0); assert_se(write(1, "a", 1) < 0 && errno == ENOSPC); assert_se(write(2, "y", 1) == 1); - assert_se(read(3, buffer, sizeof(buffer)) == 1); + assert_se(read(pipe_read_fd, buffer, sizeof(buffer)) == 1); assert_se(buffer[0] == 'y'); assert_se(fd_get_path(0, &path) >= 0); From 3ebd598624d1f371144df97a08fde40cd47664bf Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Fri, 5 Apr 2024 19:23:12 +0200 Subject: [PATCH 3/4] test: account for build dir being under one of the tmpfs-ed directories If we're running test-execute from the build directory which is under one of the tmpfs-ed directories (i.e. /root or /tmp), test-execute might behave strangely, since in that case manager_new() pins the system systemd-executor binary instead of the build dir one, which may lead to a very confusing test fails (if there's enough difference between the system and built sd-executor binary). Let's account for that and bind-mount the build dir under the tmpfs-ed directory if necessary. --- src/test/test-execute.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 76659fe071..294977bbf5 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -7,6 +7,7 @@ #include "sd-event.h" +#include "build-path.h" #include "capability-util.h" #include "cpu-set-util.h" #include "copy.h" @@ -1428,7 +1429,8 @@ static int prepare_ns(const char *process_name) { NULL); assert_se(r >= 0); if (r == 0) { - _cleanup_free_ char *unit_dir = NULL; + _cleanup_free_ char *unit_dir = NULL, *build_dir = NULL, *build_dir_mount = NULL; + int ret; /* Make "/" read-only. */ assert_se(mount_nofollow_verbose(LOG_DEBUG, NULL, "/", NULL, MS_BIND|MS_REMOUNT|MS_RDONLY, NULL) >= 0); @@ -1440,15 +1442,42 @@ static int prepare_ns(const char *process_name) { assert_se(mkdir_p(PRIVATE_UNIT_DIR, 0755) >= 0); assert_se(mount_nofollow_verbose(LOG_DEBUG, "tmpfs", PRIVATE_UNIT_DIR, "tmpfs", MS_NOSUID|MS_NODEV, NULL) >= 0); + /* Mark our test "playground" as MS_SLAVE, so we can MS_MOVE mounts underneath it. */ + assert_se(mount_nofollow_verbose(LOG_DEBUG, NULL, PRIVATE_UNIT_DIR, NULL, MS_SLAVE, NULL) >= 0); /* Copy unit files to make them accessible even when unprivileged. */ assert_se(get_testdata_dir("test-execute/", &unit_dir) >= 0); assert_se(copy_directory_at(AT_FDCWD, unit_dir, AT_FDCWD, PRIVATE_UNIT_DIR, COPY_MERGE_EMPTY) >= 0); /* Mount tmpfs on the following directories to make not StateDirectory= or friends disturb the host. */ + ret = get_build_exec_dir(&build_dir); + assert_se(ret >= 0 || ret == -ENOEXEC); + + if (build_dir) { + /* Account for a build directory being in one of the soon-to-be-tmpfs directories. If we + * overmount it with an empty tmpfs, manager_new() will pin the wrong systemd-executor binary, + * which can then lead to unexpected (and painful to debug) test fails. */ + assert_se(access(build_dir, F_OK) >= 0); + assert_se(build_dir_mount = path_join(PRIVATE_UNIT_DIR, "build_dir")); + assert_se(mkdir_p(build_dir_mount, 0755) >= 0); + assert_se(mount_nofollow_verbose(LOG_DEBUG, build_dir, build_dir_mount, NULL, MS_BIND, NULL) >= 0); + } + FOREACH_STRING(p, "/dev/shm", "/root", "/tmp", "/var/tmp", "/var/lib") assert_se(mount_nofollow_verbose(LOG_DEBUG, "tmpfs", p, "tmpfs", MS_NOSUID|MS_NODEV, NULL) >= 0); + if (build_dir_mount) { + ret = RET_NERRNO(access(build_dir, F_OK)); + assert_se(ret >= 0 || ret == -ENOENT); + + if (ret == -ENOENT) { + /* The build directory got overmounted by tmpfs, so let's use the "backup" bind mount to + * bring it back. */ + assert_se(mkdir_p(build_dir, 0755) >= 0); + assert_se(mount_nofollow_verbose(LOG_DEBUG, build_dir_mount, build_dir, NULL, MS_MOVE, NULL) >= 0); + } + } + /* Prepare credstore like tmpfiles.d/credstore.conf for LoadCredential= tests. */ FOREACH_STRING(p, "/run/credstore", "/run/credstore.encrypted") { assert_se(mkdir_p(p, 0) >= 0); From 1d07188b159878fc025cbd81bcaad3ba333bbb1c Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Fri, 5 Apr 2024 19:33:28 +0200 Subject: [PATCH 4/4] base-filesystem: check for __s390x__ first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On s390x both __s390__ and __s390x__ are defined, and with the original order we'd go through the __s390__ branch and emit a warning: [169/2118] Compiling C object src/shared/libsystemd-shared-256.a.p/base-filesystem.c.o ../src/shared/base-filesystem.c:136:11: note: ‘#pragma message: Please add an entry above specifying whether your architecture uses /lib64/, /lib32/, or no such links.’ 136 | # pragma message "Please add an entry above specifying whether your architecture uses /lib64/, /lib32/, or no such links." | ^~~~~~~ --- src/shared/base-filesystem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/base-filesystem.c b/src/shared/base-filesystem.c index 569ef466c3..a4e2dae245 100644 --- a/src/shared/base-filesystem.c +++ b/src/shared/base-filesystem.c @@ -120,13 +120,13 @@ static const BaseFilesystem table[] = { # else # error "Unknown RISC-V ABI" # endif -#elif defined(__s390__) - /* s390-linux-gnu */ #elif defined(__s390x__) { "lib64", 0, "usr/lib/"LIB_ARCH_TUPLE"\0" "usr/lib64\0" "usr/lib\0", "ld-lsb-s390x.so.3" }, # define KNOW_LIB64_DIRS 1 +#elif defined(__s390__) + /* s390-linux-gnu */ #elif defined(__sparc__) #endif /* gcc doesn't allow pragma to be used within constructs, hence log about this separately below */