Skip to content

fix: compute jailer cpu time without underflow #5034

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 13, 2025
Merged

fix: compute jailer cpu time without underflow #5034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
344aa3a
refactor: derive `Debug` for `jailer::env::Env`
roypat Feb 10, 2025
95858fb
fix: compute jailer cpu time without underflow
roypat Feb 10, 2025
e725e3b
fix: stop double counting jailer time in process_startup_time_cpu_us
roypat Feb 10, 2025
9895cc9
refactor: clarify computation of jailer_cpu_time_us
roypat Feb 10, 2025
c9b5b16
Merge branch 'main' into fix-jailer-underflow
roypat Feb 11, 2025
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ and this project adheres to
balloon device is inflated post UFFD-backed snapshot restore, Firecracker now
causes `remove` UFFD messages to be sent to the UFFD handler. Previously, no
such message would be sent.
- [#5034](https://github.com/firecracker-microvm/firecracker/pull/5034): Fix an
integer underflow in the jailer when computing the value it passes to
Firecracker's `--parent-cpu-time-us` values, which caused development builds
of Firecracker to crash (but production builds were unaffected as underflows
do not panic in release mode).

## [1.10.1]

Expand Down
43 changes: 12 additions & 31 deletions src/jailer/src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@

use std::ffi::{CString, OsString};
use std::fs::{self, canonicalize, read_to_string, File, OpenOptions, Permissions};
use std::io;
use std::io::Write;
use std::os::unix::fs::PermissionsExt;
use std::os::unix::io::AsRawFd;
use std::os::unix::process::CommandExt;
use std::path::{Component, Path, PathBuf};
use std::process::{exit, id, Command, Stdio};
use std::{fmt, io};

use utils::arg_parser::UtilsArgParserError::MissingValue;
use utils::time::{get_time_us, ClockType};
Expand Down Expand Up @@ -114,6 +114,7 @@
NotFound,
}

#[derive(Debug)]
pub struct Env {
id: String,
chroot_dir: PathBuf,
Expand All @@ -132,26 +133,6 @@
uffd_dev_minor: Option<u32>,
}

impl fmt::Debug for Env {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Env")
.field("id", &self.id)
.field("chroot_dir", &self.chroot_dir)
.field("exec_file_path", &self.exec_file_path)
.field("uid", &self.uid)
.field("gid", &self.gid)
.field("netns", &self.netns)
.field("daemonize", &self.daemonize)
.field("new_pid_ns", &self.new_pid_ns)
.field("start_time_us", &self.start_time_us)
.field("jailer_cpu_time_us", &self.jailer_cpu_time_us)
.field("extra_args", &self.extra_args)
.field("cgroups", &self.cgroup_conf)
.field("resource_limits", &self.resource_limits)
.finish()
}
}

impl Env {
pub fn new(
arguments: &arg_parser::Arguments,
Expand Down Expand Up @@ -495,7 +476,10 @@
Command::new(chroot_exec_file)
.args(["--id", &self.id])
.args(["--start-time-us", &self.start_time_us.to_string()])
.args(["--start-time-cpu-us", &self.start_time_cpu_us.to_string()])
.args([
"--start-time-cpu-us",
&get_time_us(ClockType::ProcessCpu).to_string(),
])

Check warning on line 482 in src/jailer/src/env.rs

View check run for this annotation

Codecov / codecov/patch

src/jailer/src/env.rs#L479-L482 

Added lines #L479 - L482 were not covered by tests
.args(["--parent-cpu-time-us", &self.jailer_cpu_time_us.to_string()])
.stdin(Stdio::inherit())
.stdout(Stdio::inherit())
Expand Down Expand Up @@ -661,11 +645,10 @@
self.mknod_and_own_dev(DEV_UFFD_PATH, DEV_UFFD_MAJOR, minor)?;
}

self.jailer_cpu_time_us = get_time_us(ClockType::ProcessCpu) - self.start_time_cpu_us;

Check warning on line 648 in src/jailer/src/env.rs

View check run for this annotation

Codecov / codecov/patch

src/jailer/src/env.rs#L648 

Added line #L648 was not covered by tests

// Daemonize before exec, if so required (when the dev_null variable != None).
if let Some(dev_null) = dev_null {
// Meter CPU usage before fork()
self.jailer_cpu_time_us = get_time_us(ClockType::ProcessCpu);

// We follow the double fork method to daemonize the jailer referring to
// https://0xjet.github.io/3OHA/2022/04/11/post.html
// setsid() will fail if the calling process is a process group leader.
Expand All @@ -688,7 +671,7 @@
.into_empty_result()
.map_err(JailerError::SetSid)?;

// Meter CPU usage before fork()
// Meter CPU usage after first fork()
self.jailer_cpu_time_us += get_time_us(ClockType::ProcessCpu);

// Daemons should not have controlling terminals.
Expand All @@ -712,12 +695,10 @@
dup2(dev_null.as_raw_fd(), STDIN_FILENO)?;
dup2(dev_null.as_raw_fd(), STDOUT_FILENO)?;
dup2(dev_null.as_raw_fd(), STDERR_FILENO)?;
}

// Compute jailer's total CPU time up to the current time.
self.jailer_cpu_time_us += get_time_us(ClockType::ProcessCpu) - self.start_time_cpu_us;
// Reset process start time.
self.start_time_cpu_us = 0;
// Meter CPU usage after second fork()
self.jailer_cpu_time_us += get_time_us(ClockType::ProcessCpu);
}

Check warning on line 701 in src/jailer/src/env.rs

View check run for this annotation

Codecov / codecov/patch

src/jailer/src/env.rs#L700-L701 

Added lines #L700 - L701 were not covered by tests

// If specified, exec the provided binary into a new PID namespace.
if self.new_pid_ns {
Expand Down