vmm/sys_util: refactor signal handling

* added a single function in vmm that installs all relevant
  signal handlers;
* left signal handler functions in vmm (currently only for SIGSYS);
* moved signal handling logic to sys_util;
* split sys_util's register_signal_handler into 2 separate functions,
  one to be called exclusively for vCPUs and a second general purpose
  one. The vCPU-specific one doen't change the signal mask and
  alters the signal number, forcing it between SIGRTMIN and SIGRTMAX.
  The general purpose one is setup_sigsys_handler renamed, and will
  morph into a generic one in a following commit.

Signed-off-by: Alexandra Iordache <[email protected]>

Author: Alexandra Iordache

src/main.rs

@@ -30,6 +30,7 @@ use api_server::{ApiServer, Error};
 use fc_util::validators::validate_instance_id;
 use logger::{Metric, LOGGER, METRICS};
 use mmds::MMDS;
+use vmm::signal_handler::register_signal_handlers;
 use vmm::vmm_config::instance_info::{InstanceInfo, InstanceState};
 
 const DEFAULT_API_SOCK_PATH: &str = "/tmp/firecracker.socket";
@@ -40,11 +41,10 @@ fn main() {
         .preinit(Some(DEFAULT_INSTANCE_ID.to_string()))
         .expect("Failed to register logger");
 
-    if let Err(e) = vmm::setup_sigsys_handler() {
-        error!("Failed to register signal handler: {}", e);
+    if let Err(e) = register_signal_handlers() {
+        error!("Failed to register signal handlers: {}", e);
         process::exit(i32::from(vmm::FC_EXIT_CODE_GENERIC_ERROR));
     }
-
     // Start firecracker by setting up a panic hook, which will be called before
     // terminating as we're building with panic = "abort".
     // It's worth noting that the abort is caused by sending a SIG_ABORT signal to the process.

sys_util/src/signal.rs

@@ -7,34 +7,17 @@
 
 use super::SyscallReturnCode;
 use libc::{
-    c_int, c_void, pthread_kill, pthread_t, sigaction, siginfo_t, EINVAL, SA_SIGINFO, SIGHUP,
-    SIGSYS,
+    c_int, c_void, pthread_kill, pthread_t, sigaction, sigfillset, siginfo_t, sigset_t, EINVAL,
 };
 use std::io;
 use std::mem;
 use std::os::unix::thread::JoinHandleExt;
+use std::ptr::null_mut;
 use std::thread::JoinHandle;
 
-type SiginfoHandler = extern "C" fn(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) -> ();
-
-pub enum SignalHandler {
-    Siginfo(SiginfoHandler),
-    // TODO add a`SimpleHandler` when `libc` adds `sa_handler` support to `sigaction`.
-}
-
-/// Fills a `sigaction` structure from of the signal handler.
-impl Into<sigaction> for SignalHandler {
-    fn into(self) -> sigaction {
-        let mut act: sigaction = unsafe { mem::zeroed() };
-        match self {
-            SignalHandler::Siginfo(function) => {
-                act.sa_flags = SA_SIGINFO;
-                act.sa_sigaction = function as *const () as usize;
-            }
-        }
-        act
-    }
-}
+/// Type that represents a signal handler function.
+pub type SignalHandler =
+    extern "C" fn(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) -> ();
 
 extern "C" {
     fn __libc_current_sigrtmin() -> c_int;
@@ -55,36 +38,47 @@ fn SIGRTMAX() -> c_int {
 
 /// Verifies that a signal number is valid.
 ///
-/// VCPU signals need to have values enclosed within the OS limits for realtime signals,
-/// and the remaining ones need to be between the minimum (SIGHUP) and maximum (SIGSYS) values.
+/// VCPU signals need to have values enclosed within the OS limits for realtime signals.
 ///
 /// Will return Ok(num) or Err(EINVAL).
-pub fn validate_signal_num(num: c_int, for_vcpu: bool) -> io::Result<c_int> {
-    if for_vcpu {
-        let actual_num = num + SIGRTMIN();
-        if actual_num <= SIGRTMAX() {
-            return Ok(actual_num);
-        }
-    } else if SIGHUP <= num && num <= SIGSYS {
-        return Ok(num);
+pub fn validate_vcpu_signal_num(num: c_int) -> io::Result<c_int> {
+    let actual_num = num + SIGRTMIN();
+    if actual_num <= SIGRTMAX() {
+        Ok(actual_num)
+    } else {
+        Err(io::Error::from_raw_os_error(EINVAL))
     }
-    Err(io::Error::from_raw_os_error(EINVAL))
 }
 
-/// Registers `handler` as the signal handler of signum `num`.
-///
-/// Uses `sigaction` to register the handler.
+/// Registers `handler` as the vCPU's signal handler of signum `num`.
 ///
 /// This is considered unsafe because the given handler will be called asynchronously, interrupting
 /// whatever the thread was doing and therefore must only do async-signal-safe operations.
-pub unsafe fn register_signal_handler(
-    num: i32,
-    handler: SignalHandler,
-    for_vcpu: bool,
-) -> io::Result<()> {
-    let num = validate_signal_num(num, for_vcpu)?;
-    let act: sigaction = handler.into();
-    SyscallReturnCode(sigaction(num, &act, ::std::ptr::null_mut())).into_empty_result()
+pub unsafe fn register_vcpu_signal_handler(num: i32, handler: SignalHandler) -> io::Result<()> {
+    let num = validate_vcpu_signal_num(num)?;
+    // Safe, because this is a POD struct.
+    let mut sigact: sigaction = mem::zeroed();
+    sigact.sa_flags = libc::SA_SIGINFO;
+    sigact.sa_sigaction = handler as usize;
+    SyscallReturnCode(sigaction(num, &sigact, null_mut())).into_empty_result()
+}
+
+/// Registers the specified signal handler for `SIGSYS`.
+pub fn register_sigsys_handler(sigsys_handler: SignalHandler) -> Result<(), io::Error> {
+    // Safe, because this is a POD struct.
+    let mut sigact: sigaction = unsafe { mem::zeroed() };
+    sigact.sa_flags = libc::SA_SIGINFO;
+    sigact.sa_sigaction = sigsys_handler as usize;
+
+    // We set all the bits of sa_mask, so all signals are blocked on the current thread while the
+    // SIGSYS handler is executing. Safe because the parameter is valid and we check the return
+    // value.
+    if unsafe { sigfillset(&mut sigact.sa_mask as *mut sigset_t) } < 0 {
+        return Err(io::Error::last_os_error());
+    }
+
+    // Safe because the parameters are valid and we check the return value.
+    unsafe { SyscallReturnCode(sigaction(libc::SIGSYS, &sigact, null_mut())).into_empty_result() }
 }
 
 /// Trait for threads that can be signalled via `pthread_kill`.
@@ -101,7 +95,7 @@ pub unsafe trait Killable {
     ///
     /// The value of `num + SIGRTMIN` must not exceed `SIGRTMAX`.
     fn kill(&self, num: i32) -> io::Result<()> {
-        let num = validate_signal_num(num, true)?;
+        let num = validate_vcpu_signal_num(num)?;
 
         // Safe because we ensure we are using a valid pthread handle, a valid signal number, and
         // check the return result.
@@ -119,7 +113,6 @@ unsafe impl<T> Killable for JoinHandle<T> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use libc;
     use std::thread;
     use std::time::Duration;
 
@@ -135,25 +128,9 @@ mod tests {
     fn test_register_signal_handler() {
         unsafe {
             // testing bad value
-            assert!(register_signal_handler(
-                SIGRTMAX(),
-                SignalHandler::Siginfo(handle_signal),
-                true
-            )
-            .is_err());
-            format!(
-                "{:?}",
-                register_signal_handler(SIGRTMAX(), SignalHandler::Siginfo(handle_signal), true)
-            );
-            assert!(
-                register_signal_handler(0, SignalHandler::Siginfo(handle_signal), true).is_ok()
-            );
-            assert!(register_signal_handler(
-                libc::SIGSYS,
-                SignalHandler::Siginfo(handle_signal),
-                false
-            )
-            .is_ok());
+            assert!(register_vcpu_signal_handler(SIGRTMAX(), handle_signal).is_err());
+            assert!(register_vcpu_signal_handler(0, handle_signal).is_ok());
+            assert!(register_sigsys_handler(handle_signal).is_ok());
         }
     }
 
@@ -168,7 +145,7 @@ mod tests {
         // be brought down when the signal is received, as part of the default behaviour. Signal
         // handlers are global, so we install this before starting the thread.
         unsafe {
-            register_signal_handler(0, SignalHandler::Siginfo(handle_signal), true)
+            register_vcpu_signal_handler(0, handle_signal)
                 .expect("failed to register vcpu signal handler");
         }
 

vmm/src/lib.rs

@@ -40,7 +40,7 @@ extern crate sys_util;
 pub mod default_syscalls;
 mod device_manager;
 /// Signal handling utilities.
-mod signal_handler;
+pub mod signal_handler;
 /// Wrappers over structures used to configure the VMM.
 pub mod vmm_config;
 mod vstate;
@@ -77,7 +77,6 @@ use memory_model::{GuestAddress, GuestMemory};
 use net_util::TapError;
 #[cfg(target_arch = "aarch64")]
 use serde_json::Value;
-pub use signal_handler::setup_sigsys_handler;
 use sys_util::{EventFd, Terminal};
 use vmm_config::boot_source::{BootSourceConfig, BootSourceConfigError};
 use vmm_config::drive::{BlockDeviceConfig, BlockDeviceConfigs, DriveError};

vmm/src/signal_handler.rs

@@ -5,13 +5,12 @@ extern crate logger;
 extern crate sys_util;
 
 use std::io;
-use std::mem;
-use std::ptr::null_mut;
 use std::result::Result;
 
-use libc::{sigaction, sigfillset, sigset_t};
+use libc::{_exit, c_int, c_void, siginfo_t, SIGSYS};
 
 use logger::{Metric, LOGGER, METRICS};
+use sys_util::register_sigsys_handler;
 
 // The offset of `si_syscall` (offending syscall identifier) within the siginfo structure
 // expressed as an `(u)int*`.
@@ -22,44 +21,15 @@ const SI_OFF_SYSCALL: isize = 6;
 
 const SYS_SECCOMP_CODE: i32 = 1;
 
-// This no longer relies on sys_util::register_signal_handler(), which is a lot weirder than it
-// should be (at least for this use case). Also, we want to change the sa_mask field of the
-// sigaction struct.
-/// Sets up the specified signal handler for `SIGSYS`.
-pub fn setup_sigsys_handler() -> Result<(), io::Error> {
-    // Safe, because this is a POD struct.
-    let mut sigact: sigaction = unsafe { mem::zeroed() };
-    sigact.sa_flags = libc::SA_SIGINFO;
-    sigact.sa_sigaction = sigsys_handler as usize;
-
-    // We set all the bits of sa_mask, so all signals are blocked on the current thread while the
-    // SIGSYS handler is executing. Safe because the parameter is valid and we check the return
-    // value.
-    if unsafe { sigfillset(&mut sigact.sa_mask as *mut sigset_t) } < 0 {
-        return Err(io::Error::last_os_error());
-    }
-
-    // Safe because the parameters are valid and we check the return value.
-    if unsafe { sigaction(libc::SIGSYS, &sigact, null_mut()) } < 0 {
-        return Err(io::Error::last_os_error());
-    }
-
-    Ok(())
-}
-
-extern "C" fn sigsys_handler(
-    num: libc::c_int,
-    info: *mut libc::siginfo_t,
-    _unused: *mut libc::c_void,
-) {
+extern "C" fn sigsys_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) {
     // Safe because we're just reading some fields from a supposedly valid argument.
     let si_signo = unsafe { (*info).si_signo };
     let si_code = unsafe { (*info).si_code };
 
     // Sanity check. The condition should never be true.
-    if num != si_signo || num != libc::SIGSYS || si_code != SYS_SECCOMP_CODE as i32 {
+    if num != si_signo || num != SIGSYS || si_code != SYS_SECCOMP_CODE as i32 {
         // Safe because we're terminating the process anyway.
-        unsafe { libc::_exit(i32::from(super::FC_EXIT_CODE_UNEXPECTED_ERROR)) };
+        unsafe { _exit(i32::from(super::FC_EXIT_CODE_UNEXPECTED_ERROR)) };
     }
 
     // Other signals which might do async unsafe things incompatible with the rest of this
@@ -79,10 +49,18 @@ extern "C" fn sigsys_handler(
     // running unit tests.
     #[cfg(not(test))]
     unsafe {
-        libc::_exit(i32::from(super::FC_EXIT_CODE_BAD_SYSCALL))
+        _exit(i32::from(super::FC_EXIT_CODE_BAD_SYSCALL))
     };
 }
 
+/// Registers all the required signal handlers.
+///
+/// Custom handlers are installed for: `SIGSYS`.
+///
+pub fn register_signal_handlers() -> Result<(), io::Error> {
+    register_sigsys_handler(sigsys_handler)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -120,7 +98,7 @@ mod tests {
 
     #[test]
     fn test_signal_handler() {
-        assert!(setup_sigsys_handler().is_ok());
+        assert!(register_signal_handlers().is_ok());
 
         let filter = SeccompFilter::new(
             vec![

vmm/src/vstate.rs

@@ -464,7 +464,7 @@ mod tests {
     use super::*;
 
     use libc::{c_int, c_void, siginfo_t};
-    use sys_util::{register_signal_handler, Killable, SignalHandler};
+    use sys_util::{register_vcpu_signal_handler, Killable};
 
     // Auxiliary function being used throughout the tests.
     fn setup_vcpu() -> (Vm, Vcpu) {
@@ -704,7 +704,7 @@ mod tests {
         // be brought down when the signal is received, as part of the default behaviour. Signal
         // handlers are global, so we install this before starting the thread.
         unsafe {
-            register_signal_handler(signum, SignalHandler::Siginfo(handle_signal), true)
+            register_vcpu_signal_handler(signum, handle_signal)
                 .expect("failed to register vcpu signal handler");
         }