Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

install-to-disk: Check for mounts before install #805

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1802,6 +1802,7 @@ fn test_gather_root_args() {
maj_min: "252:4".into(),
options: "rw".into(),
uuid: Some("965eb3c7-5a3f-470d-aaa2-1bcf04334bc6".into()),
children: None,
};
let r = find_root_args_to_inherit(&[], &inspect).unwrap();
assert_eq!(r.mount_spec, "UUID=965eb3c7-5a3f-470d-aaa2-1bcf04334bc6");
Expand Down
6 changes: 6 additions & 0 deletions lib/src/install/baseline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use super::State;
use super::RUN_BOOTC;
use super::RW_KARG;
use crate::mount;
use crate::mount::is_mounted_in_pid1_mountns;
use crate::task::Task;

// This ensures we end up under 512 to be small-sized.
Expand Down Expand Up @@ -162,6 +163,11 @@ pub(crate) fn install_create_rootfs(
// Canonicalize devpath
let devpath: Utf8PathBuf = device.path().into();

// Always disallow writing to mounted device
if is_mounted_in_pid1_mountns(device.path())? {
anyhow::bail!("Device {} is mounted", device.path())
}

// Handle wiping any existing data
if opts.wipe {
let dev = &opts.device;
Expand Down
43 changes: 40 additions & 3 deletions lib/src/mount.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::task::Task;

#[derive(Deserialize, Debug)]
#[serde(rename_all = "kebab-case")]
#[allow(dead_code)]
pub(crate) struct Filesystem {
// Note if you add an entry to this list, you need to change the --output invocation below too
pub(crate) source: String,
Expand All @@ -21,14 +22,15 @@ pub(crate) struct Filesystem {
pub(crate) fstype: String,
pub(crate) options: String,
pub(crate) uuid: Option<String>,
pub(crate) children: Option<Vec<Filesystem>>,
}

#[derive(Deserialize, Debug)]
pub(crate) struct Findmnt {
pub(crate) filesystems: Vec<Filesystem>,
}

fn run_findmnt(args: &[&str], path: &str) -> Result<Filesystem> {
fn run_findmnt(args: &[&str], path: &str) -> Result<Findmnt> {
let o: Findmnt = Command::new("findmnt")
.args([
"-J",
Expand All @@ -40,6 +42,12 @@ fn run_findmnt(args: &[&str], path: &str) -> Result<Filesystem> {
.arg(path)
.log_debug()
.run_and_parse_json()?;
Ok(o)
}

// Retrieve a mounted filesystem from a device given a matching path
fn findmnt_filesystem(args: &[&str], path: &str) -> Result<Filesystem> {
let o = run_findmnt(args, path)?;
o.filesystems
.into_iter()
.next()
Expand All @@ -50,13 +58,42 @@ fn run_findmnt(args: &[&str], path: &str) -> Result<Filesystem> {
/// Inspect a target which must be a mountpoint root - it is an error
/// if the target is not the mount root.
pub(crate) fn inspect_filesystem(path: &Utf8Path) -> Result<Filesystem> {
run_findmnt(&["--mountpoint"], path.as_str())
findmnt_filesystem(&["--mountpoint"], path.as_str())
}

#[context("Inspecting filesystem by UUID {uuid}")]
/// Inspect a filesystem by partition UUID
pub(crate) fn inspect_filesystem_by_uuid(uuid: &str) -> Result<Filesystem> {
run_findmnt(&["--source"], &(format!("UUID={uuid}")))
findmnt_filesystem(&["--source"], &(format!("UUID={uuid}")))
}

// Check if a specified device contains an already mounted filesystem
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"already mounted filesystem in the root mount namespace" or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, will resolve when pushed

pub(crate) fn is_mounted_in_pid1_mountns(path: String) -> Result<bool> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this take &str

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, will resolve when pushed

let o = run_findmnt(&["-N"], "1")?;
for fs in o.filesystems.iter() {
if is_source_mounted(&path, fs)? {
return Ok(true);
}
}

Ok(false)
Comment on lines +73 to +79
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all fine but with the below change could be condensed to something like
Ok(o.filesystems.iter().any(|fs| is_source_mounted(path)))

Or:

let mounted = o.filesystems.iter().any(|fs| is_source_mounted(path));
Ok(mounted)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, will resolve when pushed

}

// Recursively check a given filesystem to see if it contains an already mounted source
pub(crate) fn is_source_mounted(path: &String, mounted_fs: &Filesystem) -> Result<bool> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, will resolve when pushed

if mounted_fs.source.contains(path) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit uncertain about source.contains(path) vs mounted_fs.source.as_str() == path here...I guess you're probably doing that as a way to handle e.g. /dev/vda3 being mounted, but we're trying to match vs a whole device like /dev/vda?

Just thinking through this...what is probably cleaner actually is to do a set-intersection; on the calling side loop through all children of the device.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow what you're saying, my understanding is that we want to see if /dev/vda (the device) is mounted or if any partition on it is mounted. That's what I wrote it for, so it will kickback the mounting error if any part of the device is mounted.

Are you saying that this should only fail if the device alone is mounted? As in it shouldn't be checking the partitions of the device?

That's the reasoning behind the source.contains(path), I can't be sure what number partition will be mounted but I can work off it having the same naming stem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what I mean is doing the check like this:

diff --git a/lib/src/install/baseline.rs b/lib/src/install/baseline.rs
index f7c1c35..b8eb8bd 100644
--- a/lib/src/install/baseline.rs
+++ b/lib/src/install/baseline.rs
@@ -165,7 +165,13 @@ pub(crate) fn install_create_rootfs(
     // Handle wiping any existing data
     if opts.wipe {
         let dev = &opts.device;
+        if crate::mount::is_mounted_in_pid1_mountns(device.path()) {
+            anyhow::bail!("Device {} is mounted", device.path());
+        }
         for child in device.children.iter().flatten() {
+            if crate::mount::is_mounted_in_pid1_mountns(child.path()) {
+                anyhow::bail!("Device {} is mounted", child.path());
+            }
             let child = child.path();
             println!("Wiping {child}");
             crate::blockdev::wipefs(Utf8Path::new(&child))?;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe cleaner actually, make is_mounted_in_pid1_mountns take a &Device and have it iterate over children.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of looping through the findmnt output and searching for a matching device, loop through the device's children and check each to see if it's mounted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original thought a couple of weeks ago when I first looked at this, I ended up going with parsing the findmnt output because it seemed more straightforward for reasons I can't exactly remember now.

For the code block you noted above, should mount checking be within the opts.wipe handling? Or are you just using that as an example of what the loop would look like?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh nevermind, no need to bikeshed this one fully, I think it's fine as is. Let's just do the minor tweaks and get it in!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy that, will push as soon as I run through my testing

return Ok(true);
}

if let Some(ref children) = mounted_fs.children {
for child in children {
if is_source_mounted(path, child)? {
return Ok(true);
}
}
}

Ok(false)
}

/// Mount a device to the target path.
Expand Down