From 723705b803480dfd9c4090000ffc69fdafbb5c82 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 12 Oct 2015 21:33:23 -0400 Subject: [PATCH] sysroot: Write symlinks before calling fsync(), then rename after There might be a race here in that we create new symlink files *after* calling `syncfs`, and they are not guaranteed to end up on disk. Rework the code so that we create symlinks before, and then only rename them after (and `fsync()` the directory for good measure). Additional-fixes-by: Giuseppe Scrivano Tested-by: Giuseppe Scrivano This still needs verification that we're fixing a real bug; but I'm fairly confident this won't make the fsync situation worse. https://bugzilla.gnome.org/show_bug.cgi?id=755595 --- src/libostree/ostree-sysroot-deploy.c | 260 +++++++++++++++++++++----- src/libotutil/ot-gio-utils.c | 56 ------ src/libotutil/ot-gio-utils.h | 5 - 3 files changed, 211 insertions(+), 110 deletions(-) diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c index 3aeee1b1..f7afe3d2 100644 --- a/src/libostree/ostree-sysroot-deploy.c +++ b/src/libostree/ostree-sysroot-deploy.c @@ -33,6 +33,53 @@ #define OSTREE_CONFIGMERGE_ID "d3863baec13e4449ab0384684a8af3a7" #define OSTREE_DEPLOYMENT_COMPLETE_ID "dd440e3e549083b63d0efc7dc15255f1" +/* + * Like symlinkat() but overwrites (atomically) an existing + * symlink. + */ +static gboolean +symlink_at_replace (const char *oldpath, + int parent_dfd, + const char *newpath, + GCancellable *cancellable, + GError **error) +{ + gboolean ret = FALSE; + int res; + /* Possibly in the future generate a temporary random name here, + * would need to move "generate a temporary name" code into + * libglnx or glib? + */ + g_autofree char *temppath = g_strconcat (newpath, ".tmp", NULL); + + /* Clean up any stale temporary links */ + (void) unlinkat (parent_dfd, temppath, 0); + + /* Create the temp link */ + do + res = symlinkat (oldpath, parent_dfd, temppath); + while (G_UNLIKELY (res == -1 && errno == EINTR)); + if (res == -1) + { + glnx_set_error_from_errno (error); + goto out; + } + + /* Rename it into place */ + do + res = renameat (parent_dfd, temppath, parent_dfd, newpath); + while (G_UNLIKELY (res == -1 && errno == EINTR)); + if (res == -1) + { + glnx_set_error_from_errno (error); + goto out; + } + + ret = TRUE; + out: + return ret; +} + static gboolean dirfd_copy_attributes_and_xattrs (int src_parent_dfd, const char *src_name, @@ -1046,21 +1093,92 @@ full_system_sync (OstreeSysroot *self, } static gboolean -swap_bootlinks (OstreeSysroot *self, - int bootversion, - GPtrArray *new_deployments, - GCancellable *cancellable, - GError **error) +create_new_bootlinks (OstreeSysroot *self, + int bootversion, + GPtrArray *new_deployments, + GCancellable *cancellable, + GError **error) { gboolean ret = FALSE; guint i; int old_subbootversion; int new_subbootversion; - g_autoptr(GFile) ostree_dir = g_file_get_child (self->path, "ostree"); - g_autofree char *ostree_bootdir_name = g_strdup_printf ("boot.%d", bootversion); - g_autoptr(GFile) ostree_bootdir = g_file_resolve_relative_path (ostree_dir, ostree_bootdir_name); + glnx_fd_close int ostree_dfd = -1; + glnx_fd_close int ostree_subbootdir_dfd = -1; + g_autofree char *ostree_bootdir_name = NULL; g_autofree char *ostree_subbootdir_name = NULL; - g_autoptr(GFile) ostree_subbootdir = NULL; + + if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_dfd, error)) + goto out; + + ostree_bootdir_name = g_strdup_printf ("boot.%d", bootversion); + + if (bootversion != self->bootversion) + { + if (!_ostree_sysroot_read_current_subbootversion (self, bootversion, &old_subbootversion, + cancellable, error)) + goto out; + } + else + old_subbootversion = self->subbootversion; + + new_subbootversion = old_subbootversion == 0 ? 1 : 0; + + /* Create the "subbootdir", which is a directory holding a symlink farm pointing to + * deployments per-osname. + */ + ostree_subbootdir_name = g_strdup_printf ("boot.%d.%d", bootversion, new_subbootversion); + if (!glnx_shutil_rm_rf_at (ostree_dfd, ostree_subbootdir_name, cancellable, error)) + goto out; + if (!glnx_shutil_mkdir_p_at (ostree_dfd, ostree_subbootdir_name, 0755, cancellable, error)) + goto out; + if (!glnx_opendirat (ostree_dfd, ostree_subbootdir_name, FALSE, &ostree_subbootdir_dfd, error)) + goto out; + + for (i = 0; i < new_deployments->len; i++) + { + OstreeDeployment *deployment = new_deployments->pdata[i]; + g_autofree char *bootlink_parent = g_strconcat (ostree_deployment_get_osname (deployment), + "/", + ostree_deployment_get_bootcsum (deployment), + NULL); + g_autofree char *bootlink_pathname = g_strdup_printf ("%s/%d", bootlink_parent, ostree_deployment_get_bootserial (deployment)); + g_autofree char *bootlink_target = g_strdup_printf ("../../../deploy/%s/deploy/%s.%d", + ostree_deployment_get_osname (deployment), + ostree_deployment_get_csum (deployment), + ostree_deployment_get_deployserial (deployment)); + + if (!glnx_shutil_mkdir_p_at (ostree_subbootdir_dfd, bootlink_parent, 0755, cancellable, error)) + goto out; + + if (!symlink_at_replace (bootlink_target, ostree_subbootdir_dfd, bootlink_pathname, + cancellable, error)) + goto out; + } + + ret = TRUE; + out: + return ret; +} + +static gboolean +swap_bootlinks (OstreeSysroot *self, + int bootversion, + GPtrArray *new_deployments, + GCancellable *cancellable, + GError **error) +{ + gboolean ret = FALSE; + int old_subbootversion, new_subbootversion; + glnx_fd_close int ostree_dfd = -1; + glnx_fd_close int ostree_subbootdir_dfd = -1; + g_autofree char *ostree_bootdir_name = NULL; + g_autofree char *ostree_subbootdir_name = NULL; + + if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_dfd, error)) + goto out; + + ostree_bootdir_name = g_strdup_printf ("boot.%d", bootversion); if (bootversion != self->bootversion) { @@ -1074,39 +1192,11 @@ swap_bootlinks (OstreeSysroot *self, new_subbootversion = old_subbootversion == 0 ? 1 : 0; ostree_subbootdir_name = g_strdup_printf ("boot.%d.%d", bootversion, new_subbootversion); - ostree_subbootdir = g_file_resolve_relative_path (ostree_dir, ostree_subbootdir_name); - if (!gs_shutil_rm_rf (ostree_subbootdir, cancellable, error)) + if (!symlink_at_replace (ostree_subbootdir_name, ostree_dfd, ostree_bootdir_name, + cancellable, error)) goto out; - - if (!ot_util_ensure_directory_and_fsync (ostree_subbootdir, cancellable, error)) - goto out; - - for (i = 0; i < new_deployments->len; i++) - { - OstreeDeployment *deployment = new_deployments->pdata[i]; - g_autofree char *bootlink_pathname = g_strdup_printf ("%s/%s/%d", - ostree_deployment_get_osname (deployment), - ostree_deployment_get_bootcsum (deployment), - ostree_deployment_get_bootserial (deployment)); - g_autofree char *bootlink_target = g_strdup_printf ("../../../deploy/%s/deploy/%s.%d", - ostree_deployment_get_osname (deployment), - ostree_deployment_get_csum (deployment), - ostree_deployment_get_deployserial (deployment)); - g_autoptr(GFile) linkname = g_file_get_child (ostree_subbootdir, bootlink_pathname); - g_autoptr(GFile) linkname_parent = g_file_get_parent (linkname); - - if (!ot_util_ensure_directory_and_fsync (linkname_parent, cancellable, error)) - goto out; - - if (!g_file_make_symbolic_link (linkname, bootlink_target, cancellable, error)) - goto out; - } - - if (!ot_gfile_atomic_symlink_swap (ostree_bootdir, ostree_subbootdir_name, - cancellable, error)) - goto out; - + ret = TRUE; out: return ret; @@ -1374,6 +1464,32 @@ install_deployment_kernel (OstreeSysroot *sysroot, return ret; } +static gboolean +prepare_new_bootloader_link (OstreeSysroot *sysroot, + int current_bootversion, + int new_bootversion, + GCancellable *cancellable, + GError **error) +{ + gboolean ret = FALSE; + g_autofree char *new_target = NULL; + + g_assert ((current_bootversion == 0 && new_bootversion == 1) || + (current_bootversion == 1 && new_bootversion == 0)); + + new_target = g_strdup_printf ("loader.%d", new_bootversion); + + /* 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)) + goto out; + + ret = TRUE; + out: + return ret; +} + static gboolean swap_bootloader (OstreeSysroot *sysroot, int current_bootversion, @@ -1382,19 +1498,43 @@ swap_bootloader (OstreeSysroot *sysroot, GError **error) { gboolean ret = FALSE; - g_autoptr(GFile) boot_loader_link = NULL; - g_autofree char *new_target = NULL; + glnx_fd_close int boot_dfd = -1; + int res; g_assert ((current_bootversion == 0 && new_bootversion == 1) || (current_bootversion == 1 && new_bootversion == 0)); - boot_loader_link = g_file_resolve_relative_path (sysroot->path, "boot/loader"); - new_target = g_strdup_printf ("loader.%d", new_bootversion); - - if (!ot_gfile_atomic_symlink_swap (boot_loader_link, new_target, - cancellable, error)) + if (!glnx_opendirat (sysroot->sysroot_fd, "boot", TRUE, &boot_dfd, error)) goto out; + /* 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 + */ + do + res = renameat (boot_dfd, "loader.tmp", boot_dfd, "loader"); + while (G_UNLIKELY (res == -1 && errno == EINTR)); + if (res == -1) + { + glnx_set_error_from_errno (error); + goto out; + } + + /* Now we explicitly fsync this directory, even though it + * isn't required for atomicity, for two reasons: + * - It should be very cheap as we're just syncing whatever + * data was written since the last sync which was hopefully + * less than a second ago. + * - It should be sync'd before shutdown as that could crash + * for whatever reason, and we wouldn't want to confuse the + * admin by going back to the previous session. + */ + if (fsync (boot_dfd) != 0) + { + glnx_set_error_from_errno (error); + goto out; + } + ret = TRUE; out: return ret; @@ -1565,6 +1705,14 @@ ostree_sysroot_write_deployments (OstreeSysroot *self, if (!requires_new_bootversion) { + if (!create_new_bootlinks (self, self->bootversion, + new_deployments, + cancellable, error)) + { + g_prefix_error (error, "Creating new current bootlinks: "); + goto out; + } + if (!full_system_sync (self, cancellable, error)) { g_prefix_error (error, "Full sync: "); @@ -1633,11 +1781,18 @@ ostree_sysroot_write_deployments (OstreeSysroot *self, } } - /* Swap bootlinks for *new* version */ + /* Create and swap bootlinks for *new* version */ + if (!create_new_bootlinks (self, new_bootversion, + new_deployments, + cancellable, error)) + { + g_prefix_error (error, "Creating new version bootlinks: "); + goto out; + } if (!swap_bootlinks (self, new_bootversion, new_deployments, cancellable, error)) { - g_prefix_error (error, "Generating new bootlinks: "); + g_prefix_error (error, "Swapping new version bootlinks: "); goto out; } @@ -1657,6 +1812,13 @@ ostree_sysroot_write_deployments (OstreeSysroot *self, } } + if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, + cancellable, error)) + { + g_prefix_error (error, "Preparing final bootloader swap: "); + goto out; + } + if (!full_system_sync (self, cancellable, error)) { g_prefix_error (error, "Full sync: "); diff --git a/src/libotutil/ot-gio-utils.c b/src/libotutil/ot-gio-utils.c index 1143ca90..b10c04c5 100644 --- a/src/libotutil/ot-gio-utils.c +++ b/src/libotutil/ot-gio-utils.c @@ -508,59 +508,3 @@ ot_util_ensure_directory_and_fsync (GFile *dir, (void) close (parentfd); return ret; } - -/** - * ot_gfile_atomic_symlink_swap: - * @path: Replace the contents of this symbolic link - * @target: New symbolic link target - * @cancellable: - * @error - * - * Create a new temporary symbolic link, then use the Unix rename() - * function to atomically replace @path with the new symbolic link. - * Do not use this function inside directories such as /tmp as it uses - * a predicatable file name. - */ -gboolean -ot_gfile_atomic_symlink_swap (GFile *path, - const char *target, - GCancellable *cancellable, - GError **error) -{ - gboolean ret = FALSE; - g_autoptr(GFile) parent = g_file_get_parent (path); - g_autofree char *tmpname = g_strconcat (gs_file_get_basename_cached (path), ".tmp", NULL); - g_autoptr(GFile) tmppath = g_file_get_child (parent, tmpname); - int parent_dfd = -1; - - if (!ot_gfile_ensure_unlinked (tmppath, cancellable, error)) - goto out; - - if (!g_file_make_symbolic_link (tmppath, target, cancellable, error)) - goto out; - - if (!gs_file_open_dir_fd (parent, &parent_dfd, cancellable, error)) - goto out; - - /* Ensure the link has hit disk */ - if (fsync (parent_dfd) != 0) - { - gs_set_error_from_errno (error, errno); - goto out; - } - - if (!gs_file_rename (tmppath, path, cancellable, error)) - goto out; - - /* And sync again for good measure */ - if (fsync (parent_dfd) != 0) - { - gs_set_error_from_errno (error, errno); - goto out; - } - - ret = TRUE; - out: - if (parent_dfd != -1) (void) close (parent_dfd); - return ret; -} diff --git a/src/libotutil/ot-gio-utils.h b/src/libotutil/ot-gio-utils.h index bd0ef5ca..355073af 100644 --- a/src/libotutil/ot-gio-utils.h +++ b/src/libotutil/ot-gio-utils.h @@ -85,11 +85,6 @@ gboolean ot_gfile_ensure_unlinked (GFile *path, GCancellable *cancellable, GError **error); -gboolean ot_gfile_atomic_symlink_swap (GFile *path, - const char *target, - GCancellable *cancellable, - GError **error); - gboolean ot_util_ensure_directory_and_fsync (GFile *dir, GCancellable *cancellable, GError **error);