From b6feae0ace5d2e04bc85a3562650b579e7c44cb0 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Wed, 22 Mar 2023 22:22:46 +0000 Subject: [PATCH] vmm: only touch the tty flags if it's being used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/main.rs | 9 --------- vmm/src/device_manager.rs | 41 ++++++++++++++++++++++++++++++++++----- vmm/src/lib.rs | 14 +++++++++---- vmm/src/vm.rs | 30 +++++----------------------- 4 files changed, 51 insertions(+), 43 deletions(-) diff --git a/src/main.rs b/src/main.rs index 3acd89127..99ec060a2 100644 --- a/src/main.rs +++ b/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); diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 1b0f8c2b0..96e610c55 100644 --- a/vmm/src/device_manager.rs +++ b/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>, + // Are any devices using the tty? + on_tty: Option>, + // Interrupt controller #[cfg(target_arch = "x86_64")] interrupt_controller: Option>>, @@ -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, console_pty: Option, console_resize_pipe: Option, + on_tty: Arc, ) -> 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, console_pty: Option, resize_pipe: Option, + on_tty: &Arc, ) -> DeviceManagerResult>> { 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, console_pty: Option, console_resize_pipe: Option, + on_tty: &Arc, ) -> DeviceManagerResult> { let serial_config = self.config.lock().unwrap().serial.clone(); let serial_writer: Option> = 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(); + } + } } } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index c881cfd24..90a574339 100644 --- a/vmm/src/lib.rs +++ b/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, threads: Vec>, + on_tty: Arc, } 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, 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| { diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index a61baaee6..767c0a8df 100644 --- a/vmm/src/vm.rs +++ b/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>, device_manager: Arc>, config: Arc>, - on_tty: bool, state: RwLock, cpu_manager: Arc>, memory_manager: Arc>, @@ -468,6 +464,7 @@ impl Vm { serial_pty: Option, console_pty: Option, console_resize_pipe: Option, + on_tty: Arc, snapshot: Option, ) -> Result { 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, console_pty: Option, console_resize_pipe: Option, + on_tty: Arc, snapshot: Option, source_url: Option<&str>, prefault: Option, @@ -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(()) }