Ensure ESP is mounted #127

kelvinfan001 · 2020-12-21T23:05:58Z

Following coreos/fedora-coreos-config#794,
the ESP will no longer be mounted by default in FCOS. Mount the
ESP at a directory with a random name in bootupd's sandboxed /tmp
ourselves before reading from/writing to it.

Note that ESPs on redundant disks is not yet supported, this commit
simply mounts the device with part-label EFI-SYSTEM if one exists.

ProtectClock=yes in bootupd.service is preventing the ESP from
being mounted, remove that for now.

kelvinfan001 · 2020-12-21T23:06:35Z

Kola tests pass with a local FCOS build with coreos/fedora-coreos-config#794.

bgilbert · 2020-12-22T00:05:17Z

No longer ProtectClock=yes in bootupd.service to allow bootupd to mount the ESP.

Hmm, could you expand on this? Why do these conflict?

bgilbert

Unfortunately there are deeper architectural consequences to having multiple ESPs. Every operation will need to iterate over all ESPs, and cannot assume that the ESPs are all in the same state. (There's a reasonable argument that if the ESPs get desynced, fixing it isn't our responsibility, but it'd be better UX if we handled it anyway.)

bgilbert · 2020-12-22T00:10:44Z

src/efi.rs

        let sysroot = openat::Dir::open("/")?;
        let esp = sysroot.sub_dir(&self.esp_path())?;
        Ok(esp)
    }
+
+    fn ensure_mounted_esp(&self) -> Result<()> {
+        let mount_point = &Path::new("/").join(MOUNT_PATH);


Probably we should create a temporary directory to use as a mountpoint, and then clean it up afterward.

We should probably add PrivateTmp=yes too. (EDIT: already is there)

Yeah if we use PrivateTmp then we can rely on systemd to clean up our tempdir.
Then I'd use https://docs.rs/tempfile/3.1.0/tempfile/struct.TempDir.html#method.into_path (i.e. make a tempdir, mount on it and then return the path).

What do you think about only updating ESP's that are listed in NVRAM? Non-UEFI is trickier, risky to step on all BIOS Boot or MBR gaps with grub-install.

For CoreOS, at least, we can make some simplifying assumptions. We own the whole system, so we can update every ESP and BIOS Boot partition we see. CoreOS boots via Default Boot Behavior and doesn't set Boot NVRAM variables at all.

If bootupd is going to generalize beyond CoreOS, of course, those assumptions may no longer be workable.

Yep, I'm thinking the idea to rebase Silverblue on CoreOS.

That use case includes dual boot, so I think "fair game" includes overwriting the MBR gap, BIOS Boot, and ESP:/EFI/fedora/ on the drive that contains the current active /; but not anything else. i.e. EFI/fedora is owned but not the entire ESP. There's a low, but non-zero, chance of USB sticks, SD Cards, external drives or even other internal drives, are present and used to boot another OS; and also can't be touched.

Another possible use case is Silverblue (or Workstation) running from an SD Card or USB stick. It's a use case we're more seriously considering for Workstation as a way to maybe reduce dependency on dual-boot installations.

Right, so far bootupd is Fedora CoreOS specific but the idea from the start is it could generalize indeed to other rpm-ostree systems (i.e. not necessarily coreos-assembler generated) as well as even not rpm-ostree systems (traditional yum/dnf/zypper/apt).

It's still not clear to me that the latter effort is really worth it but I don't want to exclude it.

src/efi.rs

kelvinfan001 · 2020-12-22T00:17:11Z

Hmm, could you expand on this? Why do these conflict?

Admittedly, it was just trial and error. The tests passed when I removed this; otherwise, there were some permission issues.

cgwalters

Thanks for working on this!

cgwalters · 2020-12-22T16:34:46Z

src/efi.rs

        let sysroot = openat::Dir::open("/")?;
        let esp = sysroot.sub_dir(&self.esp_path())?;
        Ok(esp)
    }
+
+    fn ensure_mounted_esp(&self) -> Result<()> {
+        let mount_point = &Path::new("/").join(MOUNT_PATH);


We should probably add PrivateTmp=yes too. (EDIT: already is there)

cgwalters · 2020-12-22T16:56:21Z

src/efi.rs

        let sysroot = openat::Dir::open("/")?;
        let esp = sysroot.sub_dir(&self.esp_path())?;
        Ok(esp)
    }
+
+    fn ensure_mounted_esp(&self) -> Result<()> {
+        let mount_point = &Path::new("/").join(MOUNT_PATH);


Yeah if we use PrivateTmp then we can rely on systemd to clean up our tempdir.
Then I'd use https://docs.rs/tempfile/3.1.0/tempfile/struct.TempDir.html#method.into_path (i.e. make a tempdir, mount on it and then return the path).

cgwalters · 2020-12-22T16:57:03Z

systemd/bootupd.service

@@ -19,7 +19,6 @@ ProtectHome=yes
 ReadOnlyPaths=/usr
 PrivateTmp=yes
 PrivateNetwork=yes
-ProtectClock=yes


Probably something going wrong in the seccomp policy systemd makes. It's...about time...we looked at that.

cgwalters · 2021-01-04T16:01:56Z

src/efi.rs

+
+    fn ensure_mounted_esp(&self) -> Result<()> {
+        let mount_point = &Path::new("/").join(&*MOUNT_PATH);
+        let output = std::process::Command::new("mountpoint")


This isn't bad, but now that we know our process is the only one doing the mount (in our mount namespace), I think it'd be better to have a something like Arc<Mutex<Option<PathBuf>>> or...actually there's a lazy_init crate for that even.

Basically do the tempdir creation + mount as a single locked unit instead.

cgwalters

One minor must fix for the crate ref, everything else is optional or can be followup.

Thanks for working on this!

cgwalters · 2021-01-04T16:11:23Z

tests/kola/data/libtest.sh

@@ -81,3 +81,10 @@ assert_not_file_has_content_literal () {
    done
 }

+# Mount the EFI partition at a temporary location.
+tmpefimount=/tmp/efi


I know this is just the test suite, but I try as a general rule to avoid predictably named temporary files. We could have this code mirror the main logic and use e.g. $(mktemp -d) and "return" it by printing, so
callers do e.g.:
tmpefimount=$(mount_tmp_efi)
etc.

Not a blocker, just noting.

cgwalters · 2021-01-04T16:12:18Z

src/efi.rs

@@ -14,6 +14,7 @@ use anyhow::{bail, Context, Result};
 use openat_ext::OpenatDirExt;

 use chrono::prelude::*;
+use structopt::lazy_static::lazy_static;


We should add the dependency to Cargo.toml directly; it looks like a bug that structopt is re-exporting it.

src/efi.rs

Following coreos/fedora-coreos-config#794, the ESP will no longer be mounted by default in FCOS. Mount the ESP at a directory with a random name in bootupd's sandboxed /tmp ourselves before reading from/writing to it. Note that ESPs on redundant disks is not yet supported, this commit simply mounts the device with part-label `EFI-SYSTEM` if one exists. `ProtectClock=yes` in `bootupd.service` is preventing the ESP from being mounted, remove that for now.

Following coreos/fedora-coreos-config#794, the ESP will no longer be mounted in /boot. Adjust tests to mount it before reading from it.

cgwalters

/lgtm

openshift-ci-robot · 2021-01-05T20:25:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, kelvinfan001

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,kelvinfan001]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cgwalters · 2021-01-06T01:48:09Z

Moved the ProtectClock issue to systemd/systemd#18142

kelvinfan001 requested a review from cgwalters December 21, 2020 23:05

openshift-ci-robot added the do-not-merge/work-in-progress label Dec 21, 2020

openshift-ci-robot requested a review from lucab December 21, 2020 23:06

openshift-ci-robot added the approved label Dec 21, 2020

kelvinfan001 force-pushed the kfan-mount-efi branch 4 times, most recently from 9dc090a to 2359f12 Compare December 21, 2020 23:20

bgilbert reviewed Dec 22, 2020

View reviewed changes

cgwalters reviewed Dec 22, 2020

View reviewed changes

kelvinfan001 force-pushed the kfan-mount-efi branch 4 times, most recently from 0b875d0 to a4b2255 Compare December 24, 2020 19:32

bgilbert mentioned this pull request Dec 25, 2020

Support mirroring /boot, ESP, BIOS bootloader on first boot coreos/fedora-coreos-config#718

Merged

kelvinfan001 changed the title ~~WIP: Ensure ESP is mounted~~ Ensure ESP is mounted Jan 4, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress label Jan 4, 2021

cgwalters reviewed Jan 4, 2021

View reviewed changes

cgwalters approved these changes Jan 4, 2021

View reviewed changes

travier reviewed Jan 5, 2021

View reviewed changes

src/efi.rs Show resolved Hide resolved

kelvinfan001 added 2 commits January 5, 2021 14:47

tests: Mount ESP before operations

644180e

Following coreos/fedora-coreos-config#794, the ESP will no longer be mounted in /boot. Adjust tests to mount it before reading from it.

kelvinfan001 force-pushed the kfan-mount-efi branch from a4b2255 to 644180e Compare January 5, 2021 19:47

cgwalters approved these changes Jan 5, 2021

View reviewed changes

openshift-ci-robot assigned cgwalters Jan 5, 2021

openshift-ci-robot added the lgtm label Jan 5, 2021

openshift-merge-robot merged commit 402bc6c into coreos:master Jan 5, 2021

cgwalters mentioned this pull request Jan 6, 2021

ProtectClock=yes blocks mount() system call for block device nodes systemd/systemd#18142

Closed

jlebon mentioned this pull request Jan 6, 2021

Support updating multiple EFIs in mirrored setups #132

Open

travier mentioned this pull request May 16, 2024

fwupdmgr - UEFI ESP partition not detected or configured coreos/fedora-coreos-tracker#1623

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure ESP is mounted #127

Ensure ESP is mounted #127

kelvinfan001 commented Dec 21, 2020 •

edited

Loading

kelvinfan001 commented Dec 21, 2020

bgilbert commented Dec 22, 2020

bgilbert left a comment

bgilbert Dec 22, 2020

cgwalters Dec 22, 2020 •

edited

Loading

cgwalters Dec 22, 2020

cmurf Dec 25, 2020

bgilbert Dec 25, 2020

cmurf Dec 26, 2020 •

edited

Loading

cgwalters Jan 4, 2021

kelvinfan001 commented Dec 22, 2020 •

edited

Loading

cgwalters left a comment

cgwalters Dec 22, 2020 •

edited

Loading

cgwalters Dec 22, 2020

cgwalters Dec 22, 2020

cgwalters Jan 4, 2021

cgwalters left a comment

cgwalters Jan 4, 2021

cgwalters Jan 4, 2021

cgwalters left a comment

openshift-ci-robot commented Jan 5, 2021

cgwalters commented Jan 6, 2021

Ensure ESP is mounted #127

Ensure ESP is mounted #127

Conversation

kelvinfan001 commented Dec 21, 2020 • edited Loading

kelvinfan001 commented Dec 21, 2020

bgilbert commented Dec 22, 2020

bgilbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmurf Dec 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvinfan001 commented Dec 22, 2020 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 5, 2021

cgwalters commented Jan 6, 2021

kelvinfan001 commented Dec 21, 2020 •

edited

Loading

cgwalters Dec 22, 2020 •

edited

Loading

cmurf Dec 26, 2020 •

edited

Loading

kelvinfan001 commented Dec 22, 2020 •

edited

Loading

cgwalters Dec 22, 2020 •

edited

Loading