From 5c831ddec801d653014b4eea820a1d6afbb91a63 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 5 Dec 2023 14:57:13 +0900 Subject: [PATCH 1/8] find-esp: do not fail when /boot on btrfs RAID on searching ESP or xbootldr When /boot or friends is on btrfs RAID, btrfs_get_block_device_at() will succeed with 0 and provide zero devnum. Then, - if we are previleged, devname_from_devnum() maps the devnum to /run/systemd/inaccessible/blk, and the subsequent verification by blkid will fail, - if we are unprevileged, sd_device_new_from_devnum() will fail. This makes - when find_esp() or find_xbootldr() is called without any paths, that is, called with the searching mode, then returns -ENOKEY, which should be handled gracefully by the caller, - when they are called with an input path, then they provide the proper error message and suggestion. Fixes RHBZ#2251262 (https://bugzilla.redhat.com/show_bug.cgi?id=2251262). --- src/shared/find-esp.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/shared/find-esp.c b/src/shared/find-esp.c index 9b8c7f73bcd..ce1069e9432 100644 --- a/src/shared/find-esp.c +++ b/src/shared/find-esp.c @@ -396,6 +396,11 @@ static int verify_esp( if (relax_checks) goto finish; + if (devnum_is_zero(devid)) + return log_full_errno(searching ? LOG_DEBUG : LOG_ERR, + SYNTHETIC_ERRNO(searching ? EADDRNOTAVAIL : ENODEV), + "Could not determine backing block device of directory \"%s\" (btrfs RAID?).", p); + /* If we are unprivileged we ask udev for the metadata about the partition. If we are privileged we * use blkid instead. Why? Because this code is called from 'bootctl' which is pretty much an * emergency recovery tool that should also work when udev isn't up (i.e. from the emergency shell), @@ -767,6 +772,15 @@ static int verify_xbootldr( if (relax_checks) goto finish; + if (devnum_is_zero(devid)) + return log_full_errno(searching ? LOG_DEBUG : LOG_ERR, + SYNTHETIC_ERRNO(searching ? EADDRNOTAVAIL : ENODEV), + "Could not determine backing block device of directory \"%s\" (btrfs RAID?).%s", + p, + searching ? "" : + "\nHint: set $SYSTEMD_RELAX_XBOOTLDR_CHECKS=yes environment variable " + "to bypass this and further verifications for the directory."); + if (unprivileged_mode) r = verify_xbootldr_udev(devid, flags, ret_uuid); else From 9bbd3c699c3789b1c6e1a0626bb1aa42740cd28b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Dec 2023 11:55:17 +0900 Subject: [PATCH 2/8] find-esp: introduce verify_esp_flags_init() helper function And split VERIFY_ESP_RELAX_CHECKS into two. No functional change, just refactoring. --- src/shared/find-esp.c | 64 ++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/src/shared/find-esp.c b/src/shared/find-esp.c index ce1069e9432..7fcdd32df54 100644 --- a/src/shared/find-esp.c +++ b/src/shared/find-esp.c @@ -27,9 +27,29 @@ typedef enum VerifyESPFlags { VERIFY_ESP_SEARCHING = 1 << 0, /* Downgrade various "not found" logs to debug level */ VERIFY_ESP_UNPRIVILEGED_MODE = 1 << 1, /* Call into udev rather than blkid */ - VERIFY_ESP_RELAX_CHECKS = 1 << 2, /* Do not validate ESP partition */ + VERIFY_ESP_SKIP_FSTYPE_CHECK = 1 << 2, /* Skip filesystem check */ + VERIFY_ESP_SKIP_DEVICE_CHECK = 1 << 3, /* Skip device node check */ } VerifyESPFlags; +static VerifyESPFlags verify_esp_flags_init(int unprivileged_mode, const char *env_name_for_relaxing) { + VerifyESPFlags flags = 0; + + assert(env_name_for_relaxing); + + if (unprivileged_mode < 0) + unprivileged_mode = geteuid() != 0; + if (unprivileged_mode) + flags |= VERIFY_ESP_UNPRIVILEGED_MODE; + + if (getenv_bool(env_name_for_relaxing) > 0) + flags |= VERIFY_ESP_SKIP_FSTYPE_CHECK | VERIFY_ESP_SKIP_DEVICE_CHECK; + + if (detect_container() > 0) + flags |= VERIFY_ESP_SKIP_DEVICE_CHECK; + + return flags; +} + static int verify_esp_blkid( dev_t devid, VerifyESPFlags flags, @@ -326,8 +346,8 @@ static int verify_esp( dev_t *ret_devid, VerifyESPFlags flags) { - bool relax_checks, searching = FLAGS_SET(flags, VERIFY_ESP_SEARCHING), - unprivileged_mode = FLAGS_SET(flags, VERIFY_ESP_UNPRIVILEGED_MODE); + bool searching = FLAGS_SET(flags, VERIFY_ESP_SEARCHING), + unprivileged_mode = FLAGS_SET(flags, VERIFY_ESP_UNPRIVILEGED_MODE); _cleanup_free_ char *p = NULL; _cleanup_close_ int pfd = -EBADF; dev_t devid = 0; @@ -343,10 +363,6 @@ static int verify_esp( * -EACESS → if 'unprivileged_mode' is set, and we have trouble accessing the thing */ - relax_checks = - getenv_bool("SYSTEMD_RELAX_ESP_CHECKS") > 0 || - FLAGS_SET(flags, VERIFY_ESP_RELAX_CHECKS); - /* Non-root user can only check the status, so if an error occurred in the following, it does not cause any * issues. Let's also, silence the error messages. */ @@ -356,7 +372,7 @@ static int verify_esp( (unprivileged_mode && ERRNO_IS_PRIVILEGE(r)) ? LOG_DEBUG : LOG_ERR, r, "Failed to open parent directory of \"%s\": %m", path); - if (!relax_checks) { + if (!FLAGS_SET(flags, VERIFY_ESP_SKIP_FSTYPE_CHECK)) { _cleanup_free_ char *f = NULL; struct statfs sfs; @@ -383,17 +399,13 @@ static int verify_esp( "File system \"%s\" is not a FAT EFI System Partition (ESP) file system.", p); } - relax_checks = - relax_checks || - detect_container() > 0; - - r = verify_fsroot_dir(pfd, p, flags, relax_checks ? NULL : &devid); + r = verify_fsroot_dir(pfd, p, flags, FLAGS_SET(flags, VERIFY_ESP_SKIP_DEVICE_CHECK) ? NULL : &devid); if (r < 0) return r; /* In a container we don't have access to block devices, skip this part of the verification, we trust * the container manager set everything up correctly on its own. */ - if (relax_checks) + if (FLAGS_SET(flags, VERIFY_ESP_SKIP_DEVICE_CHECK)) goto finish; if (devnum_is_zero(devid)) @@ -459,15 +471,13 @@ int find_esp_and_warn_at( assert(rfd >= 0 || rfd == AT_FDCWD); - if (unprivileged_mode < 0) - unprivileged_mode = geteuid() != 0; - flags = unprivileged_mode > 0 ? VERIFY_ESP_UNPRIVILEGED_MODE : 0; + flags = verify_esp_flags_init(unprivileged_mode, "SYSTEMD_RELAX_ESP_CHECKS"); r = dir_fd_is_root_or_cwd(rfd); if (r < 0) return log_error_errno(r, "Failed to check if directory file descriptor is root: %m"); if (r == 0) - flags |= VERIFY_ESP_RELAX_CHECKS; + flags |= VERIFY_ESP_SKIP_FSTYPE_CHECK | VERIFY_ESP_SKIP_DEVICE_CHECK; if (path) return verify_esp(rfd, path, ret_path, ret_part, ret_pstart, ret_psize, ret_uuid, ret_devid, flags); @@ -747,8 +757,7 @@ static int verify_xbootldr( _cleanup_free_ char *p = NULL; _cleanup_close_ int pfd = -EBADF; bool searching = FLAGS_SET(flags, VERIFY_ESP_SEARCHING), - unprivileged_mode = FLAGS_SET(flags, VERIFY_ESP_UNPRIVILEGED_MODE), - relax_checks; + unprivileged_mode = FLAGS_SET(flags, VERIFY_ESP_UNPRIVILEGED_MODE); dev_t devid = 0; int r; @@ -761,15 +770,11 @@ static int verify_xbootldr( (unprivileged_mode && ERRNO_IS_PRIVILEGE(r)) ? LOG_DEBUG : LOG_ERR, r, "Failed to open parent directory of \"%s\": %m", path); - relax_checks = - getenv_bool("SYSTEMD_RELAX_XBOOTLDR_CHECKS") > 0 || - detect_container() > 0; - - r = verify_fsroot_dir(pfd, p, flags, relax_checks ? NULL : &devid); + r = verify_fsroot_dir(pfd, p, flags, FLAGS_SET(flags, VERIFY_ESP_SKIP_DEVICE_CHECK) ? NULL : &devid); if (r < 0) return r; - if (relax_checks) + if (FLAGS_SET(flags, VERIFY_ESP_SKIP_DEVICE_CHECK)) goto finish; if (devnum_is_zero(devid)) @@ -814,17 +819,14 @@ int find_xbootldr_and_warn_at( sd_id128_t *ret_uuid, dev_t *ret_devid) { - VerifyESPFlags flags = 0; + VerifyESPFlags flags; int r; /* Similar to find_esp_and_warn(), but finds the XBOOTLDR partition. Returns the same errors. */ assert(rfd >= 0 || rfd == AT_FDCWD); - if (unprivileged_mode < 0) - unprivileged_mode = geteuid() != 0; - if (unprivileged_mode) - flags |= VERIFY_ESP_UNPRIVILEGED_MODE; + flags = verify_esp_flags_init(unprivileged_mode, "SYSTEMD_RELAX_XBOOTLDR_CHECKS"); if (path) return verify_xbootldr(rfd, path, flags, ret_path, ret_uuid, ret_devid); From 997ba18af1bcc9982e2ffc622f993727a48960c5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Dec 2023 12:48:44 +0900 Subject: [PATCH 3/8] find-esp: do not skip fstype check even when --root= or --image= is specified The check was introduced by 63105f33edad423691e2d53bf7071f99c83799ba, but there is no reason to skip the check even in such cases. --- src/shared/find-esp.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/shared/find-esp.c b/src/shared/find-esp.c index 7fcdd32df54..bbfd3b175f3 100644 --- a/src/shared/find-esp.c +++ b/src/shared/find-esp.c @@ -473,12 +473,6 @@ int find_esp_and_warn_at( flags = verify_esp_flags_init(unprivileged_mode, "SYSTEMD_RELAX_ESP_CHECKS"); - r = dir_fd_is_root_or_cwd(rfd); - if (r < 0) - return log_error_errno(r, "Failed to check if directory file descriptor is root: %m"); - if (r == 0) - flags |= VERIFY_ESP_SKIP_FSTYPE_CHECK | VERIFY_ESP_SKIP_DEVICE_CHECK; - if (path) return verify_esp(rfd, path, ret_path, ret_part, ret_pstart, ret_psize, ret_uuid, ret_devid, flags); From 5b4fa6f13cf860afa53c71b52e4ceca25f8f13a5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 6 Dec 2023 13:29:36 +0900 Subject: [PATCH 4/8] test: split out host_has_{btrfs,mdadm}() from TEST-64-UDEV-STORAGE --- test/TEST-64-UDEV-STORAGE/test.sh | 4 ++-- test/test-functions | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/TEST-64-UDEV-STORAGE/test.sh b/test/TEST-64-UDEV-STORAGE/test.sh index d41a4f00f9a..b9e7bdf18ad 100755 --- a/test/TEST-64-UDEV-STORAGE/test.sh +++ b/test/TEST-64-UDEV-STORAGE/test.sh @@ -24,7 +24,7 @@ _host_has_feature() {( case "${1:?}" in btrfs) - modprobe -nv btrfs && command -v mkfs.btrfs && command -v btrfs || return $? + host_has_btrfs ;; iscsi) # Client/initiator (Open-iSCSI) @@ -36,7 +36,7 @@ _host_has_feature() {( command -v lvm || return $? ;; mdadm) - command -v mdadm || return $? + host_has_mdadm ;; multipath) command -v multipath && command -v multipathd || return $? diff --git a/test/test-functions b/test/test-functions index 42b00387898..544d8ea34b9 100644 --- a/test/test-functions +++ b/test/test-functions @@ -1190,6 +1190,11 @@ install_lvm() { mkdir -p "${initdir:?}/etc/lvm" } +host_has_btrfs() ( + set -e + modprobe -nv btrfs && command -v mkfs.btrfs && command -v btrfs || return $? +) + install_btrfs() { instmods btrfs # Not all utilities provided by btrfs-progs are listed here; extend the list @@ -1257,6 +1262,11 @@ install_iscsi() { fi } +host_has_mdadm() ( + set -e + command -v mdadm || return $? +) + install_mdadm() { local unit local mdadm_units=( From 4ed943e97bb7ef4227837f7a6f607cd313cae70b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 6 Dec 2023 13:32:15 +0900 Subject: [PATCH 5/8] test: make install_mdadm() also install relevant kernel modules Installing mdadm without kernel modules is mostly meaningless. --- test/test-functions | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test-functions b/test/test-functions index 544d8ea34b9..b2a83f155bf 100644 --- a/test/test-functions +++ b/test/test-functions @@ -1280,6 +1280,7 @@ install_mdadm() { system-shutdown/mdadm.shutdown ) + instmods "=md" image_install mdadm mdmon inst_rules 01-md-raid-creating.rules 63-md-raid-arrays.rules 64-md-raid-assembly.rules 69-md-clustered-confirm-device.rules # Fedora/CentOS/RHEL ships this rule file From 0f236e8cd6309cdc392d9e62bc545dcd497a9c50 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 14:28:00 +0900 Subject: [PATCH 6/8] test: mask mdmonitor when building image Follow-up for 22e31655f3f9f54d932d4f48b92b36698e701729. --- test/test-functions | 4 ++++ test/units/testsuite-64.sh | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test-functions b/test/test-functions index b2a83f155bf..f613dc215da 100644 --- a/test/test-functions +++ b/test/test-functions @@ -1289,6 +1289,10 @@ install_mdadm() { for unit in "${mdadm_units[@]}"; do image_install "${ROOTLIBDIR:?}/$unit" done + + # Disable the mdmonitor service, since it fails if there's no valid email address + # configured in /etc/mdadm.conf, which just unnecessarily pollutes the logs + "${SYSTEMCTL:?}" mask --root "${initdir:?}" mdmonitor.service || : } install_compiled_systemd() { diff --git a/test/units/testsuite-64.sh b/test/units/testsuite-64.sh index 0e598cc6b3e..81edb0ab7a1 100755 --- a/test/units/testsuite-64.sh +++ b/test/units/testsuite-64.sh @@ -1159,10 +1159,6 @@ testcase_mdadm_lvm() { helper_check_device_units } -# Disable the mdmonitor service, since it fails if there's no valid email address -# configured in /etc/mdadm.conf, which just unnecessarily pollutes the logs -systemctl list-unit-files mdmonitor.service >/dev/null && systemctl mask --runtime mdmonitor.service - udevadm settle udevadm control --log-level debug lsblk -a From 97bbb9cfbd6dffb6409a70311d925a95dae9da3b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 5 Dec 2023 16:31:53 +0900 Subject: [PATCH 7/8] test: create ESP and xbootldr partitions --- test/TEST-24-CRYPTSETUP/test.sh | 6 +++--- test/test-functions | 31 +++++++++++++++++++++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/test/TEST-24-CRYPTSETUP/test.sh b/test/TEST-24-CRYPTSETUP/test.sh index d0ec63d8708..eace3f23c03 100755 --- a/test/TEST-24-CRYPTSETUP/test.sh +++ b/test/TEST-24-CRYPTSETUP/test.sh @@ -27,7 +27,7 @@ check_result_qemu() { mount_initdir - cryptsetup luksOpen "${LOOPDEV:?}p2" "${DM_NAME:?}" <"$TESTDIR/keyfile" + cryptsetup luksOpen "${LOOPDEV:?}p4" "${DM_NAME:?}" <"$TESTDIR/keyfile" mount "/dev/mapper/$DM_NAME" "$initdir/var" check_result_common "${initdir:?}" && ret=0 || ret=$? @@ -43,8 +43,8 @@ test_create_image() { create_empty_image_rootdir echo -n test >"${TESTDIR:?}/keyfile" - cryptsetup -q luksFormat --uuid="$PART_UUID" --pbkdf pbkdf2 --pbkdf-force-iterations 1000 "${LOOPDEV:?}p2" "$TESTDIR/keyfile" - cryptsetup luksOpen "${LOOPDEV}p2" "${DM_NAME:?}" <"$TESTDIR/keyfile" + cryptsetup -q luksFormat --uuid="$PART_UUID" --pbkdf pbkdf2 --pbkdf-force-iterations 1000 "${LOOPDEV:?}p4" "$TESTDIR/keyfile" + cryptsetup luksOpen "${LOOPDEV}p4" "${DM_NAME:?}" <"$TESTDIR/keyfile" mkfs.ext4 -L var "/dev/mapper/$DM_NAME" mkdir -p "${initdir:?}/var" mount "/dev/mapper/$DM_NAME" "$initdir/var" diff --git a/test/test-functions b/test/test-functions index f613dc215da..57dc5d82165 100644 --- a/test/test-functions +++ b/test/test-functions @@ -1598,6 +1598,9 @@ create_empty_image() { # Partition sizes are in MiBs local root_size=768 local data_size=100 + local esp_size=128 + local boot_size=128 + local total= if ! get_bool "$NO_BUILD"; then if meson configure "${BUILD_DIR:?}" | grep 'static-lib\|standalone-binaries' | awk '{ print $2 }' | grep -q 'true'; then root_size=$((root_size + 200)) @@ -1620,28 +1623,44 @@ create_empty_image() { data_size=$((data_size + IMAGE_ADDITIONAL_DATA_SIZE)) fi - echo "Setting up ${IMAGE_PUBLIC:?} (${root_size} MB)" + total=$((root_size + data_size + esp_size + boot_size)) + + echo "Setting up ${IMAGE_PUBLIC:?} (${total} MB)" rm -f "${IMAGE_PRIVATE:?}" "$IMAGE_PUBLIC" # Create the blank file to use as a root filesystem - truncate -s "${root_size}M" "$IMAGE_PUBLIC" + truncate -s "${total}M" "$IMAGE_PUBLIC" LOOPDEV="$(losetup --show -P -f "$IMAGE_PUBLIC")" [[ -b "$LOOPDEV" ]] || return 1 # Create two partitions - a root one and a data one (utilized by some tests) sfdisk "$LOOPDEV" < Date: Tue, 5 Dec 2023 23:18:17 +0900 Subject: [PATCH 8/8] test: add basic coverity tests for bootctl --- test/TEST-74-AUX-UTILS/test.sh | 6 + test/units/testsuite-74.bootctl.sh | 261 +++++++++++++++++++++++++++++ 2 files changed, 267 insertions(+) create mode 100755 test/units/testsuite-74.bootctl.sh diff --git a/test/TEST-74-AUX-UTILS/test.sh b/test/TEST-74-AUX-UTILS/test.sh index f033ec469f3..7940600612d 100755 --- a/test/TEST-74-AUX-UTILS/test.sh +++ b/test/TEST-74-AUX-UTILS/test.sh @@ -16,6 +16,12 @@ test_append_files() { # the QEMU test, as nspawn refuses the invalid machine ID with -EUCLEAN printf "556f48e837bc4424a710fa2e2c9d3e3c\ne3d\n" >"$workspace/etc/machine-id" fi + + if host_has_btrfs && host_has_mdadm; then + install_btrfs + install_mdadm + generate_module_dependencies + fi } do_test "$@" diff --git a/test/units/testsuite-74.bootctl.sh b/test/units/testsuite-74.bootctl.sh new file mode 100755 index 00000000000..61373b506e4 --- /dev/null +++ b/test/units/testsuite-74.bootctl.sh @@ -0,0 +1,261 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eux +set -o pipefail + +if systemd-detect-virt --quiet --container; then + echo "running on container, skipping." + exit 0 +fi + +if ! command -v bootctl >/dev/null; then + echo "bootctl not found, skipping." + exit 0 +fi + +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +# shellcheck source=test/units/test-control.sh +. "$(dirname "$0")"/test-control.sh + +basic_tests() { + bootctl "$@" --help + bootctl "$@" --version + + bootctl "$@" install --make-entry-directory=yes + bootctl "$@" remove --make-entry-directory=yes + + bootctl "$@" install --all-architectures + bootctl "$@" remove --all-architectures + + bootctl "$@" install --make-entry-directory=yes --all-architectures + bootctl "$@" remove --make-entry-directory=yes --all-architectures + + bootctl "$@" install + (! bootctl "$@" update) + bootctl "$@" update --graceful + + bootctl "$@" is-installed + bootctl "$@" is-installed --graceful + bootctl "$@" random-seed + + bootctl "$@" + bootctl "$@" status + bootctl "$@" status --quiet + bootctl "$@" list + bootctl "$@" list --quiet + bootctl "$@" list --json=short + bootctl "$@" list --json=pretty + + bootctl "$@" remove + (! bootctl "$@" is-installed) + (! bootctl "$@" is-installed --graceful) +} + +testcase_bootctl_basic() { + assert_eq "$(bootctl --print-esp-path)" "/efi" + assert_eq "$(bootctl --print-boot-path)" "/boot" + bootctl --print-root-device + + basic_tests +} + +cleanup_image() ( + set +e + + if [[ -z "${IMAGE_DIR:-}" ]]; then + return 0 + fi + + umount "${IMAGE_DIR}/root" + + if [[ -n "${LOOPDEV:-}" ]]; then + losetup -d "${LOOPDEV}" + unset LOOPDEV + fi + + udevadm settle + + rm -rf "${IMAGE_DIR}" + unset IMAGE_DIR + + return 0 +) + +testcase_bootctl_image() { + IMAGE_DIR="$(mktemp --directory /tmp/test-bootctl.XXXXXXXXXX)" + trap cleanup_image RETURN + + truncate -s 256m "${IMAGE_DIR}/image" + + cat >"${IMAGE_DIR}/partscript" </dev/null; then + echo "mdadm not found, skipping." + return 0 + fi + + if ! command -v mkfs.btrfs >/dev/null; then + echo "mkfs.btrfs not found, skipping." + return 0 + fi + + IMAGE_DIR="$(mktemp --directory /tmp/test-bootctl.XXXXXXXXXX)" + trap cleanup_raid RETURN + + truncate -s 256m "${IMAGE_DIR}/image1" + truncate -s 256m "${IMAGE_DIR}/image2" + + cat >"${IMAGE_DIR}/partscript" <