ostree-remount: improve do_remount(), use warn()

When we remount read only, we only want to change the vfs read only
flag, not the fs one, so use MS_BIND.
On the contrary when we remount read write, we want to change both (already ok).
To check that we can write, we now use 'access'.

Except when using old mount versions, we should never need do_remount(),
as everything should have been properly setup by prepare-root,
so start to warn() when we actually remounted /sysroot or /etc.

Author: champtar

src/switchroot/ostree-remount.c

@@ -43,7 +43,7 @@
 #include "otcore.h"
 
 static void
-do_remount (const char *target, bool writable)
+do_remount (const char *target, bool writable, bool use_warn)
 {
   struct stat stbuf;
   if (lstat (target, &stbuf) < 0)
@@ -53,18 +53,27 @@ do_remount (const char *target, bool writable)
    */
   if (S_ISLNK (stbuf.st_mode))
     return;
-  /* If not a mountpoint, skip it */
+
   struct statvfs stvfsbuf;
   if (statvfs (target, &stvfsbuf) == -1)
-    return;
+    err (EXIT_FAILURE, "failed to statvfs(%s)", target);
 
-  const bool currently_writable = ((stvfsbuf.f_flag & ST_RDONLY) == 0);
-  if (writable == currently_writable)
-    return;
+  if (writable)
+    {
+      // vfs could be 'rw' but fs 'ro', so use access to check
+      if (access (target, W_OK) == 0)
+        return;
+    }
+  else
+    {
+      // ensure vfs 'ro' mount flags is set even if fs is ro
+      if ((stvfsbuf.f_flag & ST_RDONLY) != 0)
+        return;
+    }
 
   int mnt_flags = MS_REMOUNT | MS_SILENT;
   if (!writable)
-    mnt_flags |= MS_RDONLY;
+    mnt_flags |= MS_BIND | MS_RDONLY;
   if (mount (target, target, NULL, mnt_flags, NULL) < 0)
     {
       /* Also ignore EINVAL - if the target isn't a mountpoint
@@ -76,7 +85,10 @@ do_remount (const char *target, bool writable)
         return;
     }
 
-  printf ("Remounted %s: %s\n", writable ? "rw" : "ro", target);
+  if (use_warn)
+    warn ("Remounted %s: %s\n", writable ? "rw" : "ro", target);
+  else
+    printf ("Remounted %s: %s\n", writable ? "rw" : "ro", target);
 }
 
 /* Relabel the directory $real_path, which is going to be an overlayfs mount,
@@ -226,29 +238,25 @@ main (int argc, char *argv[])
     }
 
   /* Handle remounting /sysroot; if it's explicitly marked as read-only (opt in)
-   * then ensure it's readonly, otherwise mount writable, the same as /
+   * then ensure it's readonly, otherwise remount writable, the same as /
+   * We should not need this so warn when we do remount.
    */
   gboolean sysroot_configured_readonly = FALSE;
   g_variant_dict_lookup (ostree_run_metadata, OTCORE_RUN_BOOTED_KEY_SYSROOT_RO, "b",
                          &sysroot_configured_readonly);
-  do_remount ("/sysroot", !sysroot_configured_readonly);
+  do_remount ("/sysroot", !sysroot_configured_readonly, true);
 
-  /* And also make sure to make /etc rw again. We make this conditional on
-   * sysroot_configured_readonly && !transient_etc because only in that case is it a
-   * bind-mount. */
+  /* Make sure /etc is 'rw'. We make this conditional on sysroot_configured_readonly
+   * && !transient_etc because only in that case is it a bind-mount.
+   * We should not need this so warn when we do remount.
+   */
   if (sysroot_configured_readonly && !transient_etc)
-    do_remount ("/etc", true);
-
-  /* If /var was created as as an OSTree default bind mount (instead of being a separate
-   * filesystem) then remounting the root mount read-only also remounted it. So just like /etc, we
-   * need to make it read-write by default. If it was a separate filesystem, we expect it to be
-   * writable anyways, so it doesn't hurt to remount it if so.
-   *
-   * And if we started out with a writable system root, then we need
-   * to ensure that the /var bind mount created by the systemd generator
-   * is writable too.
+    do_remount ("/etc", true, true);
+
+  /* If mount uses the legacy mount API (before util-linux 2.39), then the 'rw'
+   * mount option in the 'var.mount' unit is ignored, so we need to remount here.
    */
-  do_remount ("/var", true);
+  do_remount ("/var", true, false);
 
   exit (EXIT_SUCCESS);
 }

tests/kolainst/destructive/root-transient-ro.sh

@@ -36,6 +36,9 @@ EOF
 
   test '!' -w '/'
 
+  journalctl -u ostree-remount.service -o cat > /tmp/ostree-remount.logs
+  assert_not_file_has_content /tmp/ostree-remount.logs Remount
+
   echo "ok root transient-ro"
   ;;
   *) 

tests/kolainst/destructive/soft-reboot.sh

@@ -156,6 +156,9 @@ case "${AUTOPKGTEST_REBOOT_MARK:-}" in
   fi
   assert_file_has_content_literal err.txt "different kernel state"
 
+  journalctl -u ostree-remount.service -o cat > /tmp/ostree-remount.logs
+  assert_not_file_has_content /tmp/ostree-remount.logs Remount
+
   echo "ok soft reboot all tests"
   ;;
   *) 

tests/kolainst/destructive/var-mount.sh

@@ -8,12 +8,14 @@ case "${AUTOPKGTEST_REBOOT_MARK:-}" in
   "") 
     touch "/var/somenewfile"
     stateroot=$(ostree admin status 2> /dev/null | grep '^\*' | cut -d ' ' -f2 || true)
-    echo "/sysroot/ostree/deploy/${stateroot}/var /var none bind 0 0" >> /etc/fstab
+    echo "/sysroot/ostree/deploy/${stateroot}/var /var none bind,rw 0 0" >> /etc/fstab
     /tmp/autopkgtest-reboot "2"
     ;;
   "2")
     systemctl status var.mount
     test -f /var/somenewfile
+    journalctl -u ostree-remount.service -o cat > /tmp/ostree-remount.logs
+    assert_not_file_has_content /tmp/ostree-remount.logs Remount
     ;;
   *) fatal "Unexpected AUTOPKGTEST_REBOOT_MARK=${AUTOPKGTEST_REBOOT_MARK}" ;;
 esac