From 38a1b45783a66dacc0e9980f67e9d58f0b697589 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Thu, 23 Mar 2023 17:36:34 +0000 Subject: [PATCH] vmm: use the SIGWINCH listener for TTYs too Previously, we were only using it for PTYs, because for PTYs there's no alternative. But since we have to have it for PTYs anyway, if we also use it for TTYs, we can eliminate all of the code that handled SIGWINCH for TTYs. Signed-off-by: Alyssa Ross --- vmm/src/device_manager.rs | 6 +++ vmm/src/seccomp_filters.rs | 3 ++ vmm/src/sigwinch_listener.rs | 61 +++++++++++++++++--------- vmm/src/vm.rs | 83 ++---------------------------------- 4 files changed, 55 insertions(+), 98 deletions(-) diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index d79d3f7b7..1b0f8c2b0 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -1928,6 +1928,12 @@ impl DeviceManager { // SAFETY: stdout is valid and owned solely by us. let stdout = unsafe { File::from_raw_fd(stdout) }; + // SAFETY: FFI call. Trivially safe. + if unsafe { libc::isatty(libc::STDOUT_FILENO) } == 1 { + self.listen_for_sigwinch_on_tty(stdout.try_clone().unwrap()) + .unwrap(); + } + // If an interactive TTY then we can accept input // SAFETY: FFI call. Trivially safe. if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { diff --git a/vmm/src/seccomp_filters.rs b/vmm/src/seccomp_filters.rs index 8177d9faf..c48a68fd4 100644 --- a/vmm/src/seccomp_filters.rs +++ b/vmm/src/seccomp_filters.rs @@ -41,6 +41,7 @@ macro_rules! or { const TCGETS: u64 = 0x5401; const TCSETS: u64 = 0x5402; const TIOCSCTTY: u64 = 0x540E; +const TIOCGPGRP: u64 = 0x540F; const TIOCSPGRP: u64 = 0x5410; const TIOCGWINSZ: u64 = 0x5413; const TIOCSPTLCK: u64 = 0x4004_5431; @@ -264,6 +265,7 @@ fn create_vmm_ioctl_seccomp_rule_common( and![Cond::new(1, ArgLen::Dword, Eq, SIOCSIFNETMASK)?], and![Cond::new(1, ArgLen::Dword, Eq, TCSETS)?], and![Cond::new(1, ArgLen::Dword, Eq, TCGETS)?], + and![Cond::new(1, ArgLen::Dword, Eq, TIOCGPGRP)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCGTPEER)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCGWINSZ)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCSCTTY)?], @@ -455,6 +457,7 @@ fn signal_handler_thread_rules() -> Result)>, Backend fn create_pty_foreground_ioctl_seccomp_rule() -> Result, BackendError> { Ok(or![ + and![Cond::new(1, ArgLen::Dword, Eq, TIOCGPGRP)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCSCTTY)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCSPGRP)?], ]) diff --git a/vmm/src/sigwinch_listener.rs b/vmm/src/sigwinch_listener.rs index cad87f883..049dc077d 100644 --- a/vmm/src/sigwinch_listener.rs +++ b/vmm/src/sigwinch_listener.rs @@ -5,8 +5,8 @@ use crate::clone3::{clone3, clone_args, CLONE_CLEAR_SIGHAND}; use arch::_NSIG; use libc::{ c_int, c_void, close, fork, getpgrp, ioctl, pipe2, poll, pollfd, setsid, sigemptyset, - siginfo_t, signal, sigprocmask, syscall, tcsetpgrp, SYS_close_range, EINVAL, ENOSYS, O_CLOEXEC, - POLLERR, SIGWINCH, SIG_DFL, SIG_SETMASK, STDERR_FILENO, TIOCSCTTY, + siginfo_t, signal, sigprocmask, syscall, tcgetpgrp, tcsetpgrp, SYS_close_range, EINVAL, ENOSYS, + ENOTTY, O_CLOEXEC, POLLERR, SIGWINCH, SIG_DFL, SIG_SETMASK, STDERR_FILENO, TIOCSCTTY, }; use seccompiler::{apply_filter, BpfProgram}; use std::cell::RefCell; @@ -118,13 +118,48 @@ unsafe fn close_unused_fds(keep_fds: &mut [RawFd]) { } } -fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, pty: File) -> ! { - let pty_fd = pty.into_raw_fd(); +fn set_foreground_process_group(tty: &mut File) -> io::Result<()> { + // SAFETY: trivially safe. + let my_pgrp = unsafe { getpgrp() }; + // SAFETY: we have borrowed tty. + let tty_pgrp = unsafe { tcgetpgrp(tty.as_raw_fd()) }; + if tty_pgrp == -1 { + let e = io::Error::last_os_error(); + if e.raw_os_error() != Some(ENOTTY) { + return Err(e); + } + } + if tty_pgrp == my_pgrp { + return Ok(()); + } + + // SAFETY: trivially safe. + let my_pgrp = unsafe { setsid() }; + if my_pgrp == -1 { + return Err(io::Error::last_os_error()); + } + + // Set the tty to be this process's controlling terminal. + // SAFETY: we have borrowed tty. + if unsafe { ioctl(tty.as_raw_fd(), TIOCSCTTY, 0) } == -1 { + return Err(io::Error::last_os_error()); + } + + // Become the foreground process group of the tty. + // SAFETY: we have borrowed tty. + if unsafe { tcsetpgrp(tty.as_raw_fd(), my_pgrp) } == -1 { + return Err(io::Error::last_os_error()); + } + + Ok(()) +} + +fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, mut tty: File) -> ! { // SAFETY: any references to these file descriptors are // unreachable, because this function never returns. unsafe { - close_unused_fds(&mut [STDERR_FILENO, tx.as_raw_fd(), pty_fd]); + close_unused_fds(&mut [STDERR_FILENO, tx.as_raw_fd(), tty.as_raw_fd()]); } TX.with(|opt| opt.replace(Some(tx))); @@ -137,20 +172,8 @@ fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, pty: File) -> ! register_signal_handler(SIGWINCH, sigwinch_handler).unwrap(); - // SAFETY: FFI calls - unsafe { - // Create a new session (and therefore a new process group). - assert_ne!(setsid(), -1); - - // Set the tty to be this process's controlling terminal. - assert_ne!(ioctl(pty_fd, TIOCSCTTY, 0), -1); - - // Become the foreground process group of the tty. - assert_ne!(tcsetpgrp(pty_fd, getpgrp()), -1); - - // Close the PTY fd - assert_ne!(close(pty_fd), -1); - } + set_foreground_process_group(&mut tty).unwrap(); + drop(tty); notify(); diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index f3c64d50c..0633c317d 100644 --- a/vmm/src/vm.rs +++ b/vmm/src/vm.rs @@ -21,7 +21,7 @@ use crate::coredump::{ CpuElf64Writable, DumpState, Elf64Writable, GuestDebuggable, GuestDebuggableError, NoteDescType, }; use crate::cpu; -use crate::device_manager::{Console, DeviceManager, DeviceManagerError, PtyPair}; +use crate::device_manager::{DeviceManager, DeviceManagerError, PtyPair}; use crate::device_tree::DeviceTree; #[cfg(feature = "guest_debug")] use crate::gdb::{Debuggable, DebuggableError, GdbRequestPayload, GdbResponsePayload}; @@ -33,7 +33,6 @@ use crate::migration::get_vm_snapshot; #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))] use crate::migration::url_to_file; use crate::migration::{url_to_path, SNAPSHOT_CONFIG_FILE, SNAPSHOT_STATE_FILE}; -use crate::seccomp_filters::{get_seccomp_filter, Thread}; use crate::GuestMemoryMmap; use crate::{ PciDeviceInfo, CPU_MANAGER_SNAPSHOT_ID, DEVICE_MANAGER_SNAPSHOT_ID, MEMORY_MANAGER_SNAPSHOT_ID, @@ -56,6 +55,7 @@ use gdbstub_arch::aarch64::reg::AArch64CoreRegs as CoreRegs; #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))] use gdbstub_arch::x86::reg::X86_64CoreRegs as CoreRegs; use hypervisor::{HypervisorVmError, VmOps}; +use libc::SIGWINCH; use linux_loader::cmdline::Cmdline; #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))] use linux_loader::elf; @@ -64,9 +64,8 @@ use linux_loader::loader::elf::PvhBootCapability::PvhEntryPresent; #[cfg(target_arch = "aarch64")] use linux_loader::loader::pe::Error::InvalidImageMagicNumber; use linux_loader::loader::KernelLoader; -use seccompiler::{apply_filter, SeccompAction}; +use seccompiler::SeccompAction; use serde::{Deserialize, Serialize}; -use signal_hook::{consts::SIGWINCH, iterator::backend::Handle, iterator::Signals}; use std::cmp; use std::collections::BTreeMap; use std::collections::HashMap; @@ -80,7 +79,6 @@ use std::mem::size_of; use std::num::Wrapping; use std::ops::Deref; use std::os::unix::net::UnixStream; -use std::panic::AssertUnwindSafe; use std::sync::{Arc, Mutex, RwLock}; use std::time::Instant; use std::{result, str, thread}; @@ -96,7 +94,6 @@ use vm_migration::{ SnapshotData, Snapshottable, Transportable, }; use vmm_sys_util::eventfd::EventFd; -use vmm_sys_util::signal::unblock_signal; use vmm_sys_util::sock_ctrl_msg::ScmSocket; use vmm_sys_util::terminal::Terminal; @@ -441,7 +438,6 @@ pub struct Vm { device_manager: Arc>, config: Arc>, on_tty: bool, - signals: Option, state: RwLock, cpu_manager: Arc>, memory_manager: Arc>, @@ -451,8 +447,7 @@ pub struct Vm { #[cfg(all(feature = "kvm", target_arch = "x86_64"))] saved_clock: Option, numa_nodes: NumaNodes, - seccomp_action: SeccompAction, - exit_evt: EventFd, + #[cfg_attr(any(not(feature = "kvm"), target_arch = "aarch64"), allow(dead_code))] hypervisor: Arc, stop_on_boot: bool, load_payload_handle: Option>>, @@ -646,7 +641,6 @@ impl Vm { config, on_tty, threads: Vec::with_capacity(1), - signals: None, state: RwLock::new(vm_state), cpu_manager, memory_manager, @@ -654,8 +648,6 @@ impl Vm { #[cfg(all(feature = "kvm", target_arch = "x86_64"))] saved_clock, numa_nodes, - seccomp_action: seccomp_action.clone(), - exit_evt, hypervisor, stop_on_boot, load_payload_handle, @@ -1222,11 +1214,6 @@ impl Vm { .map_err(Error::SetTerminalCanon)?; } - // Trigger the termination of the signal_handler thread - if let Some(signals) = self.signals.take() { - signals.close(); - } - // Wake up the DeviceManager threads so they will get terminated cleanly self.device_manager .lock() @@ -1632,18 +1619,6 @@ impl Vm { Ok(self.device_manager.lock().unwrap().counters()) } - fn signal_handler(mut signals: Signals, console_input_clone: Arc) { - for sig in &Vm::HANDLED_SIGNALS { - unblock_signal(*sig).unwrap(); - } - - for signal in signals.forever() { - if signal == SIGWINCH { - console_input_clone.update_console_size(); - } - } - } - #[cfg(feature = "tdx")] fn extract_tdvf_sections(&mut self) -> Result<(Vec, bool)> { use arch::x86_64::tdx::*; @@ -1940,54 +1915,6 @@ impl Vm { Ok(()) } - fn setup_signal_handler(&mut self) -> Result<()> { - let console = self.device_manager.lock().unwrap().console().clone(); - let signals = Signals::new(Vm::HANDLED_SIGNALS); - - if !console.need_resize() { - return Ok(()); - } - - match signals { - Ok(signals) => { - self.signals = Some(signals.handle()); - let exit_evt = self.exit_evt.try_clone().map_err(Error::EventFdClone)?; - let signal_handler_seccomp_filter = get_seccomp_filter( - &self.seccomp_action, - Thread::SignalHandler, - self.hypervisor.hypervisor_type(), - ) - .map_err(Error::CreateSeccompFilter)?; - self.threads.push( - thread::Builder::new() - .name("vm_signal_handler".to_string()) - .spawn(move || { - if !signal_handler_seccomp_filter.is_empty() { - if let Err(e) = apply_filter(&signal_handler_seccomp_filter) - .map_err(Error::ApplySeccompFilter) - { - error!("Error applying seccomp filter: {:?}", e); - exit_evt.write(1).ok(); - return; - } - } - std::panic::catch_unwind(AssertUnwindSafe(|| { - Vm::signal_handler(signals, console); - })) - .map_err(|_| { - error!("vm signal_handler thread panicked"); - exit_evt.write(1).ok() - }) - .ok(); - }) - .map_err(Error::SignalHandlerSpawn)?, - ); - } - Err(e) => error!("Signal not found {}", e), - } - Ok(()) - } - fn setup_tty(&self) -> Result<()> { if self.on_tty { io::stdin() @@ -2052,7 +1979,6 @@ impl Vm { #[cfg(target_arch = "x86_64")] let rsdp_addr = self.create_acpi_tables(); - self.setup_signal_handler()?; self.setup_tty()?; // Load kernel synchronously or if asynchronous then wait for load to @@ -2167,7 +2093,6 @@ impl Vm { .start_restored_vcpus() .map_err(Error::CpuManager)?; - self.setup_signal_handler()?; self.setup_tty()?; event!("vm", "restored");