vmm: only touch the tty flags if it's being used

When neither serial nor console are connected to the tty,
cloud-hypervisor shouldn't touch the tty at all.  One way in which
this is annoying is that if I am running cloud-hypervisor without it
using my terminal, I expect to be able to suspend it with ^Z like any
other process, but that doesn't work if it's put the terminal into raw
mode.

Instead of putting the tty into raw mode when a VM is created or
restored, do it when a serial or console device is created.  Since we
now know it can't be put into raw mode until the Vm object is created,
we can move setting it back to canon mode into the drop handler for
that object, which should always be run in normal operation.  We still
also put the tty into canon mode in the SIGTERM / SIGINT handler, but
check whether the tty was actually used, rather than whether stdin is
a tty.  This requires passing on_tty around as an atomic boolean.

I explored more of an abstraction over the tty — having an object that
encapsulated stdout and put the tty into raw mode when initialized and
into canon mode when dropped — but it wasn't practical, mostly due to
the special requirements of the signal handler.  I also investigated
whether the SIGWINCH listener process could be used here, which I
think would have worked but I'm hesitant to involve it in serial
handling as well as conosle handling.

There's no longer a check for whether the file descriptor is a tty
before setting it into canon mode — it's redundant, because if it's
not a tty it just won't respond to the ioctl.

Tested by shutting down through the API, SIGTERM, and an error
injected after setting raw mode.

Signed-off-by: Alyssa Ross <hi@alyssa.is>

src/main.rs

@@ -21,7 +21,6 @@ use thiserror::Error;
 use vmm::config;
 use vmm_sys_util::eventfd::EventFd;
 use vmm_sys_util::signal::block_signal;
-use vmm_sys_util::terminal::Terminal;
 
 #[cfg(feature = "dhat-heap")]
 #[global_allocator]
@@ -586,14 +585,6 @@ fn main() {
         }
     };
 
-    // SAFETY: trivially safe
-    let on_tty = unsafe { libc::isatty(libc::STDIN_FILENO) } != 0;
-    if on_tty {
-        // Don't forget to set the terminal in canonical mode
-        // before to exit.
-        std::io::stdin().lock().set_canon_mode().unwrap();
-    }
-
     #[cfg(feature = "dhat-heap")]
     drop(_profiler);
 

vmm/src/device_manager.rs

@@ -70,6 +70,7 @@ use std::os::unix::fs::OpenOptionsExt;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 use std::path::PathBuf;
 use std::result;
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::Instant;
 use tracer::trace_scoped;
@@ -100,6 +101,7 @@ use vm_migration::{
 use vm_virtio::AccessPlatform;
 use vm_virtio::VirtioDeviceType;
 use vmm_sys_util::eventfd::EventFd;
+use vmm_sys_util::terminal::Terminal;
 
 #[cfg(target_arch = "aarch64")]
 const MMIO_LEN: u64 = 0x1000;
@@ -831,6 +833,9 @@ pub struct DeviceManager {
     // pty foreground status,
     console_resize_pipe: Option<Arc<File>>,
 
+    // Are any devices using the tty?
+    on_tty: Option<Arc<AtomicBool>>,
+
     // Interrupt controller
     #[cfg(target_arch = "x86_64")]
     interrupt_controller: Option<Arc<Mutex<ioapic::Ioapic>>>,
@@ -1115,6 +1120,7 @@ impl DeviceManager {
             serial_manager: None,
             console_pty: None,
             console_resize_pipe: None,
+            on_tty: None,
             virtio_mem_devices: Vec::new(),
             #[cfg(target_arch = "aarch64")]
             gpio_device: None,
@@ -1162,6 +1168,7 @@ impl DeviceManager {
         serial_pty: Option<PtyPair>,
         console_pty: Option<PtyPair>,
         console_resize_pipe: Option<File>,
+        on_tty: Arc<AtomicBool>,
     ) -> DeviceManagerResult<()> {
         trace_scoped!("create_devices");
 
@@ -1223,7 +1230,9 @@ impl DeviceManager {
             serial_pty,
             console_pty,
             console_resize_pipe,
+            &on_tty,
         )?;
+        self.on_tty = Some(on_tty);
 
         if let Some(tpm) = self.config.clone().lock().unwrap().tpm.as_ref() {
             let tpm_dev = self.add_tpm_device(tpm.socket.clone())?;
@@ -1864,7 +1873,7 @@ impl DeviceManager {
         Ok(())
     }
 
-    fn set_raw_mode(&self, f: &mut File) -> vmm_sys_util::errno::Result<()> {
+    fn set_raw_mode(&self, f: &mut dyn AsRawFd) -> vmm_sys_util::errno::Result<()> {
         // SAFETY: FFI call. Variable t is guaranteed to be a valid termios from modify_mode.
         self.modify_mode(f.as_raw_fd(), |t| unsafe { cfmakeraw(t) })
     }
@@ -1888,6 +1897,7 @@ impl DeviceManager {
         virtio_devices: &mut Vec<MetaVirtioDevice>,
         console_pty: Option<PtyPair>,
         resize_pipe: Option<File>,
+        on_tty: &Arc<AtomicBool>,
     ) -> DeviceManagerResult<Option<Arc<virtio_devices::ConsoleResizer>>> {
         let console_config = self.config.lock().unwrap().console.clone();
         let endpoint = match console_config.mode {
@@ -1926,7 +1936,12 @@ impl DeviceManager {
                     return vmm_sys_util::errno::errno_result().map_err(DeviceManagerError::DupFd);
                 }
                 // SAFETY: stdout is valid and owned solely by us.
-                let stdout = unsafe { File::from_raw_fd(stdout) };
+                let mut stdout = unsafe { File::from_raw_fd(stdout) };
+
+                on_tty.store(true, Ordering::SeqCst);
+
+                // Make sure stdout is in raw mode, if it's a terminal.
+                let _ = self.set_raw_mode(&mut stdout);
 
                 // SAFETY: FFI call. Trivially safe.
                 if unsafe { libc::isatty(libc::STDOUT_FILENO) } == 1 {
@@ -2004,6 +2019,7 @@ impl DeviceManager {
         serial_pty: Option<PtyPair>,
         console_pty: Option<PtyPair>,
         console_resize_pipe: Option<File>,
+        on_tty: &Arc<AtomicBool>,
     ) -> DeviceManagerResult<Arc<Console>> {
         let serial_config = self.config.lock().unwrap().serial.clone();
         let serial_writer: Option<Box<dyn io::Write + Send>> = match serial_config.mode {
@@ -2025,7 +2041,12 @@ impl DeviceManager {
                 }
                 None
             }
-            ConsoleOutputMode::Tty => Some(Box::new(stdout())),
+            ConsoleOutputMode::Tty => {
+                let mut out = stdout();
+                on_tty.store(true, Ordering::SeqCst);
+                let _ = self.set_raw_mode(&mut out);
+                Some(Box::new(out))
+            }
             ConsoleOutputMode::Off | ConsoleOutputMode::Null => None,
         };
         if serial_config.mode != ConsoleOutputMode::Off {
@@ -2052,8 +2073,12 @@ impl DeviceManager {
             };
         }
 
-        let console_resizer =
-            self.add_virtio_console_device(virtio_devices, console_pty, console_resize_pipe)?;
+        let console_resizer = self.add_virtio_console_device(
+            virtio_devices,
+            console_pty,
+            console_resize_pipe,
+            on_tty,
+        )?;
 
         Ok(Arc::new(Console { console_resizer }))
     }
@@ -4631,5 +4656,11 @@ impl Drop for DeviceManager {
         for handle in self.virtio_devices.drain(..) {
             handle.virtio_device.lock().unwrap().shutdown();
         }
+
+        if let Some(ref on_tty) = self.on_tty {
+            if on_tty.load(Ordering::SeqCst) {
+                let _ = std::io::stdin().lock().set_canon_mode();
+            }
+        }
     }
 }

vmm/src/lib.rs

@@ -41,6 +41,7 @@ use std::os::unix::net::UnixListener;
 use std::os::unix::net::UnixStream;
 use std::panic::AssertUnwindSafe;
 use std::path::PathBuf;
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::mpsc::{Receiver, RecvError, SendError, Sender};
 use std::sync::{Arc, Mutex};
 use std::time::Instant;
@@ -422,12 +423,13 @@ pub struct Vmm {
     activate_evt: EventFd,
     signals: Option<Handle>,
     threads: Vec<thread::JoinHandle<()>>,
+    on_tty: Arc<AtomicBool>,
 }
 
 impl Vmm {
     pub const HANDLED_SIGNALS: [i32; 2] = [SIGTERM, SIGINT];
 
-    fn signal_handler(mut signals: Signals, on_tty: bool, exit_evt: &EventFd) {
+    fn signal_handler(mut signals: Signals, on_tty: Arc<AtomicBool>, exit_evt: &EventFd) {
         for sig in &Self::HANDLED_SIGNALS {
             unblock_signal(*sig).unwrap();
         }
@@ -437,7 +439,7 @@ impl Vmm {
                 SIGTERM | SIGINT => {
                     if exit_evt.write(1).is_err() {
                         // Resetting the terminal is usually done as the VMM exits
-                        if on_tty {
+                        if on_tty.load(Ordering::SeqCst) {
                             io::stdin()
                                 .lock()
                                 .set_canon_mode()
@@ -457,8 +459,7 @@ impl Vmm {
             Ok(signals) => {
                 self.signals = Some(signals.handle());
                 let exit_evt = self.exit_evt.try_clone().map_err(Error::EventFdClone)?;
-                // SAFETY: trivially safe
-                let on_tty = unsafe { libc::isatty(libc::STDIN_FILENO) } != 0;
+                let on_tty = Arc::clone(&self.on_tty);
 
                 let signal_handler_seccomp_filter = get_seccomp_filter(
                     &self.seccomp_action,
@@ -547,6 +548,7 @@ impl Vmm {
             activate_evt,
             signals: None,
             threads: vec![],
+            on_tty: Arc::new(AtomicBool::new(false)),
         })
     }
 
@@ -597,6 +599,7 @@ impl Vmm {
                         None,
                         None,
                         None,
+                        Arc::clone(&self.on_tty),
                         None,
                         None,
                         None,
@@ -695,6 +698,7 @@ impl Vmm {
             None,
             None,
             None,
+            Arc::clone(&self.on_tty),
             Some(snapshot),
             Some(source_url),
             Some(restore_cfg.prefault),
@@ -775,6 +779,7 @@ impl Vmm {
             serial_pty,
             console_pty,
             console_resize_pipe,
+            Arc::clone(&self.on_tty),
             None,
             None,
             None,
@@ -1284,6 +1289,7 @@ impl Vmm {
             None,
             None,
             None,
+            Arc::clone(&self.on_tty),
             Some(snapshot),
         )
         .map_err(|e| {

vmm/src/vm.rs

@@ -79,6 +79,7 @@ use std::mem::size_of;
 use std::num::Wrapping;
 use std::ops::Deref;
 use std::os::unix::net::UnixStream;
+use std::sync::atomic::AtomicBool;
 use std::sync::{Arc, Mutex, RwLock};
 use std::time::Instant;
 use std::{result, str, thread};
@@ -95,7 +96,6 @@ use vm_migration::{
 };
 use vmm_sys_util::eventfd::EventFd;
 use vmm_sys_util::sock_ctrl_msg::ScmSocket;
-use vmm_sys_util::terminal::Terminal;
 
 /// Errors associated with VM management
 #[derive(Debug, Error)]
@@ -138,9 +138,6 @@ pub enum Error {
     #[error("Error from device manager: {0:?}")]
     DeviceManager(DeviceManagerError),
 
-    #[error("Cannot setup terminal in raw mode: {0}")]
-    SetTerminalRaw(#[source] vmm_sys_util::errno::Error),
-
     #[error("Cannot spawn a signal handler thread: {0}")]
     SignalHandlerSpawn(#[source] io::Error),
 
@@ -434,7 +431,6 @@ pub struct Vm {
     threads: Vec<thread::JoinHandle<()>>,
     device_manager: Arc<Mutex<DeviceManager>>,
     config: Arc<Mutex<VmConfig>>,
-    on_tty: bool,
     state: RwLock<VmState>,
     cpu_manager: Arc<Mutex<cpu::CpuManager>>,
     memory_manager: Arc<Mutex<MemoryManager>>,
@@ -468,6 +464,7 @@ impl Vm {
         serial_pty: Option<PtyPair>,
         console_pty: Option<PtyPair>,
         console_resize_pipe: Option<File>,
+        on_tty: Arc<AtomicBool>,
         snapshot: Option<Snapshot>,
     ) -> Result<Self> {
         trace_scoped!("Vm::new_from_memory_manager");
@@ -589,12 +586,9 @@ impl Vm {
         device_manager
             .lock()
             .unwrap()
-            .create_devices(serial_pty, console_pty, console_resize_pipe)
+            .create_devices(serial_pty, console_pty, console_resize_pipe, on_tty)
             .map_err(Error::DeviceManager)?;
 
-        // SAFETY: trivially safe
-        let on_tty = unsafe { libc::isatty(libc::STDIN_FILENO) } != 0;
-
         #[cfg(feature = "tdx")]
         let kernel = config
             .lock()
@@ -636,7 +630,6 @@ impl Vm {
             initramfs,
             device_manager,
             config,
-            on_tty,
             threads: Vec::with_capacity(1),
             state: RwLock::new(vm_state),
             cpu_manager,
@@ -743,6 +736,7 @@ impl Vm {
         serial_pty: Option<PtyPair>,
         console_pty: Option<PtyPair>,
         console_resize_pipe: Option<File>,
+        on_tty: Arc<AtomicBool>,
         snapshot: Option<Snapshot>,
         source_url: Option<&str>,
         prefault: Option<bool>,
@@ -812,6 +806,7 @@ impl Vm {
             serial_pty,
             console_pty,
             console_resize_pipe,
+            on_tty,
             snapshot,
         )
     }
@@ -1903,17 +1898,6 @@ impl Vm {
         Ok(())
     }
 
-    fn setup_tty(&self) -> Result<()> {
-        if self.on_tty {
-            io::stdin()
-                .lock()
-                .set_raw_mode()
-                .map_err(Error::SetTerminalRaw)?;
-        }
-
-        Ok(())
-    }
-
     // Creates ACPI tables
     // In case of TDX being used, this is a no-op since the tables will be
     // created and passed when populating the HOB.
@@ -1967,8 +1951,6 @@ impl Vm {
         #[cfg(target_arch = "x86_64")]
         let rsdp_addr = self.create_acpi_tables();
 
-        self.setup_tty()?;
-
         // Load kernel synchronously or if asynchronous then wait for load to
         // finish.
         let entry_point = self.entry_point()?;
@@ -2081,8 +2063,6 @@ impl Vm {
             .start_restored_vcpus()
             .map_err(Error::CpuManager)?;
 
-        self.setup_tty()?;
-
         event!("vm", "restored");
         Ok(())
     }