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