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 <hi@alyssa.is>
This commit is contained in:
Alyssa Ross 2023-03-23 17:36:34 +00:00 committed by Rob Bradford
parent e9841db486
commit 38a1b45783
4 changed files with 55 additions and 98 deletions

View File

@ -1928,6 +1928,12 @@ impl DeviceManager {
// SAFETY: stdout is valid and owned solely by us. // SAFETY: stdout is valid and owned solely by us.
let stdout = unsafe { File::from_raw_fd(stdout) }; 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 // If an interactive TTY then we can accept input
// SAFETY: FFI call. Trivially safe. // SAFETY: FFI call. Trivially safe.
if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } {

View File

@ -41,6 +41,7 @@ macro_rules! or {
const TCGETS: u64 = 0x5401; const TCGETS: u64 = 0x5401;
const TCSETS: u64 = 0x5402; const TCSETS: u64 = 0x5402;
const TIOCSCTTY: u64 = 0x540E; const TIOCSCTTY: u64 = 0x540E;
const TIOCGPGRP: u64 = 0x540F;
const TIOCSPGRP: u64 = 0x5410; const TIOCSPGRP: u64 = 0x5410;
const TIOCGWINSZ: u64 = 0x5413; const TIOCGWINSZ: u64 = 0x5413;
const TIOCSPTLCK: u64 = 0x4004_5431; 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, SIOCSIFNETMASK)?],
and![Cond::new(1, ArgLen::Dword, Eq, TCSETS)?], and![Cond::new(1, ArgLen::Dword, Eq, TCSETS)?],
and![Cond::new(1, ArgLen::Dword, Eq, TCGETS)?], 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, TIOCGTPEER)?],
and![Cond::new(1, ArgLen::Dword, Eq, TIOCGWINSZ)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCGWINSZ)?],
and![Cond::new(1, ArgLen::Dword, Eq, TIOCSCTTY)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCSCTTY)?],
@ -455,6 +457,7 @@ fn signal_handler_thread_rules() -> Result<Vec<(i64, Vec<SeccompRule>)>, Backend
fn create_pty_foreground_ioctl_seccomp_rule() -> Result<Vec<SeccompRule>, BackendError> { fn create_pty_foreground_ioctl_seccomp_rule() -> Result<Vec<SeccompRule>, BackendError> {
Ok(or![ 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, TIOCSCTTY)?],
and![Cond::new(1, ArgLen::Dword, Eq, TIOCSPGRP)?], and![Cond::new(1, ArgLen::Dword, Eq, TIOCSPGRP)?],
]) ])

View File

@ -5,8 +5,8 @@ use crate::clone3::{clone3, clone_args, CLONE_CLEAR_SIGHAND};
use arch::_NSIG; use arch::_NSIG;
use libc::{ use libc::{
c_int, c_void, close, fork, getpgrp, ioctl, pipe2, poll, pollfd, setsid, sigemptyset, 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, siginfo_t, signal, sigprocmask, syscall, tcgetpgrp, tcsetpgrp, SYS_close_range, EINVAL, ENOSYS,
POLLERR, SIGWINCH, SIG_DFL, SIG_SETMASK, STDERR_FILENO, TIOCSCTTY, ENOTTY, O_CLOEXEC, POLLERR, SIGWINCH, SIG_DFL, SIG_SETMASK, STDERR_FILENO, TIOCSCTTY,
}; };
use seccompiler::{apply_filter, BpfProgram}; use seccompiler::{apply_filter, BpfProgram};
use std::cell::RefCell; 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) -> ! { fn set_foreground_process_group(tty: &mut File) -> io::Result<()> {
let pty_fd = pty.into_raw_fd(); // 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 // SAFETY: any references to these file descriptors are
// unreachable, because this function never returns. // unreachable, because this function never returns.
unsafe { 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))); 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(); register_signal_handler(SIGWINCH, sigwinch_handler).unwrap();
// SAFETY: FFI calls set_foreground_process_group(&mut tty).unwrap();
unsafe { drop(tty);
// 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);
}
notify(); notify();

View File

@ -21,7 +21,7 @@ use crate::coredump::{
CpuElf64Writable, DumpState, Elf64Writable, GuestDebuggable, GuestDebuggableError, NoteDescType, CpuElf64Writable, DumpState, Elf64Writable, GuestDebuggable, GuestDebuggableError, NoteDescType,
}; };
use crate::cpu; use crate::cpu;
use crate::device_manager::{Console, DeviceManager, DeviceManagerError, PtyPair}; use crate::device_manager::{DeviceManager, DeviceManagerError, PtyPair};
use crate::device_tree::DeviceTree; use crate::device_tree::DeviceTree;
#[cfg(feature = "guest_debug")] #[cfg(feature = "guest_debug")]
use crate::gdb::{Debuggable, DebuggableError, GdbRequestPayload, GdbResponsePayload}; 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"))] #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))]
use crate::migration::url_to_file; use crate::migration::url_to_file;
use crate::migration::{url_to_path, SNAPSHOT_CONFIG_FILE, SNAPSHOT_STATE_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::GuestMemoryMmap;
use crate::{ use crate::{
PciDeviceInfo, CPU_MANAGER_SNAPSHOT_ID, DEVICE_MANAGER_SNAPSHOT_ID, MEMORY_MANAGER_SNAPSHOT_ID, 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"))] #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))]
use gdbstub_arch::x86::reg::X86_64CoreRegs as CoreRegs; use gdbstub_arch::x86::reg::X86_64CoreRegs as CoreRegs;
use hypervisor::{HypervisorVmError, VmOps}; use hypervisor::{HypervisorVmError, VmOps};
use libc::SIGWINCH;
use linux_loader::cmdline::Cmdline; use linux_loader::cmdline::Cmdline;
#[cfg(all(target_arch = "x86_64", feature = "guest_debug"))] #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))]
use linux_loader::elf; use linux_loader::elf;
@ -64,9 +64,8 @@ use linux_loader::loader::elf::PvhBootCapability::PvhEntryPresent;
#[cfg(target_arch = "aarch64")] #[cfg(target_arch = "aarch64")]
use linux_loader::loader::pe::Error::InvalidImageMagicNumber; use linux_loader::loader::pe::Error::InvalidImageMagicNumber;
use linux_loader::loader::KernelLoader; use linux_loader::loader::KernelLoader;
use seccompiler::{apply_filter, SeccompAction}; use seccompiler::SeccompAction;
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use signal_hook::{consts::SIGWINCH, iterator::backend::Handle, iterator::Signals};
use std::cmp; use std::cmp;
use std::collections::BTreeMap; use std::collections::BTreeMap;
use std::collections::HashMap; use std::collections::HashMap;
@ -80,7 +79,6 @@ use std::mem::size_of;
use std::num::Wrapping; use std::num::Wrapping;
use std::ops::Deref; use std::ops::Deref;
use std::os::unix::net::UnixStream; use std::os::unix::net::UnixStream;
use std::panic::AssertUnwindSafe;
use std::sync::{Arc, Mutex, RwLock}; use std::sync::{Arc, Mutex, RwLock};
use std::time::Instant; use std::time::Instant;
use std::{result, str, thread}; use std::{result, str, thread};
@ -96,7 +94,6 @@ use vm_migration::{
SnapshotData, Snapshottable, Transportable, SnapshotData, Snapshottable, Transportable,
}; };
use vmm_sys_util::eventfd::EventFd; 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::sock_ctrl_msg::ScmSocket;
use vmm_sys_util::terminal::Terminal; use vmm_sys_util::terminal::Terminal;
@ -441,7 +438,6 @@ pub struct Vm {
device_manager: Arc<Mutex<DeviceManager>>, device_manager: Arc<Mutex<DeviceManager>>,
config: Arc<Mutex<VmConfig>>, config: Arc<Mutex<VmConfig>>,
on_tty: bool, on_tty: bool,
signals: Option<Handle>,
state: RwLock<VmState>, state: RwLock<VmState>,
cpu_manager: Arc<Mutex<cpu::CpuManager>>, cpu_manager: Arc<Mutex<cpu::CpuManager>>,
memory_manager: Arc<Mutex<MemoryManager>>, memory_manager: Arc<Mutex<MemoryManager>>,
@ -451,8 +447,7 @@ pub struct Vm {
#[cfg(all(feature = "kvm", target_arch = "x86_64"))] #[cfg(all(feature = "kvm", target_arch = "x86_64"))]
saved_clock: Option<hypervisor::ClockData>, saved_clock: Option<hypervisor::ClockData>,
numa_nodes: NumaNodes, numa_nodes: NumaNodes,
seccomp_action: SeccompAction, #[cfg_attr(any(not(feature = "kvm"), target_arch = "aarch64"), allow(dead_code))]
exit_evt: EventFd,
hypervisor: Arc<dyn hypervisor::Hypervisor>, hypervisor: Arc<dyn hypervisor::Hypervisor>,
stop_on_boot: bool, stop_on_boot: bool,
load_payload_handle: Option<thread::JoinHandle<Result<EntryPoint>>>, load_payload_handle: Option<thread::JoinHandle<Result<EntryPoint>>>,
@ -646,7 +641,6 @@ impl Vm {
config, config,
on_tty, on_tty,
threads: Vec::with_capacity(1), threads: Vec::with_capacity(1),
signals: None,
state: RwLock::new(vm_state), state: RwLock::new(vm_state),
cpu_manager, cpu_manager,
memory_manager, memory_manager,
@ -654,8 +648,6 @@ impl Vm {
#[cfg(all(feature = "kvm", target_arch = "x86_64"))] #[cfg(all(feature = "kvm", target_arch = "x86_64"))]
saved_clock, saved_clock,
numa_nodes, numa_nodes,
seccomp_action: seccomp_action.clone(),
exit_evt,
hypervisor, hypervisor,
stop_on_boot, stop_on_boot,
load_payload_handle, load_payload_handle,
@ -1222,11 +1214,6 @@ impl Vm {
.map_err(Error::SetTerminalCanon)?; .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 // Wake up the DeviceManager threads so they will get terminated cleanly
self.device_manager self.device_manager
.lock() .lock()
@ -1632,18 +1619,6 @@ impl Vm {
Ok(self.device_manager.lock().unwrap().counters()) Ok(self.device_manager.lock().unwrap().counters())
} }
fn signal_handler(mut signals: Signals, console_input_clone: Arc<Console>) {
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")] #[cfg(feature = "tdx")]
fn extract_tdvf_sections(&mut self) -> Result<(Vec<TdvfSection>, bool)> { fn extract_tdvf_sections(&mut self) -> Result<(Vec<TdvfSection>, bool)> {
use arch::x86_64::tdx::*; use arch::x86_64::tdx::*;
@ -1940,54 +1915,6 @@ impl Vm {
Ok(()) 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<()> { fn setup_tty(&self) -> Result<()> {
if self.on_tty { if self.on_tty {
io::stdin() io::stdin()
@ -2052,7 +1979,6 @@ impl Vm {
#[cfg(target_arch = "x86_64")] #[cfg(target_arch = "x86_64")]
let rsdp_addr = self.create_acpi_tables(); let rsdp_addr = self.create_acpi_tables();
self.setup_signal_handler()?;
self.setup_tty()?; self.setup_tty()?;
// Load kernel synchronously or if asynchronous then wait for load to // Load kernel synchronously or if asynchronous then wait for load to
@ -2167,7 +2093,6 @@ impl Vm {
.start_restored_vcpus() .start_restored_vcpus()
.map_err(Error::CpuManager)?; .map_err(Error::CpuManager)?;
self.setup_signal_handler()?;
self.setup_tty()?; self.setup_tty()?;
event!("vm", "restored"); event!("vm", "restored");