Skip to content

Commit

Permalink
feat(jailer): honor solitary --parent-cgroup option on cgroupsv2
Browse files Browse the repository at this point in the history
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: firecracker-microvm#4287

Signed-off-by: Pablo Barbáchano <pablob@amazon.com>
  • Loading branch information
pb8o committed Dec 14, 2023
1 parent 783f821 commit 27a19fe
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 4 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
2 changes: 2 additions & 0 deletions docs/jailer.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ jailer --id <id> \
the jailer will write all cgroup parameters specified through `--cgroup` in
`/sys/fs/cgroup/<controller_name>/all_uvms/external_uvms/<id>`. By default, the
parent cgroup is `exec-file`.
If there are no `--cgroup` parameters specified and `--group-version=2` was
passed, then the jailer will move the process to the specified cgroup.
- `cgroup-version` is used to select which type of cgroup hierarchy to use for
the creation of cgroups. The default value is "1" which means that cgroups
specified with the `cgroup` argument will be created within a v1 hierarchy.
Expand Down
10 changes: 10 additions & 0 deletions src/jailer/src/cgroup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
17 changes: 15 additions & 2 deletions src/jailer/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,11 +233,24 @@ impl Env {
.parse::<u8>()
.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: <cgroup_controller>.<cgroup_property>=<value>,...
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() {
Expand Down
4 changes: 2 additions & 2 deletions src/jailer/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Expand All @@ -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}")]
Expand Down

0 comments on commit 27a19fe

Please sign in to comment.