Fix signal handling in VCPU threads

This commit addresses a number of issues with the way in which the
SIGRTMIN() + 0 signal is used to kick VCPU threads.  It

1. Moves the registration of the signal handler to the main thread.
   There's no need to register the handler once for each VCPU as
   there's one handler per process, rather than one per thread.
2. Ensures expect is not called in the VCPU thread before
   start_barrier.wait() is called.  In the current code,
   failure to register the signal handler causes crosvm to hang
   rather than to exit as the VCPU thread panics before calling
   start_barrier.wait().  The main thread then blocks forever while
   waiting on the barrier.
3. Uses the KVM_SET_SIGNAL_MASK ioctl to remove a race condition in
   the current code.  In the current code, a SIGRTMIN() + 0 signal,
   received during a vm exit, would be consumed before the next call
   to KVM_RUN, which would execute as normal and not be interrupted.
   This could delay the VM from stopping when requested to do so.
   Note that the new code doesn't unblock all signals during
   the call to KVM_RUN.  It only unblocks SIGRTMIN() + 0.  This is
   important as SIGCHILD is blocked at the start of run_config, and
   we probably don't want this unblocked periodically in each of the
   VCPU threads.

TEST=run crosvm and stop it in both single and multi-process mode.
BUG=none

Signed-off-by: Mark Ryan <mark.d.ryan@intel.com>
Change-Id: Ibda7d6220482aa11b2f5feee410d1d2b67a7e774
Reviewed-on: https://chromium-review.googlesource.com/1019443
Commit-Ready: Mark D Ryan <mark.d.ryan@intel.com>
Tested-by: Mark D Ryan <mark.d.ryan@intel.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>

1 files changed, 44 insertions, 7 deletions

diff --git a/src/linux.rs b/src/linux.rs
index d56f32d..71c7ae7 100644
--- a/src/linux.rs
+++ b/src/linux.rs
@@ -44,6 +44,7 @@ use aarch64::AArch64 as Arch;
 pub enum Error {
     BalloonDeviceNew(devices::virtio::BalloonError),
     BlockDeviceNew(sys_util::Error),
+    BlockSignal(sys_util::signal::Error),
     ChownWaylandRoot(sys_util::Error),
     CloneEventFd(sys_util::Error),
     Cmdline(kernel_cmdline::Error),
@@ -74,6 +75,7 @@ pub enum Error {
     RegisterIrqfd(sys_util::Error),
     RegisterNet(device_manager::Error),
     RegisterRng(device_manager::Error),
+    RegisterSignalHandler(sys_util::Error),
     RegisterVsock(device_manager::Error),
     RegisterWayland(device_manager::Error),
     RngDeviceNew(devices::virtio::RngError),
@@ -98,6 +100,7 @@ impl fmt::Display for Error {
         match self {
             &Error::BalloonDeviceNew(ref e) => write!(f, "failed to create balloon: {:?}", e),
             &Error::BlockDeviceNew(ref e) => write!(f, "failed to create block device: {:?}", e),
+            &Error::BlockSignal(ref e) => write!(f, "failed to block signal: {:?}", e),
             &Error::ChownWaylandRoot(ref e) => {
                 write!(f, "error chowning wayland root directory: {:?}", e)
             }
@@ -142,6 +145,9 @@ impl fmt::Display for Error {
             &Error::RegisterIrqfd(ref e) => write!(f, "error registering irqfd: {:?}", e),
             &Error::RegisterNet(ref e) => write!(f, "error registering net device: {:?}", e),
             &Error::RegisterRng(ref e) => write!(f, "error registering rng device: {:?}", e),
+            &Error::RegisterSignalHandler(ref e) => {
+                write!(f, "error registering signal handler: {:?}", e)
+            }
             &Error::RegisterVsock(ref e) => {
                 write!(f, "error registering virtual socket device: {:?}", e)
             }
@@ -459,6 +465,17 @@ fn setup_vcpu(kvm: &Kvm,
     Ok(vcpu)
 }
 
+fn setup_vcpu_signal_handler() -> Result<()> {
+    unsafe {
+        extern "C" fn handle_signal() {}
+        // Our signal handler does nothing and is trivially async signal safe.
+        register_signal_handler(SIGRTMIN() + 0, handle_signal)
+            .map_err(Error::RegisterSignalHandler)?;
+    }
+    block_signal(SIGRTMIN() + 0).map_err(Error::BlockSignal)?;
+    Ok(())
+}
+
 fn run_vcpu(vcpu: Vcpu,
             cpu_id: u32,
             start_barrier: Arc<Barrier>,
@@ -469,15 +486,30 @@ fn run_vcpu(vcpu: Vcpu,
     thread::Builder::new()
         .name(format!("crosvm_vcpu{}", cpu_id))
         .spawn(move || {
-            unsafe {
-                extern "C" fn handle_signal() {}
-                // Our signal handler does nothing and is trivially async signal safe.
-                register_signal_handler(SIGRTMIN() + 0, handle_signal)
-                    .expect("failed to register vcpu signal handler");
-            }
+            let mut sig_ok = true;
+            match get_blocked_signals() {
+                Ok(mut v) => {
+                    v.retain(|&x| x != SIGRTMIN() + 0);
+                    if let Err(e) = vcpu.set_signal_mask(&v) {
+                        error!(
+                            "Failed to set the KVM_SIGNAL_MASK for vcpu {} : {:?}",
+                            cpu_id, e
+                        );
+                        sig_ok = false;
+                    }
+                }
+                Err(e) => {
+                    error!(
+                        "Failed to retrieve signal mask for vcpu {} : {:?}",
+                        cpu_id, e
+                    );
+                    sig_ok = false;
+                }
+            };
 
             start_barrier.wait();
-            loop {
+
+            while sig_ok {
                 let run_res = vcpu.run();
                 match run_res {
                     Ok(run) => {
@@ -515,6 +547,10 @@ fn run_vcpu(vcpu: Vcpu,
                 if kill_signaled.load(Ordering::SeqCst) {
                     break;
                 }
+
+                // Try to clear the signal that we use to kick VCPU if it is
+                // pending before attempting to handle pause requests.
+                clear_signal(SIGRTMIN() + 0).expect("failed to clear pending signal");
             }
             exit_evt
                 .write(1)
@@ -745,6 +781,7 @@ pub fn run_config(cfg: Config) -> Result<()> {
                               &CString::new(cmdline).unwrap()).
         map_err(|e| Error::SetupSystemMemory(e))?;
 
+    setup_vcpu_signal_handler()?;
     for (cpu_id, vcpu) in vcpus.into_iter().enumerate() {
         let handle = run_vcpu(vcpu,
                               cpu_id as u32,