Skip to content

Commit 68ab56e

Browse files
committed
fix(jailer): account time spent in the jailer more accurately
After da92bf6, the CPU time we got from get_time_us was much shorter, and that caused an underflow when calculating the jailer_cpu_time_us. That resulted in Firecracker getting called with a huge value for `--parent-cpu-time`. Before this fix: /firecracker [...] --parent-cpu-time-us 18446744073709551368 After this fix: /firecracker [...] --parent-cpu-time-us 4375 The new time is higher than before da92bf6, because a) that commit introduced a fork, which carries some overhead, and b) we now account for CPU time during forks. Fixes: da92bf6 Signed-off-by: Pablo Barbáchano <[email protected]>
1 parent d566ee7 commit 68ab56e

File tree

1 file changed

+13
-10
lines changed

1 file changed

+13
-10
lines changed

src/jailer/src/env.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -343,21 +343,12 @@ impl Env {
343343
}
344344

345345
fn exec_into_new_pid_ns(&mut self, chroot_exec_file: PathBuf) -> Result<(), JailerError> {
346-
// Compute jailer's total CPU time up to the current time.
347-
self.jailer_cpu_time_us =
348-
utils::time::get_time_us(utils::time::ClockType::ProcessCpu) - self.start_time_cpu_us;
349-
350346
// Duplicate the current process. The child process will belong to the previously created
351347
// PID namespace. The current process will not be moved into the newly created namespace,
352348
// but its first child will assume the role of init(1) in the new namespace.
353349
let pid = clone(std::ptr::null_mut(), libc::CLONE_NEWPID)?;
354350
match pid {
355-
0 => {
356-
// Reset process start time.
357-
self.start_time_cpu_us = 0;
358-
359-
Err(JailerError::Exec(self.exec_command(chroot_exec_file)))
360-
}
351+
0 => Err(JailerError::Exec(self.exec_command(chroot_exec_file))),
361352
child_pid => {
362353
// Save the PID of the process running the exec file provided
363354
// inside <chroot_exec_file>.pid file.
@@ -674,6 +665,9 @@ impl Env {
674665

675666
// Daemonize before exec, if so required (when the dev_null variable != None).
676667
if let Some(dev_null) = dev_null {
668+
// Meter CPU usage before fork()
669+
self.jailer_cpu_time_us = utils::time::get_time_us(utils::time::ClockType::ProcessCpu);
670+
677671
// We follow the double fork method to daemonize the jailer referring to
678672
// https://0xjet.github.io/3OHA/2022/04/11/post.html
679673
// setsid() will fail if the calling process is a process group leader.
@@ -696,6 +690,9 @@ impl Env {
696690
.into_empty_result()
697691
.map_err(JailerError::SetSid)?;
698692

693+
// Meter CPU usage before fork()
694+
self.jailer_cpu_time_us += utils::time::get_time_us(utils::time::ClockType::ProcessCpu);
695+
699696
// Daemons should not have controlling terminals.
700697
// If a daemon has a controlling terminal, it can receive signals
701698
// from it that might cause it to halt or exit unexpectedly.
@@ -719,6 +716,12 @@ impl Env {
719716
dup2(dev_null.as_raw_fd(), STDERR_FILENO)?;
720717
}
721718

719+
// Compute jailer's total CPU time up to the current time.
720+
self.jailer_cpu_time_us +=
721+
utils::time::get_time_us(utils::time::ClockType::ProcessCpu) - self.start_time_cpu_us;
722+
// Reset process start time.
723+
self.start_time_cpu_us = 0;
724+
722725
// If specified, exec the provided binary into a new PID namespace.
723726
if self.new_pid_ns {
724727
self.exec_into_new_pid_ns(chroot_exec_file)

0 commit comments

Comments
 (0)