From 10556a95b4820d713ca440015be9cb2aca6711e2 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 9 Jan 2021 20:42:43 +0000 Subject: main: Unconditionally set up mount namespace I was being very conservative initially here, but I think it's really safe to just unconditionally set up the mount namespace. This avoids having to check twice for a read-only `/sysroot` (once in the binary and once in the library). --- src/ostree/ot-main.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/src/ostree/ot-main.c b/src/ostree/ot-main.c index bffa40c4..d153dcec 100644 --- a/src/ostree/ot-main.c +++ b/src/ostree/ot-main.c @@ -122,26 +122,10 @@ maybe_setup_mount_namespace (gboolean *out_ns, if (errno == ENOENT) return TRUE; - glnx_autofd int sysroot_subdir_fd = glnx_opendirat_with_errno (AT_FDCWD, "/sysroot", TRUE); - if (sysroot_subdir_fd < 0) - { - if (errno != ENOENT) - return glnx_throw_errno_prefix (error, "opendirat"); - /* No /sysroot - nothing to do */ - return TRUE; - } - - struct statvfs stvfs; - if (fstatvfs (sysroot_subdir_fd, &stvfs) < 0) - return glnx_throw_errno_prefix (error, "fstatvfs"); - if (stvfs.f_flag & ST_RDONLY) - { - if (unshare (CLONE_NEWNS) < 0) - return glnx_throw_errno_prefix (error, "preparing writable sysroot: unshare (CLONE_NEWNS)"); - - *out_ns = TRUE; - } + if (unshare (CLONE_NEWNS) < 0) + return glnx_throw_errno_prefix (error, "setting up mount namespace: unshare(CLONE_NEWNS)"); + *out_ns = TRUE; return TRUE; } -- cgit v1.2.1 From a1c0cffeb38fb218d9b0eac50e319fa1c79584c1 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 9 Jan 2021 20:40:55 +0000 Subject: sysroot: Also maintain canonical boot_fd Just like we hold a fd for `/sysroot`, also do so for `/boot` instead of opening and closing it in a few places. This is a preparatory cleanup for further work. --- src/libostree/ostree-sysroot-deploy.c | 32 +++++++++++++++----------------- src/libostree/ostree-sysroot-private.h | 4 ++++ src/libostree/ostree-sysroot.c | 15 +++++++++++++++ 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c index 900efe49..ad9b56d1 100644 --- a/src/libostree/ostree-sysroot-deploy.c +++ b/src/libostree/ostree-sysroot-deploy.c @@ -1582,11 +1582,11 @@ full_system_sync (OstreeSysroot *self, out_stats->root_syncfs_msec = (end_msec - start_msec); - start_msec = g_get_monotonic_time () / 1000; - glnx_autofd int boot_dfd = -1; - if (!glnx_opendirat (self->sysroot_fd, "boot", TRUE, &boot_dfd, error)) + if (!_ostree_sysroot_ensure_boot_fd (self, error)) return FALSE; - if (!fsfreeze_thaw_cycle (self, boot_dfd, cancellable, error)) + + start_msec = g_get_monotonic_time () / 1000; + if (!fsfreeze_thaw_cycle (self, self->boot_fd, cancellable, error)) return FALSE; end_msec = g_get_monotonic_time () / 1000; out_stats->boot_syncfs_msec = (end_msec - start_msec); @@ -1770,8 +1770,7 @@ install_deployment_kernel (OstreeSysroot *sysroot, cancellable, error)) return FALSE; - glnx_autofd int boot_dfd = -1; - if (!glnx_opendirat (sysroot->sysroot_fd, "boot", TRUE, &boot_dfd, error)) + if (!_ostree_sysroot_ensure_boot_fd (sysroot, error)) return FALSE; const char *osname = ostree_deployment_get_osname (deployment); @@ -1781,14 +1780,14 @@ install_deployment_kernel (OstreeSysroot *sysroot, g_autofree char *bootconf_name = g_strdup_printf ("ostree-%d-%s.conf", n_deployments - ostree_deployment_get_index (deployment), osname); - if (!glnx_shutil_mkdir_p_at (boot_dfd, bootcsumdir, 0775, cancellable, error)) + if (!glnx_shutil_mkdir_p_at (sysroot->boot_fd, bootcsumdir, 0775, cancellable, error)) return FALSE; glnx_autofd int bootcsum_dfd = -1; - if (!glnx_opendirat (boot_dfd, bootcsumdir, TRUE, &bootcsum_dfd, error)) + if (!glnx_opendirat (sysroot->boot_fd, bootcsumdir, TRUE, &bootcsum_dfd, error)) return FALSE; - if (!glnx_shutil_mkdir_p_at (boot_dfd, bootconfdir, 0775, cancellable, error)) + if (!glnx_shutil_mkdir_p_at (sysroot->boot_fd, bootconfdir, 0775, cancellable, error)) return FALSE; /* Install (hardlink/copy) the kernel into /boot/ostree/osname-${bootcsum} if @@ -1879,18 +1878,18 @@ install_deployment_kernel (OstreeSysroot *sysroot, { overlay_initrds = g_ptr_array_new_with_free_func (g_free); - if (!glnx_shutil_mkdir_p_at (boot_dfd, _OSTREE_SYSROOT_BOOT_INITRAMFS_OVERLAYS, + if (!glnx_shutil_mkdir_p_at (sysroot->boot_fd, _OSTREE_SYSROOT_BOOT_INITRAMFS_OVERLAYS, 0755, cancellable, error)) return FALSE; } - if (!glnx_fstatat_allow_noent (boot_dfd, rel_destpath, NULL, 0, error)) + if (!glnx_fstatat_allow_noent (sysroot->boot_fd, rel_destpath, NULL, 0, error)) return FALSE; if (errno == ENOENT) { g_autofree char *srcpath = g_strdup_printf (_OSTREE_SYSROOT_RUNSTATE_STAGED_INITRDS_DIR "/%s", checksum); - if (!install_into_boot (repo, sepolicy, AT_FDCWD, srcpath, boot_dfd, rel_destpath, + if (!install_into_boot (repo, sepolicy, AT_FDCWD, srcpath, sysroot->boot_fd, rel_destpath, cancellable, error)) return FALSE; } @@ -2019,7 +2018,7 @@ install_deployment_kernel (OstreeSysroot *sysroot, ostree_bootconfig_parser_set (bootconfig, "options", options_key); glnx_autofd int bootconf_dfd = -1; - if (!glnx_opendirat (boot_dfd, bootconfdir, TRUE, &bootconf_dfd, error)) + if (!glnx_opendirat (sysroot->boot_fd, bootconfdir, TRUE, &bootconf_dfd, error)) return FALSE; if (!ostree_bootconfig_parser_write_at (ostree_deployment_get_bootconfig (deployment), @@ -2076,15 +2075,14 @@ swap_bootloader (OstreeSysroot *sysroot, g_assert ((current_bootversion == 0 && new_bootversion == 1) || (current_bootversion == 1 && new_bootversion == 0)); - glnx_autofd int boot_dfd = -1; - if (!glnx_opendirat (sysroot->sysroot_fd, "boot", TRUE, &boot_dfd, error)) + 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 (boot_dfd, "loader.tmp", boot_dfd, "loader", error)) + if (!glnx_renameat (sysroot->boot_fd, "loader.tmp", sysroot->boot_fd, "loader", error)) return FALSE; /* Now we explicitly fsync this directory, even though it @@ -2096,7 +2094,7 @@ swap_bootloader (OstreeSysroot *sysroot, * for whatever reason, and we wouldn't want to confuse the * admin by going back to the previous session. */ - if (fsync (boot_dfd) != 0) + if (fsync (sysroot->boot_fd) != 0) return glnx_throw_errno_prefix (error, "fsync(boot)"); /* TODO: In the future also execute this automatically via a systemd unit diff --git a/src/libostree/ostree-sysroot-private.h b/src/libostree/ostree-sysroot-private.h index 318b0b19..641f0533 100644 --- a/src/libostree/ostree-sysroot-private.h +++ b/src/libostree/ostree-sysroot-private.h @@ -56,6 +56,7 @@ struct OstreeSysroot { GFile *path; int sysroot_fd; + int boot_fd; GLnxLockFile lock; OstreeSysrootLoadState loadstate; @@ -160,6 +161,9 @@ char * _ostree_sysroot_get_runstate_path (OstreeDeployment *deployment, const ch char *_ostree_sysroot_join_lines (GPtrArray *lines); +gboolean +_ostree_sysroot_ensure_boot_fd (OstreeSysroot *self, GError **error); + gboolean _ostree_sysroot_query_bootloader (OstreeSysroot *sysroot, OstreeBootloader **out_bootloader, GCancellable *cancellable, diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index e3d7e425..d04b665d 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -198,6 +198,7 @@ ostree_sysroot_init (OstreeSysroot *self) keys, G_N_ELEMENTS (keys)); self->sysroot_fd = -1; + self->boot_fd = -1; } /** @@ -278,6 +279,19 @@ ensure_sysroot_fd (OstreeSysroot *self, &self->sysroot_fd, error)) return FALSE; } + + return TRUE; +} + +gboolean +_ostree_sysroot_ensure_boot_fd (OstreeSysroot *self, GError **error) +{ + if (self->boot_fd == -1) + { + if (!glnx_opendirat (self->sysroot_fd, "boot", TRUE, + &self->boot_fd, error)) + return FALSE; + } return TRUE; } @@ -380,6 +394,7 @@ void ostree_sysroot_unload (OstreeSysroot *self) { glnx_close_fd (&self->sysroot_fd); + glnx_close_fd (&self->boot_fd); } /** -- cgit v1.2.1 From 9a526bbaa5ad6a75e3b0d8052d93934df3e7a20f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 9 Jan 2021 18:34:27 +0000 Subject: sysroot: Handle ro /boot but rw /sysroot The recent change in https://github.com/coreos/fedora-coreos-config/pull/659 broke some of our tests that do `mount -o remount,rw /sysroot` but leave `/boot` read-only. We had code for having `/boot` read-only before `/sysroot` but in practice we had a file descriptor for `/sysroot` that we opened before the remount that would happen later on. Clean things up here so that in the library, we also remount `/boot` at the same time we remount `/sysroot` if either are readonly. Delete the legacy code for remounting `/boot` rw if we're not in a mount namespace. I am fairly confident most users are either using the `ostree` CLI, or they're using the mount namespace. --- src/libostree/ostree-sysroot-deploy.c | 87 +---------------------------------- src/libostree/ostree-sysroot.c | 47 ++++++++++++++----- src/libotutil/otutil.h | 6 +++ 3 files changed, 44 insertions(+), 96 deletions(-) diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c index ad9b56d1..0fff820b 100644 --- a/src/libostree/ostree-sysroot-deploy.c +++ b/src/libostree/ostree-sysroot-deploy.c @@ -56,9 +56,6 @@ #define OSTREE_DEPLOYMENT_FINALIZING_ID SD_ID128_MAKE(e8,64,6c,d6,3d,ff,46,25,b7,79,09,a8,e7,a4,09,94) #endif -static gboolean -is_ro_mount (const char *path); - /* * Like symlinkat() but overwrites (atomically) an existing * symlink. @@ -2225,57 +2222,6 @@ cleanup_legacy_current_symlinks (OstreeSysroot *self, return TRUE; } -/* Detect whether or not @path refers to a read-only mountpoint. This is - * currently just used to handle a potentially read-only /boot by transiently - * remounting it read-write. In the future we might also do this for e.g. - * /sysroot. - */ -static gboolean -is_ro_mount (const char *path) -{ -#ifdef HAVE_LIBMOUNT - /* Dragging in all of this crud is apparently necessary just to determine - * whether something is a mount point. - * - * Systemd has a totally different implementation in - * src/basic/mount-util.c. - */ - struct libmnt_table *tb = mnt_new_table_from_file ("/proc/self/mountinfo"); - struct libmnt_fs *fs; - struct libmnt_cache *cache; - gboolean is_mount = FALSE; - struct statvfs stvfsbuf; - - if (!tb) - return FALSE; - - /* to canonicalize all necessary paths */ - cache = mnt_new_cache (); - mnt_table_set_cache (tb, cache); - - fs = mnt_table_find_target(tb, path, MNT_ITER_BACKWARD); - is_mount = fs && mnt_fs_get_target (fs); -#ifdef HAVE_MNT_UNREF_CACHE - mnt_unref_table (tb); - mnt_unref_cache (cache); -#else - mnt_free_table (tb); - mnt_free_cache (cache); -#endif - - if (!is_mount) - return FALSE; - - /* We *could* parse the options, but it seems more reliable to - * introspect the actual mount at runtime. - */ - if (statvfs (path, &stvfsbuf) == 0) - return (stvfsbuf.f_flag & ST_RDONLY) != 0; - -#endif - return FALSE; -} - /** * ostree_sysroot_write_deployments: * @self: Sysroot @@ -2577,42 +2523,13 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, } else { - gboolean boot_was_ro_mount = FALSE; - if (self->booted_deployment) - boot_was_ro_mount = is_ro_mount ("/boot"); - - g_debug ("boot is ro: %s", boot_was_ro_mount ? "yes" : "no"); - - if (boot_was_ro_mount) - { - if (mount ("/boot", "/boot", NULL, MS_REMOUNT | MS_SILENT, NULL) < 0) - return glnx_throw_errno_prefix (error, "Remounting /boot read-write"); - } - if (!_ostree_sysroot_query_bootloader (self, &bootloader, cancellable, error)) return FALSE; bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader); - /* Note equivalent of try/finally here */ - gboolean success = write_deployments_bootswap (self, new_deployments, opts, bootloader, - &syncstats, cancellable, error); - /* Below here don't set GError until the if (!success) check. - * Note we only bother remounting if a mount namespace isn't in use. - * */ - if (boot_was_ro_mount && !self->mount_namespace_in_use) - { - if (mount ("/boot", "/boot", NULL, MS_REMOUNT | MS_RDONLY | MS_SILENT, NULL) < 0) - { - /* Only make this a warning because we don't want to - * completely bomb out if some other process happened to - * jump in and open a file there. See above TODO - * around doing this in a new mount namespace. - */ - g_printerr ("warning: Failed to remount /boot read-only: %s\n", strerror (errno)); - } - } - if (!success) + if (!write_deployments_bootswap (self, new_deployments, opts, bootloader, + &syncstats, cancellable, error)) return FALSE; } diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index d04b665d..45baf883 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -295,6 +295,31 @@ _ostree_sysroot_ensure_boot_fd (OstreeSysroot *self, GError **error) return TRUE; } +static gboolean +remount_writable (const char *path, gboolean *did_remount, GError **error) +{ + *did_remount = FALSE; + struct statvfs stvfsbuf; + if (statvfs (path, &stvfsbuf) < 0) + { + if (errno != ENOENT) + return glnx_throw_errno_prefix (error, "statvfs(%s)", path); + else + return TRUE; + } + + if ((stvfsbuf.f_flag & ST_RDONLY) != 0) + { + /* OK, let's remount writable. */ + if (mount (path, path, NULL, MS_REMOUNT | MS_RELATIME, "") < 0) + return glnx_throw_errno_prefix (error, "Remounting %s read-write", path); + *did_remount = TRUE; + g_debug ("remounted %s writable", path); + } + + return TRUE; +} + /* Remount /sysroot read-write if necessary */ gboolean _ostree_sysroot_ensure_writable (OstreeSysroot *self, @@ -314,19 +339,19 @@ _ostree_sysroot_ensure_writable (OstreeSysroot *self, if (!self->root_is_ostree_booted) return TRUE; - /* Check if /sysroot is a read-only mountpoint */ - struct statvfs stvfsbuf; - if (statvfs ("/sysroot", &stvfsbuf) < 0) - return glnx_throw_errno_prefix (error, "fstatvfs(/sysroot)"); - if ((stvfsbuf.f_flag & ST_RDONLY) == 0) - return TRUE; + /* In these cases we also require /boot */ + if (!_ostree_sysroot_ensure_boot_fd (self, error)) + return FALSE; - /* OK, let's remount writable. */ - if (mount ("/sysroot", "/sysroot", NULL, MS_REMOUNT | MS_RELATIME, "") < 0) - return glnx_throw_errno_prefix (error, "Remounting /sysroot read-write"); + gboolean did_remount_sysroot = FALSE; + if (!remount_writable ("/sysroot", &did_remount_sysroot, error)) + return FALSE; + gboolean did_remount_boot = FALSE; + if (!remount_writable ("/boot", &did_remount_boot, error)) + return FALSE; - /* Reopen our fd */ - glnx_close_fd (&self->sysroot_fd); + /* Now close and reopen our file descriptors */ + ostree_sysroot_unload (self); if (!ensure_sysroot_fd (self, error)) return FALSE; diff --git a/src/libotutil/otutil.h b/src/libotutil/otutil.h index 7db7270d..4cc5cd9f 100644 --- a/src/libotutil/otutil.h +++ b/src/libotutil/otutil.h @@ -34,6 +34,12 @@ #define OT_VARIANT_BUILDER_INITIALIZER {{{0,}}} #endif +static inline const char * +ot_booltostr (int b) +{ + return b ? "true" : "false"; +} + #define ot_gobject_refz(o) (o ? g_object_ref (o) : o) #define ot_transfer_out_value(outp, srcp) G_STMT_START { \ -- cgit v1.2.1