From 311a027aae5e9495a3f214fdeee5f3b9afdda1da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Barb=C3=A1chano?= Date: Fri, 8 Dec 2023 12:08:47 +0100 Subject: [PATCH] fix(jailer): honor solitary --parent-cgroup option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we specify `--parent-cgroup` without any `--cgroup` option, the jailer doesn't do anything, though a user specifying it could reasonably expect it to move the process under that cgroup. We could just error in that situation, and expect something else to launch the jailer process in a cgroup, but we risk breaking anybody that is already using it that way. Instead, we move the process to the `--parent-cgroup` since it's the most intuitive, although the specified `--parent-cgroup` is not a parent in that case. Link: https://github.com/firecracker-microvm/firecracker/issues/4287 Signed-off-by: Pablo Barbáchano --- CHANGELOG.md | 3 +++ src/jailer/src/cgroup.rs | 10 ++++++++++ src/jailer/src/env.rs | 17 +++++++++++++++-- src/jailer/src/main.rs | 4 ++-- 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3dee91387de..7b6d867c7db9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- [#4309](https://github.com/firecracker-microvm/firecracker/pull/4309): The + jailer's option `--parent-cgroup` will move the process to that cgroup if no + `cgroup` options are provided. - Simplified and clarified the removal policy of deprecated API elements to follow semantic versioning 2.0.0. For more information, please refer to [this GitHub discussion](https://github.com/firecracker-microvm/firecracker/discussions/4135). diff --git a/src/jailer/src/cgroup.rs b/src/jailer/src/cgroup.rs index 349bf0183ae8..ceefdecea40a 100644 --- a/src/jailer/src/cgroup.rs +++ b/src/jailer/src/cgroup.rs @@ -161,6 +161,16 @@ impl CgroupBuilder { } } } + + // Returns the path to the root of the hierarchy + pub fn get_v2_hierarchy_path(&mut self) -> Result<&PathBuf, JailerError> { + match self.hierarchies.entry("unified".to_string()) { + Occupied(entry) => Ok(entry.into_mut()), + Vacant(_entry) => Err(JailerError::CgroupHierarchyMissing( + "cgroupsv2 hierarchy missing".to_string(), + )), + } + } } #[derive(Debug)] diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs index d7a6613075d8..14cf10e24e77 100644 --- a/src/jailer/src/env.rs +++ b/src/jailer/src/env.rs @@ -233,11 +233,24 @@ impl Env { .parse::() .map_err(|_| JailerError::CgroupInvalidVersion(cgroup_ver.to_string()))?; - let mut cgroup_builder = None; + let cgroups_args: &[String] = arguments.multiple_values("cgroup").unwrap_or_default(); + + // If the --parent-cgroup exists, and we have no other cgroups, + // then the intent is to move the process to that cgroup. + // Only applies to cgroupsv2 since it's a unified hierarchy + if cgroups_args.is_empty() && cgroup_ver == 2 { + let mut builder = CgroupBuilder::new(cgroup_ver)?; + let cg_parent = builder.get_v2_hierarchy_path()?.join(parent_cgroup); + let cg_parent_procs = cg_parent.join("cgroup.procs"); + if cg_parent.exists() { + fs::write(cg_parent_procs, std::process::id().to_string()) + .map_err(|_| JailerError::CgroupWrite(io::Error::last_os_error()))?; + } + } // cgroup format: .=,... if let Some(cgroups_args) = arguments.multiple_values("cgroup") { - let builder = cgroup_builder.get_or_insert(CgroupBuilder::new(cgroup_ver)?); + let mut builder = CgroupBuilder::new(cgroup_ver)?; for cg in cgroups_args { let aux: Vec<&str> = cg.split('=').collect(); if aux.len() != 2 || aux[1].is_empty() { diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs index d9c969b2bccb..0a63ba64f4af 100644 --- a/src/jailer/src/main.rs +++ b/src/jailer/src/main.rs @@ -32,8 +32,6 @@ pub enum JailerError { CgroupLineNotFound(String, String), #[error("Cgroup invalid file: {0}")] CgroupInvalidFile(String), - #[error("Expected value {0} for {2}. Current value: {1}")] - CgroupWrite(String, String, String), #[error("Invalid format for cgroups: {0}")] CgroupFormat(String), #[error("Hierarchy not found: {0}")] @@ -44,6 +42,8 @@ pub enum JailerError { CgroupInvalidVersion(String), #[error("Parent cgroup path is invalid. Path should not be absolute or contain '..' or '.'")] CgroupInvalidParentPath(), + #[error("Failed to write to cgroups file: {0}")] + CgroupWrite(io::Error), #[error("Failed to change owner for {0:?}: {1}")] ChangeFileOwner(PathBuf, io::Error), #[error("Failed to chdir into chroot directory: {0}")]