From 49282854f178abdaca72881a01dc7b9392557a32 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 24 Aug 2021 15:50:23 +1000 Subject: [PATCH 1/5] agent: Simplify BareMount::mount by using nix::mount::mount BareMount::mount does some complicated marshalling and uses unsafe code to call into the mount(2) system call. However, we're already using the nix crate which provides a more Rust-like wrapper for mount(2). We're even already using nix::mount::umount and nix::mount::MsFlags from the same module. In the same way, we can replace the direct usage of libc::umount() with nix::mount::umount() in one of the tests. Signed-off-by: David Gibson --- src/agent/src/mount.rs | 63 ++++++++++-------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 5b023b3393..2f318b8b2f 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -4,22 +4,18 @@ // use std::collections::HashMap; -use std::ffi::CString; use std::fs; use std::fs::File; -use std::io; use std::io::{BufRead, BufReader}; use std::iter; use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; -use std::ptr::null; use std::str::FromStr; use std::sync::Arc; use tokio::sync::Mutex; -use libc::{c_void, mount}; -use nix::mount::{self, MsFlags}; +use nix::mount::MsFlags; use nix::unistd::Gid; use regex::Regex; @@ -184,15 +180,6 @@ impl<'a> BareMount<'a> { #[instrument] pub fn mount(&self) -> Result<()> { - let source; - let dest; - let fs_type; - let mut options = null(); - let cstr_options: CString; - let cstr_source: CString; - let cstr_dest: CString; - let cstr_fs_type: CString; - if self.source.is_empty() { return Err(anyhow!("need mount source")); } @@ -201,24 +188,10 @@ impl<'a> BareMount<'a> { return Err(anyhow!("need mount destination")); } - cstr_source = CString::new(self.source)?; - source = cstr_source.as_ptr(); - - cstr_dest = CString::new(self.destination)?; - dest = cstr_dest.as_ptr(); - if self.fs_type.is_empty() { return Err(anyhow!("need mount FS type")); } - cstr_fs_type = CString::new(self.fs_type)?; - fs_type = cstr_fs_type.as_ptr(); - - if !self.options.is_empty() { - cstr_options = CString::new(self.options)?; - options = cstr_options.as_ptr() as *const c_void; - } - info!( self.logger, "mount source={:?}, dest={:?}, fs_type={:?}, options={:?}", @@ -227,17 +200,22 @@ impl<'a> BareMount<'a> { self.fs_type, self.options ); - let rc = unsafe { mount(source, dest, fs_type, self.flags.bits(), options) }; - if rc < 0 { - return Err(anyhow!( + nix::mount::mount( + Some(self.source), + self.destination, + Some(self.fs_type), + self.flags, + Some(self.options), + ) + .map_err(|e| { + anyhow!( "failed to mount {:?} to {:?}, with error: {}", self.source, self.destination, - io::Error::last_os_error() - )); - } - Ok(()) + e + ) + }) } } @@ -816,7 +794,7 @@ pub fn cgroups_mount(logger: &Logger, unified_cgroup_hierarchy: bool) -> Result< #[instrument] pub fn remove_mounts(mounts: &[String]) -> Result<()> { for m in mounts.iter() { - mount::umount(m.as_str()).context(format!("failed to umount {:?}", m))?; + nix::mount::umount(m.as_str()).context(format!("failed to umount {:?}", m))?; } Ok(()) } @@ -865,7 +843,6 @@ fn parse_options(option_list: Vec) -> HashMap { mod tests { use super::*; use crate::{skip_if_not_root, skip_loop_if_not_root, skip_loop_if_root}; - use libc::umount; use std::fs::metadata; use std::fs::File; use std::fs::OpenOptions; @@ -1023,17 +1000,7 @@ mod tests { assert!(result.is_ok(), "{}", msg); // Cleanup - unsafe { - let cstr_dest = - CString::new(dest_filename).expect("failed to convert dest to cstring"); - let umount_dest = cstr_dest.as_ptr(); - - let ret = umount(umount_dest); - - let msg = format!("{}: umount result: {:?}", msg, result); - - assert!(ret == 0, "{}", msg); - }; + nix::mount::umount(dest_filename.as_str()).unwrap(); continue; } From 9fa3beff4fc96cde9acbecbc772e23d9dae92058 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 24 Aug 2021 16:24:33 +1000 Subject: [PATCH 2/5] agent: Remove unnecessary BareMount structure struct Baremount contains the information necessary to make a new mount. As a datastructure, however, it's pointless, since every user just constructs it, immediately calls the BareMount::mount() method then discards the structure. Simplify the code by making this a direct function call baremount(). Signed-off-by: David Gibson --- src/agent/src/mount.rs | 121 ++++++++++++++----------------------- src/agent/src/namespace.rs | 5 +- src/agent/src/rpc.rs | 7 +-- src/agent/src/sandbox.rs | 5 +- src/agent/src/watcher.rs | 13 ++-- 5 files changed, 56 insertions(+), 95 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 2f318b8b2f..94d65d6c7f 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -145,78 +145,53 @@ pub const STORAGE_HANDLER_LIST: &[&str] = &[ DRIVER_WATCHABLE_BIND_TYPE, ]; -#[derive(Debug, Clone)] -pub struct BareMount<'a> { - source: &'a str, - destination: &'a str, - fs_type: &'a str, +#[instrument] +pub fn baremount( + source: &str, + destination: &str, + fs_type: &str, flags: MsFlags, - options: &'a str, - logger: Logger, -} + options: &str, + logger: &Logger, +) -> Result<()> { + let logger = logger.new(o!("subsystem" => "baremount")); -// mount mounts a source in to a destination. This will do some bookkeeping: -// * evaluate all symlinks -// * ensure the source exists -impl<'a> BareMount<'a> { - #[instrument] - pub fn new( - s: &'a str, - d: &'a str, - fs_type: &'a str, - flags: MsFlags, - options: &'a str, - logger: &Logger, - ) -> Self { - BareMount { - source: s, - destination: d, - fs_type, - flags, - options, - logger: logger.new(o!("subsystem" => "baremount")), - } + if source.is_empty() { + return Err(anyhow!("need mount source")); } - #[instrument] - pub fn mount(&self) -> Result<()> { - if self.source.is_empty() { - return Err(anyhow!("need mount source")); - } + if destination.is_empty() { + return Err(anyhow!("need mount destination")); + } - if self.destination.is_empty() { - return Err(anyhow!("need mount destination")); - } + if fs_type.is_empty() { + return Err(anyhow!("need mount FS type")); + } - if self.fs_type.is_empty() { - return Err(anyhow!("need mount FS type")); - } + info!( + logger, + "mount source={:?}, dest={:?}, fs_type={:?}, options={:?}", + source, + destination, + fs_type, + options + ); - info!( - self.logger, - "mount source={:?}, dest={:?}, fs_type={:?}, options={:?}", - self.source, - self.destination, - self.fs_type, - self.options - ); - - nix::mount::mount( - Some(self.source), - self.destination, - Some(self.fs_type), - self.flags, - Some(self.options), + nix::mount::mount( + Some(source), + destination, + Some(fs_type), + flags, + Some(options), + ) + .map_err(|e| { + anyhow!( + "failed to mount {:?} to {:?}, with error: {}", + source, + destination, + e ) - .map_err(|e| { - anyhow!( - "failed to mount {:?} to {:?}, with error: {}", - self.source, - self.destination, - e - ) - }) - } + }) } #[instrument] @@ -487,16 +462,14 @@ fn mount_storage(logger: &Logger, storage: &Storage) -> Result<()> { "mount-options" => options.as_str(), ); - let bare_mount = BareMount::new( + baremount( storage.source.as_str(), storage.mount_point.as_str(), storage.fstype.as_str(), flags, options.as_str(), &logger, - ); - - bare_mount.mount() + ) } /// Looks for `mount_point` entry in the /proc/mounts. @@ -615,11 +588,9 @@ fn mount_to_rootfs(logger: &Logger, m: &InitMount) -> Result<()> { let (flags, options) = parse_mount_flags_and_options(options_vec); - let bare_mount = BareMount::new(m.src, m.dest, m.fstype, flags, options.as_str(), logger); - fs::create_dir_all(Path::new(m.dest)).context("could not create directory")?; - bare_mount.mount().or_else(|e| { + baremount(m.src, m.dest, m.fstype, flags, &options, logger).or_else(|e| { if m.src != "dev" { return Err(e); } @@ -983,7 +954,7 @@ mod tests { std::fs::create_dir_all(d).expect("failed to created directory"); } - let bare_mount = BareMount::new( + let result = baremount( &src_filename, &dest_filename, d.fs_type, @@ -992,8 +963,6 @@ mod tests { &logger, ); - let result = bare_mount.mount(); - let msg = format!("{}: result: {:?}", msg, result); if d.error_contains.is_empty() { @@ -1070,7 +1039,7 @@ mod tests { } // Create an actual mount - let bare_mount = BareMount::new( + let result = baremount( mnt_src_filename, mnt_dest_filename, "bind", @@ -1078,8 +1047,6 @@ mod tests { "", &logger, ); - - let result = bare_mount.mount(); assert!(result.is_ok(), "mount for test setup failed"); let tests = &[ diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 200fc7c09d..8970e5f30c 100644 --- a/src/agent/src/namespace.rs +++ b/src/agent/src/namespace.rs @@ -13,7 +13,7 @@ use std::fs::File; use std::path::{Path, PathBuf}; use tracing::instrument; -use crate::mount::{BareMount, FLAGS}; +use crate::mount::{baremount, FLAGS}; use slog::Logger; const PERSISTENT_NS_DIR: &str = "/var/run/sandbox-ns"; @@ -129,8 +129,7 @@ impl Namespace { } }; - let bare_mount = BareMount::new(source, destination, "none", flags, "", &logger); - bare_mount.mount().map_err(|e| { + baremount(source, destination, "none", flags, "", &logger).map_err(|e| { anyhow!( "Failed to mount {} to {} with err:{:?}", source, diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 4698e23339..775b119dfd 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -47,7 +47,7 @@ use rustjail::process::ProcessOperations; use crate::device::{add_devices, pcipath_to_sysfs, rescan_pci_bus, update_device_cgroup}; use crate::linux_abi::*; use crate::metrics::get_metrics; -use crate::mount::{add_storages, remove_mounts, BareMount, STORAGE_HANDLER_LIST}; +use crate::mount::{add_storages, baremount, remove_mounts, STORAGE_HANDLER_LIST}; use crate::namespace::{NSTYPEIPC, NSTYPEPID, NSTYPEUTS}; use crate::network::setup_guest_dns; use crate::random; @@ -1624,15 +1624,14 @@ fn setup_bundle(cid: &str, spec: &mut Spec) -> Result { let rootfs_path = bundle_path.join("rootfs"); fs::create_dir_all(&rootfs_path)?; - BareMount::new( + baremount( &spec_root.path, rootfs_path.to_str().unwrap(), "bind", MsFlags::MS_BIND, "", &sl!(), - ) - .mount()?; + )?; spec.root = Some(Root { path: rootfs_path.to_str().unwrap().to_owned(), readonly: spec_root.readonly, diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index 93a4fa0feb..6829eb2cf5 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -449,7 +449,7 @@ fn online_memory(logger: &Logger) -> Result<()> { #[cfg(test)] mod tests { use super::Sandbox; - use crate::{mount::BareMount, skip_if_not_root}; + use crate::{mount::baremount, skip_if_not_root}; use anyhow::Error; use nix::mount::MsFlags; use oci::{Linux, Root, Spec}; @@ -461,8 +461,7 @@ mod tests { use tempfile::Builder; fn bind_mount(src: &str, dst: &str, logger: &Logger) -> Result<(), Error> { - let baremount = BareMount::new(src, dst, "bind", MsFlags::MS_BIND, "", logger); - baremount.mount() + baremount(src, dst, "bind", MsFlags::MS_BIND, "", logger) } #[tokio::test] diff --git a/src/agent/src/watcher.rs b/src/agent/src/watcher.rs index b6ec528988..840c847dec 100644 --- a/src/agent/src/watcher.rs +++ b/src/agent/src/watcher.rs @@ -20,7 +20,7 @@ use tokio::sync::Mutex; use tokio::task; use tokio::time::{self, Duration}; -use crate::mount::BareMount; +use crate::mount::baremount; use crate::protocols::agent as protos; /// The maximum number of file system entries agent will watch for each mount. @@ -314,16 +314,14 @@ impl SandboxStorages { } } - match BareMount::new( + match baremount( entry.source_mount_point.to_str().unwrap(), entry.target_mount_point.to_str().unwrap(), "bind", MsFlags::MS_BIND, "bind", logger, - ) - .mount() - { + ) { Ok(_) => { entry.watch = false; info!(logger, "watchable mount replaced with bind mount") @@ -427,15 +425,14 @@ impl BindWatcher { async fn mount(&self, logger: &Logger) -> Result<()> { fs::create_dir_all(WATCH_MOUNT_POINT_PATH).await?; - BareMount::new( + baremount( "tmpfs", WATCH_MOUNT_POINT_PATH, "tmpfs", MsFlags::empty(), "", logger, - ) - .mount()?; + )?; Ok(()) } From 08d7aebc281c099eadf387f2e50b8cf1b35b8bf6 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Thu, 9 Sep 2021 13:56:59 +1000 Subject: [PATCH 3/5] agent/mount: Split out regular file case from ensure_destination_exists() ensure_destination_exists() can create either a directory or a regular file depending on the arguments. This patch extracts the regular file specific option into its own helper: ensure_destination_file_exists(). This: - Avoids doing some steps in the directory case (they're already handled by create_dir_all()) - Enables some further future cleanups Signed-off-by: David Gibson --- src/agent/src/mount.rs | 49 ++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 94d65d6c7f..3bcc46a819 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -770,26 +770,36 @@ pub fn remove_mounts(mounts: &[String]) -> Result<()> { Ok(()) } +#[instrument] +fn ensure_destination_file_exists(path: &Path) -> Result<()> { + if path.is_file() { + return Ok(()); + } else if path.exists() { + return Err(anyhow!("{:?} exists but is not a regular file", path)); + } + + // The only way parent() can return None is if the path is /, + // which always exists, so the test above will already have caught + // it, thus the unwrap() is safe + let dir = path.parent().unwrap(); + + fs::create_dir_all(dir).context(format!("create_dir_all {:?}", dir))?; + + fs::File::create(path).context(format!("create empty file {:?}", path))?; + + Ok(()) +} + // ensure_destination_exists will recursively create a given mountpoint. If directories // are created, their permissions are initialized to mountPerm(0755) #[instrument] fn ensure_destination_exists(destination: &str, fs_type: &str) -> Result<()> { let d = Path::new(destination); - if d.exists() { - return Ok(()); - } - let dir = d - .parent() - .ok_or_else(|| anyhow!("mount destination {} doesn't exist", destination))?; - - if !dir.exists() { - fs::create_dir_all(dir).context(format!("create dir all {:?}", dir))?; - } if fs_type != "bind" || d.is_dir() { fs::create_dir_all(d).context(format!("create dir all {:?}", d))?; } else { - fs::File::create(d).context(format!("create file {:?}", d))?; + ensure_destination_file_exists(d)?; } Ok(()) @@ -1377,6 +1387,23 @@ mod tests { } } + #[test] + fn test_ensure_destination_file_exists() { + let dir = tempdir().expect("failed to create tmpdir"); + + let mut testfile = dir.into_path(); + testfile.push("testfile"); + + let result = ensure_destination_file_exists(&testfile); + + assert!(result.is_ok()); + assert!(testfile.exists()); + + let result = ensure_destination_file_exists(&testfile); + assert!(result.is_ok()); + + assert!(testfile.is_file()); + } #[test] fn test_ensure_destination_exists() { let dir = tempdir().expect("failed to create tmpdir"); From 64aa56235557f5e5cc8094e9ff561138e8b27080 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 11 Aug 2021 15:20:25 +1000 Subject: [PATCH 4/5] agent: Correct mount point creation mount_storage() first makes sure the mount point for the storage volume exists. It uses fs::create_dir_all() in the case of 9p or virtiofs volumes otherwise ensure_destination_exists(). But.. ensure_destination_exists() boils down to an fs::create_dir_all() in most cases anyway. The only case it doesn't is for a bind fstype, where it creates a file instead of a directory. But, that's not correct anyway because we need to create either a file or a directory depending on the source of the bind mount, which ensure_destination_exists() doesn't know. The 9p/virtiofs paths also check if the mountpoint exists before calling fs::create_dir_all(), which is unnecessary (fs::create_dir_all already handles that case). mount_storage() does have the information to know what we need to create, so have it explicitly call ensure_destination_file_exists() for the bind mount to a non-directory case, and fs::create_dir_all() in all other cases. fixes #2390 Signed-off-by: David Gibson --- src/agent/src/mount.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 3bcc46a819..ba2cf5f6d2 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -439,17 +439,14 @@ fn mount_storage(logger: &Logger, storage: &Storage) -> Result<()> { return Ok(()); } - match storage.fstype.as_str() { - DRIVER_9P_TYPE | DRIVER_VIRTIOFS_TYPE => { - let dest_path = Path::new(storage.mount_point.as_str()); - if !dest_path.exists() { - fs::create_dir_all(dest_path).context("Create mount destination failed")?; - } - } - _ => { - ensure_destination_exists(storage.mount_point.as_str(), storage.fstype.as_str())?; - } + let mount_path = Path::new(&storage.mount_point); + let src_path = Path::new(&storage.source); + if storage.fstype == "bind" && !src_path.is_dir() { + ensure_destination_file_exists(mount_path) + } else { + fs::create_dir_all(mount_path).map_err(anyhow::Error::from) } + .context("Could not create mountpoint")?; let options_vec = storage.options.to_vec(); let options_vec = options_vec.iter().map(String::as_str).collect(); From 9d3cd9841ff90d6aa0ff421f84be5d19d9ef3a55 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 11 Aug 2021 15:25:01 +1000 Subject: [PATCH 5/5] agent/mount: Remove unused ensure_destination_exists() The only remaining callers of ensure_destination_exists() are in its own unit tests. So, just remove it. Signed-off-by: David Gibson --- src/agent/src/mount.rs | 50 ------------------------------------------ 1 file changed, 50 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index ba2cf5f6d2..1c2b2dab23 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -787,21 +787,6 @@ fn ensure_destination_file_exists(path: &Path) -> Result<()> { Ok(()) } -// ensure_destination_exists will recursively create a given mountpoint. If directories -// are created, their permissions are initialized to mountPerm(0755) -#[instrument] -fn ensure_destination_exists(destination: &str, fs_type: &str) -> Result<()> { - let d = Path::new(destination); - - if fs_type != "bind" || d.is_dir() { - fs::create_dir_all(d).context(format!("create dir all {:?}", d))?; - } else { - ensure_destination_file_exists(d)?; - } - - Ok(()) -} - #[instrument] fn parse_options(option_list: Vec) -> HashMap { let mut options = HashMap::new(); @@ -821,7 +806,6 @@ fn parse_options(option_list: Vec) -> HashMap { mod tests { use super::*; use crate::{skip_if_not_root, skip_loop_if_not_root, skip_loop_if_root}; - use std::fs::metadata; use std::fs::File; use std::fs::OpenOptions; use std::io::Write; @@ -1401,38 +1385,4 @@ mod tests { assert!(testfile.is_file()); } - #[test] - fn test_ensure_destination_exists() { - let dir = tempdir().expect("failed to create tmpdir"); - - let mut testfile = dir.into_path(); - testfile.push("testfile"); - - let result = ensure_destination_exists(testfile.to_str().unwrap(), "bind"); - - assert!(result.is_ok()); - assert!(testfile.exists()); - - let result = ensure_destination_exists(testfile.to_str().unwrap(), "bind"); - assert!(result.is_ok()); - - let meta = metadata(testfile).unwrap(); - - assert!(meta.is_file()); - - let dir = tempdir().expect("failed to create tmpdir"); - let mut testdir = dir.into_path(); - testdir.push("testdir"); - - let result = ensure_destination_exists(testdir.to_str().unwrap(), "ext4"); - assert!(result.is_ok()); - assert!(testdir.exists()); - - let result = ensure_destination_exists(testdir.to_str().unwrap(), "ext4"); - assert!(result.is_ok()); - - //let meta = metadata(testdir.to_str().unwrap()).unwrap(); - let meta = metadata(testdir).unwrap(); - assert!(meta.is_dir()); - } }