From 5825205636446f7542c0de116b1d254f0a2c4358 Mon Sep 17 00:00:00 2001 From: Valentin David Date: Mon, 15 Nov 2021 18:57:40 +0100 Subject: [PATCH] Use rename of directories instead of symbolic links in boot partition. This uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. This is working with SystemD boot on EFI using boot loader specifications. There is still the issue of losing `/loader/loader.conf` with SystemD boot. --- src/libostree/ostree-sysroot-cleanup.c | 9 +++ src/libostree/ostree-sysroot-deploy.c | 95 +++++++++++++++++----- src/libostree/ostree-sysroot.c | 106 ++++++++++++++++--------- tests/admin-test.sh | 24 ++++-- 4 files changed, 166 insertions(+), 68 deletions(-) diff --git a/src/libostree/ostree-sysroot-cleanup.c b/src/libostree/ostree-sysroot-cleanup.c index c22a6851ed..5b05551154 100644 --- a/src/libostree/ostree-sysroot-cleanup.c +++ b/src/libostree/ostree-sysroot-cleanup.c @@ -216,6 +216,15 @@ cleanup_other_bootversions (OstreeSysroot *self, const int cleanup_subbootversion = self->subbootversion == 0 ? 1 : 0; /* Reusable buffer for path */ g_autoptr(GString) buf = g_string_new (""); + struct stat stbuf; + + if ((glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW, error)) + && (!S_ISLNK (stbuf.st_mode))) + { + g_string_truncate (buf, 0); g_string_append_printf (buf, "boot/loader.%d", self->bootversion); + if (!glnx_shutil_rm_rf_at (self->sysroot_fd, buf->str, cancellable, error)) + return FALSE; + } /* These directories are for the other major version */ g_string_truncate (buf, 0); g_string_append_printf (buf, "boot/loader.%d", cleanup_bootversion); diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c index a8bf9f4485..85a3394d8e 100644 --- a/src/libostree/ostree-sysroot-deploy.c +++ b/src/libostree/ostree-sysroot-deploy.c @@ -2026,18 +2026,18 @@ install_deployment_kernel (OstreeSysroot *sysroot, return TRUE; } -/* We generate the symlink on disk, then potentially do a syncfs() to ensure +/* We generate the directory on disk, then potentially do a syncfs() to ensure * that it (and everything else we wrote) has hit disk. Only after that do we * rename it into place. */ static gboolean -prepare_new_bootloader_link (OstreeSysroot *sysroot, - int current_bootversion, - int new_bootversion, - GCancellable *cancellable, - GError **error) +prepare_new_bootloader_dir (OstreeSysroot *sysroot, + int current_bootversion, + int new_bootversion, + GCancellable *cancellable, + GError **error) { - GLNX_AUTO_PREFIX_ERROR ("Preparing final bootloader swap", error); + GLNX_AUTO_PREFIX_ERROR ("Preparing new bootloader directory", error); g_assert ((current_bootversion == 0 && new_bootversion == 1) || (current_bootversion == 1 && new_bootversion == 0)); @@ -2047,23 +2047,76 @@ prepare_new_bootloader_link (OstreeSysroot *sysroot, if (errno != EEXIST) return glnx_throw_errno_prefix (error, "symlinkat"); - g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion); + if (!_ostree_sysroot_ensure_boot_fd (sysroot, error)) + return FALSE; - /* We shouldn't actually need to replace but it's easier to reuse - that code */ - if (!symlink_at_replace (new_target, sysroot->sysroot_fd, "boot/loader.tmp", - cancellable, error)) + g_autofree char *loader_dir_name = g_strdup_printf ("loader.%d", new_bootversion); + + if (!glnx_shutil_mkdir_p_at (sysroot->boot_fd, loader_dir_name, 0755, + cancellable, error)) + return FALSE; + + g_autofree char *version_name = g_strdup_printf ("%s/version", loader_dir_name); + + if (!glnx_file_replace_contents_at (sysroot->boot_fd, version_name, + (guint8*)loader_dir_name, strlen(loader_dir_name), + 0, cancellable, error)) return FALSE; return TRUE; } +static gboolean +renameat2_exchange (int olddirfd, + const char *oldpath, + int newdirfd, + const char *newpath, + gboolean *is_atomic, + GError **error) +{ + if (renameat2 (olddirfd, oldpath, newdirfd, newpath, RENAME_EXCHANGE) == 0) + return TRUE; + else + { + if ((errno == EINVAL) + || (errno == ENOSYS)) + { + if (glnx_renameat2_exchange (olddirfd, oldpath, newdirfd, newpath) == 0) + { + is_atomic = FALSE; + return TRUE; + } + } + } + + if (errno != ENOENT) + return glnx_throw_errno_prefix (error, "renameat2"); + + if (renameat2 (olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE) == 0) + return TRUE; + else + { + if ((errno == EINVAL) + || (errno == ENOSYS)) + { + if (glnx_renameat2_noreplace (olddirfd, oldpath, newdirfd, newpath) == 0) + { + is_atomic = FALSE; + return TRUE; + } + } + } + + return glnx_throw_errno_prefix (error, "renameat2"); +} + /* Update the /boot/loader symlink to point to /boot/loader.$new_bootversion */ static gboolean swap_bootloader (OstreeSysroot *sysroot, OstreeBootloader *bootloader, int current_bootversion, int new_bootversion, + gboolean *is_atomic, GCancellable *cancellable, GError **error) { @@ -2075,11 +2128,8 @@ swap_bootloader (OstreeSysroot *sysroot, if (!_ostree_sysroot_ensure_boot_fd (sysroot, error)) return FALSE; - /* The symlink was already written, and we used syncfs() to ensure - * its data is in place. Renaming now should give us atomic semantics; - * see https://bugzilla.gnome.org/show_bug.cgi?id=755595 - */ - if (!glnx_renameat (sysroot->boot_fd, "loader.tmp", sysroot->boot_fd, "loader", error)) + g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion); + if (!renameat2_exchange(sysroot->boot_fd, new_target, sysroot->boot_fd, "loader", is_atomic, error)) return FALSE; /* Now we explicitly fsync this directory, even though it @@ -2254,6 +2304,7 @@ write_deployments_bootswap (OstreeSysroot *self, OstreeBootloader *bootloader, SyncStats *out_syncstats, char **out_subbootdir, + gboolean *is_atomic, GCancellable *cancellable, GError **error) { @@ -2317,15 +2368,16 @@ write_deployments_bootswap (OstreeSysroot *self, return glnx_prefix_error (error, "Bootloader write config"); } - if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, - cancellable, error)) + if (!prepare_new_bootloader_dir (self, self->bootversion, new_bootversion, + cancellable, error)) return FALSE; if (!full_system_sync (self, out_syncstats, cancellable, error)) return FALSE; if (!swap_bootloader (self, bootloader, self->bootversion, new_bootversion, - cancellable, error)) + is_atomic, cancellable, error)) + return FALSE; if (out_subbootdir) @@ -2534,7 +2586,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader); if (!write_deployments_bootswap (self, new_deployments, opts, bootloader, - &syncstats, &new_subbootdir, cancellable, error)) + &syncstats, &new_subbootdir, &bootloader_is_atomic, + cancellable, error)) return FALSE; } diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index 04432cbcf1..8dd8ab8bf0 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -557,6 +557,62 @@ compare_loader_configs_for_sorting (gconstpointer a_pp, return compare_boot_loader_configs (a, b); } +/* Get the bootversion from the `/boot/loader` directory or symlink. */ +static gboolean +read_current_bootversion (OstreeSysroot *self, + int *out_bootversion, + GCancellable *cancellable, + GError **error) +{ + int ret_bootversion; + struct stat stbuf; + + if (!glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW, error)) + return FALSE; + if (errno == ENOENT) + { + g_debug ("Didn't find $sysroot/boot/loader directory or symlink; assuming bootversion 0"); + ret_bootversion = 0; + } + else + { + if (!S_ISLNK (stbuf.st_mode)) + { + gsize len; + g_autofree char* version_content = glnx_file_get_contents_utf8_at(self->sysroot_fd, "boot/loader/version", + &len, cancellable, error); + if (version_content == NULL) { + return FALSE; + } + if (len != 8) + return glnx_throw (error, "Invalid version in boot/loader/version"); + else if (g_strcmp0 (version_content, "loader.0") == 0) + ret_bootversion = 0; + else if (g_strcmp0 (version_content, "loader.1") == 0) + ret_bootversion = 1; + else + return glnx_throw (error, "Invalid version in boot/loader/version"); + } + else + { + /* Backward compatibility with boot symbolic links */ + g_autofree char *target = + glnx_readlinkat_malloc (self->sysroot_fd, "boot/loader", cancellable, error); + if (!target) + return FALSE; + if (g_strcmp0 (target, "loader.0") == 0) + ret_bootversion = 0; + else if (g_strcmp0 (target, "loader.1") == 0) + ret_bootversion = 1; + else + return glnx_throw (error, "Invalid target '%s' in boot/loader", target); + } + } + + *out_bootversion = ret_bootversion; + return TRUE; +} + /* Read all the bootconfigs from `/boot/loader/`. */ gboolean _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self, @@ -571,12 +627,22 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self, g_autoptr(GPtrArray) ret_loader_configs = g_ptr_array_new_with_free_func ((GDestroyNotify)g_object_unref); - g_autofree char *entries_path = g_strdup_printf ("boot/loader.%d/entries", bootversion); + g_autofree char *entries_path = NULL; + int current_version; + if (!read_current_bootversion (self, ¤t_version, cancellable, error)) + return FALSE; + + if (current_version == bootversion) + entries_path = g_strdup ("boot/loader/entries"); + else + entries_path = g_strdup_printf ("boot/loader.%d/entries", bootversion); + gboolean entries_exists; g_auto(GLnxDirFdIterator) dfd_iter = { 0, }; if (!ot_dfd_iter_init_allow_noent (self->sysroot_fd, entries_path, &dfd_iter, &entries_exists, error)) return FALSE; + if (!entries_exists) { /* Note early return */ @@ -616,44 +682,6 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self, return TRUE; } -/* Get the bootversion from the `/boot/loader` symlink. */ -static gboolean -read_current_bootversion (OstreeSysroot *self, - int *out_bootversion, - GCancellable *cancellable, - GError **error) -{ - int ret_bootversion; - struct stat stbuf; - - if (!glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW, error)) - return FALSE; - if (errno == ENOENT) - { - g_debug ("Didn't find $sysroot/boot/loader symlink; assuming bootversion 0"); - ret_bootversion = 0; - } - else - { - if (!S_ISLNK (stbuf.st_mode)) - return glnx_throw (error, "Not a symbolic link: boot/loader"); - - g_autofree char *target = - glnx_readlinkat_malloc (self->sysroot_fd, "boot/loader", cancellable, error); - if (!target) - return FALSE; - if (g_strcmp0 (target, "loader.0") == 0) - ret_bootversion = 0; - else if (g_strcmp0 (target, "loader.1") == 0) - ret_bootversion = 1; - else - return glnx_throw (error, "Invalid target '%s' in boot/loader", target); - } - - *out_bootversion = ret_bootversion; - return TRUE; -} - static gboolean load_origin (OstreeSysroot *self, OstreeDeployment *deployment, diff --git a/tests/admin-test.sh b/tests/admin-test.sh index dd5ffd14b2..009f7243fc 100644 --- a/tests/admin-test.sh +++ b/tests/admin-test.sh @@ -80,7 +80,8 @@ echo "ok --print-current-dir" # Test layout of bootloader config and refs assert_not_has_dir sysroot/boot/loader.0 -assert_has_dir sysroot/boot/loader.1 +assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.1' assert_has_dir sysroot/ostree/boot.1.1 assert_has_file sysroot/boot/loader/entries/ostree-1-testos.conf assert_file_has_content sysroot/boot/loader/entries/ostree-1-testos.conf 'options.* root=LABEL=MOO' @@ -103,8 +104,9 @@ ${CMD_PREFIX} ostree admin deploy --os=testos testos:testos/buildmain/x86_64-run new_mtime=$(stat -c '%.Y' sysroot/ostree/deploy) assert_not_streq "${orig_mtime}" "${new_mtime}" # Need a new bootversion, sine we now have two deployments -assert_has_dir sysroot/boot/loader.0 +assert_not_has_dir sysroot/boot/loader.0 assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.0' assert_has_dir sysroot/ostree/boot.0.1 assert_not_has_dir sysroot/ostree/boot.0.0 assert_not_has_dir sysroot/ostree/boot.1.0 @@ -121,8 +123,9 @@ echo "ok second deploy" ${CMD_PREFIX} ostree admin deploy --os=testos testos:testos/buildmain/x86_64-runtime # Keep the same bootversion -assert_has_dir sysroot/boot/loader.0 +assert_not_has_dir sysroot/boot/loader.0 assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.0' # But swap subbootversion assert_has_dir sysroot/ostree/boot.0.0 assert_not_has_dir sysroot/ostree/boot.0.1 @@ -136,7 +139,8 @@ ${CMD_PREFIX} ostree admin os-init otheros ${CMD_PREFIX} ostree admin deploy --os=otheros testos/buildmain/x86_64-runtime assert_not_has_dir sysroot/boot/loader.0 -assert_has_dir sysroot/boot/loader.1 +assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.1' assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf assert_has_file sysroot/boot/loader/entries/ostree-3-otheros.conf assert_file_has_content sysroot/ostree/deploy/testos/deploy/${rev}.1/etc/os-release 'NAME=TestOS' @@ -148,8 +152,9 @@ validate_bootloader echo "ok independent deploy" ${CMD_PREFIX} ostree admin deploy --retain --os=testos testos:testos/buildmain/x86_64-runtime -assert_has_dir sysroot/boot/loader.0 +assert_not_has_dir sysroot/boot/loader.0 assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.0' assert_has_file sysroot/boot/loader/entries/ostree-4-testos.conf assert_file_has_content sysroot/ostree/deploy/testos/deploy/${rev}.2/etc/os-release 'NAME=TestOS' assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf @@ -166,7 +171,8 @@ rm sysroot/ostree/deploy/testos/deploy/${rev}.3/etc/aconfigfile ln -s /ENOENT sysroot/ostree/deploy/testos/deploy/${rev}.3/etc/a-new-broken-symlink ${CMD_PREFIX} ostree admin deploy --retain --os=testos testos:testos/buildmain/x86_64-runtime assert_not_has_dir sysroot/boot/loader.0 -assert_has_dir sysroot/boot/loader.1 +assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.1' link=sysroot/ostree/deploy/testos/deploy/${rev}.4/etc/a-new-broken-symlink if ! test -L ${link}; then ls -al ${link} @@ -190,8 +196,9 @@ assert_has_file sysroot/boot/loader/entries/ostree-1-testos.conf assert_not_has_file sysroot/boot/loader/entries/ostree-2-testos.conf assert_not_has_file sysroot/boot/loader/entries/ostree-3-otheros.conf ${CMD_PREFIX} ostree admin deploy --not-as-default --os=otheros testos:testos/buildmain/x86_64-runtime -assert_has_dir sysroot/boot/loader.0 +assert_not_has_dir sysroot/boot/loader.0 assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.0' assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf assert_has_file sysroot/boot/loader/entries/ostree-1-otheros.conf ${CMD_PREFIX} ostree admin status @@ -201,7 +208,8 @@ echo "ok deploy --not-as-default" ${CMD_PREFIX} ostree admin deploy --retain-rollback --os=otheros testos:testos/buildmain/x86_64-runtime assert_not_has_dir sysroot/boot/loader.0 -assert_has_dir sysroot/boot/loader.1 +assert_not_has_dir sysroot/boot/loader.1 +assert_file_has_content sysroot/boot/loader/version 'loader.1' assert_has_file sysroot/boot/loader/entries/ostree-3-otheros.conf assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf assert_has_file sysroot/boot/loader/entries/ostree-1-otheros.conf