From 287887c99c98e47819b3ac999cf4fa94c98407d5 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Fri, 20 Sep 2024 21:14:02 +0200 Subject: [PATCH] vmm: fix console IO safety Rebooting a VM fails with the following error when debug assertions are enabled: fatal runtime error: IO Safety violation: owned file descriptor already closed This happens because FromRawFd::from_raw_fd is used on RawFds stored in ConsoleInfo every time a VM begins to boot, so the second time (after a reboot, or if the first attempt to boot via the API failed), the fd will be closed. Until this assertion is hit, the code is operating on either closed file descriptors, or new file descriptors for something completely different. If debug assertions are disabled, it will just continue doing this with unpredictable results. To fix this, and prevent the problem reocurring, ownership of the console file descriptors needs to be properly tracked, using Rust's type system, so this commit refactors the console code to do that. The file descriptors are now passed around with reference counts, so they won't be closed prematurely. The obvious way to do this would be to just have each member of ConsoleInfo be an Arc, but we need to accomodate that serial console file descriptors can also be sockets. We can't just store an OwnedFd and convert it when it's used, because we only get a reference from the Arc, so we need to store the descriptors as their concrete types in an enum. Since this basically duplicates the ConsoleOutputMode enum from the config, the ConsoleOutputMode enum is now not used past constructing the ConsoleInfo. So that ownership can be represented consistently, the debug console's tty mode now uses its own stdout descriptor. I'm still using .try_clone().unwrap() (i.e. dup()) to clone file descriptors for Endpoint::FilePair and Endpoint::TtyPair, because I assume there's a reason for them not just to hold a single file descriptor. I've also retained the existing behaviour of having serial manager ignore the tty file descriptor passed to it (which is stdout), and instead using stdin. It looks a lot weirder now, because it has to explicitly indicate it's ignoring the fd with an underscore binding. Fixes: 52eebaf6 ("vmm: refactor DeviceManager to use console_info") Signed-off-by: Alyssa Ross --- virtio-devices/src/console.rs | 22 +--- vmm/src/console_devices.rs | 241 ++++++++++++++++++---------------- vmm/src/device_manager.rs | 132 +++++++------------ vmm/src/serial_manager.rs | 82 +++++------- 4 files changed, 213 insertions(+), 264 deletions(-) diff --git a/virtio-devices/src/console.rs b/virtio-devices/src/console.rs index f9eda350d..06c31b7d4 100644 --- a/virtio-devices/src/console.rs +++ b/virtio-devices/src/console.rs @@ -108,10 +108,11 @@ struct ConsoleEpollHandler { file_event_registered: bool, } +#[derive(Clone)] pub enum Endpoint { - File(File), - FilePair(File, File), - PtyPair(File, File), + File(Arc), + FilePair(Arc, Arc), + PtyPair(Arc, Arc), Null, } @@ -139,21 +140,6 @@ impl Endpoint { } } -impl Clone for Endpoint { - fn clone(&self) -> Self { - match self { - Self::File(f) => Self::File(f.try_clone().unwrap()), - Self::FilePair(f_out, f_in) => { - Self::FilePair(f_out.try_clone().unwrap(), f_in.try_clone().unwrap()) - } - Self::PtyPair(f_out, f_in) => { - Self::PtyPair(f_out.try_clone().unwrap(), f_in.try_clone().unwrap()) - } - Self::Null => Self::Null, - } - } -} - impl ConsoleEpollHandler { #[allow(clippy::too_many_arguments)] fn new( diff --git a/vmm/src/console_devices.rs b/vmm/src/console_devices.rs index b7dd91c3c..41956c2b3 100644 --- a/vmm/src/console_devices.rs +++ b/vmm/src/console_devices.rs @@ -23,12 +23,9 @@ use std::fs::read_link; use std::fs::File; use std::fs::OpenOptions; use std::io; -#[cfg(target_arch = "x86_64")] -use std::io::stdout; use std::mem::zeroed; use std::os::fd::AsRawFd; use std::os::fd::FromRawFd; -use std::os::fd::IntoRawFd; use std::os::fd::RawFd; use std::os::unix::fs::OpenOptionsExt; use std::os::unix::net::UnixListener; @@ -48,6 +45,10 @@ pub enum ConsoleDeviceError { #[error("Error creating console device: {0}")] CreateConsoleDevice(#[source] io::Error), + /// No socket option support for console device + #[error("No socket option support for console device")] + NoSocketOptionSupportForConsoleDevice, + /// Error setting pty raw mode #[error("Error setting pty raw mode: {0}")] SetPtyRaw(#[source] vmm_sys_util::errno::Error), @@ -63,14 +64,22 @@ pub enum ConsoleDeviceError { type ConsoleDeviceResult = result::Result; -#[derive(Default, Clone)] +#[derive(Clone)] +pub enum ConsoleOutput { + File(Arc), + Pty(Arc), + Tty(Arc), + Null, + Socket(Arc), + Off, +} + +#[derive(Clone)] pub struct ConsoleInfo { - // For each of File, Pty, Tty and Socket modes, below fields hold the FD - // of console, serial and debug devices. - pub console_main_fd: Option, - pub serial_main_fd: Option, + pub console_main_fd: ConsoleOutput, + pub serial_main_fd: ConsoleOutput, #[cfg(target_arch = "x86_64")] - pub debug_main_fd: Option, + pub debug_main_fd: ConsoleOutput, } fn modify_mode( @@ -179,123 +188,133 @@ fn dup_stdout() -> vmm_sys_util::errno::Result { pub(crate) fn pre_create_console_devices(vmm: &mut Vmm) -> ConsoleDeviceResult { let vm_config = vmm.vm_config.as_mut().unwrap().clone(); let mut vmconfig = vm_config.lock().unwrap(); - let mut console_info = ConsoleInfo::default(); - match vmconfig.console.mode { - ConsoleOutputMode::File => { - let file = File::create(vmconfig.console.file.as_ref().unwrap()) - .map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.console_main_fd = Some(file.into_raw_fd()); - } - ConsoleOutputMode::Pty => { - let (main_fd, sub_fd, path) = - create_pty().map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.console_main_fd = Some(main_fd.into_raw_fd()); - set_raw_mode(&sub_fd.as_raw_fd(), vmm.original_termios_opt.clone())?; - vmconfig.console.file = Some(path.clone()); - vmm.console_resize_pipe = Some(Arc::new( - listen_for_sigwinch_on_tty( - sub_fd, - &vmm.seccomp_action, - vmm.hypervisor.hypervisor_type(), - ) - .map_err(ConsoleDeviceError::StartSigwinchListener)?, - )); - } - ConsoleOutputMode::Tty => { - // Duplicating the file descriptors like this is needed as otherwise - // they will be closed on a reboot and the numbers reused - - let stdout = dup_stdout().map_err(ConsoleDeviceError::DupFd)?; - - // SAFETY: FFI call. Trivially safe. - if unsafe { libc::isatty(stdout.as_raw_fd()) } == 1 { + let console_info = ConsoleInfo { + console_main_fd: match vmconfig.console.mode { + ConsoleOutputMode::File => { + let file = File::create(vmconfig.console.file.as_ref().unwrap()) + .map_err(ConsoleDeviceError::CreateConsoleDevice)?; + ConsoleOutput::File(Arc::new(file)) + } + ConsoleOutputMode::Pty => { + let (main_fd, sub_fd, path) = + create_pty().map_err(ConsoleDeviceError::CreateConsoleDevice)?; + set_raw_mode(&sub_fd.as_raw_fd(), vmm.original_termios_opt.clone())?; + vmconfig.console.file = Some(path.clone()); vmm.console_resize_pipe = Some(Arc::new( listen_for_sigwinch_on_tty( - stdout.try_clone().unwrap(), + sub_fd, &vmm.seccomp_action, vmm.hypervisor.hypervisor_type(), ) .map_err(ConsoleDeviceError::StartSigwinchListener)?, )); + ConsoleOutput::Pty(Arc::new(main_fd)) } + ConsoleOutputMode::Tty => { + // Duplicating the file descriptors like this is needed as otherwise + // they will be closed on a reboot and the numbers reused - // Make sure stdout is in raw mode, if it's a terminal. - set_raw_mode(&stdout, vmm.original_termios_opt.clone())?; - console_info.console_main_fd = Some(stdout.into_raw_fd()); - } - ConsoleOutputMode::Null | ConsoleOutputMode::Socket | ConsoleOutputMode::Off => {} - } + let stdout = dup_stdout().map_err(ConsoleDeviceError::DupFd)?; - match vmconfig.serial.mode { - ConsoleOutputMode::File => { - let file = File::create(vmconfig.serial.file.as_ref().unwrap()) - .map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.serial_main_fd = Some(file.into_raw_fd()); - } - ConsoleOutputMode::Pty => { - let (main_fd, sub_fd, path) = - create_pty().map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.serial_main_fd = Some(main_fd.into_raw_fd()); - set_raw_mode(&sub_fd.as_raw_fd(), vmm.original_termios_opt.clone())?; - vmconfig.serial.file = Some(path.clone()); - } - ConsoleOutputMode::Tty => { - // During vm_shutdown, when serial device is closed, FD#2(STDOUT) - // will be closed and FD#2 could be reused in a future boot of the - // guest by a different file. - // - // To ensure FD#2 always points to STANDARD OUT, a `dup` of STDOUT - // is passed to serial device. Doing so, even if the serial device - // were to be closed, FD#2 will continue to point to STANDARD OUT. + // SAFETY: FFI call. Trivially safe. + if unsafe { libc::isatty(stdout.as_raw_fd()) } == 1 { + vmm.console_resize_pipe = Some(Arc::new( + listen_for_sigwinch_on_tty( + stdout.try_clone().unwrap(), + &vmm.seccomp_action, + vmm.hypervisor.hypervisor_type(), + ) + .map_err(ConsoleDeviceError::StartSigwinchListener)?, + )); + } - let stdout = dup_stdout().map_err(ConsoleDeviceError::DupFd)?; - - // SAFETY: FFI call. Trivially safe. - if unsafe { libc::isatty(stdout.as_raw_fd()) } == 1 { - vmm.console_resize_pipe = Some(Arc::new( - listen_for_sigwinch_on_tty( - stdout.try_clone().unwrap(), - &vmm.seccomp_action, - vmm.hypervisor.hypervisor_type(), - ) - .map_err(ConsoleDeviceError::StartSigwinchListener)?, - )); + // Make sure stdout is in raw mode, if it's a terminal. + set_raw_mode(&stdout, vmm.original_termios_opt.clone())?; + ConsoleOutput::Tty(Arc::new(stdout)) } + ConsoleOutputMode::Socket => { + return Err(ConsoleDeviceError::NoSocketOptionSupportForConsoleDevice) + } + ConsoleOutputMode::Null => ConsoleOutput::Null, + ConsoleOutputMode::Off => ConsoleOutput::Off, + }, + serial_main_fd: match vmconfig.serial.mode { + ConsoleOutputMode::File => { + let file = File::create(vmconfig.serial.file.as_ref().unwrap()) + .map_err(ConsoleDeviceError::CreateConsoleDevice)?; + ConsoleOutput::File(Arc::new(file)) + } + ConsoleOutputMode::Pty => { + let (main_fd, sub_fd, path) = + create_pty().map_err(ConsoleDeviceError::CreateConsoleDevice)?; + set_raw_mode(&sub_fd.as_raw_fd(), vmm.original_termios_opt.clone())?; + vmconfig.serial.file = Some(path.clone()); + ConsoleOutput::Pty(Arc::new(main_fd)) + } + ConsoleOutputMode::Tty => { + // During vm_shutdown, when serial device is closed, FD#2(STDOUT) + // will be closed and FD#2 could be reused in a future boot of the + // guest by a different file. + // + // To ensure FD#2 always points to STANDARD OUT, a `dup` of STDOUT + // is passed to serial device. Doing so, even if the serial device + // were to be closed, FD#2 will continue to point to STANDARD OUT. - // Make sure stdout is in raw mode, if it's a terminal. - set_raw_mode(&stdout, vmm.original_termios_opt.clone())?; - console_info.serial_main_fd = Some(stdout.into_raw_fd()); - } - ConsoleOutputMode::Socket => { - let listener = UnixListener::bind(vmconfig.serial.socket.as_ref().unwrap()) - .map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.serial_main_fd = Some(listener.into_raw_fd()); - } - ConsoleOutputMode::Null | ConsoleOutputMode::Off => {} - } + let stdout = dup_stdout().map_err(ConsoleDeviceError::DupFd)?; - #[cfg(target_arch = "x86_64")] - match vmconfig.debug_console.mode { - ConsoleOutputMode::File => { - let file = File::create(vmconfig.debug_console.file.as_ref().unwrap()) - .map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.debug_main_fd = Some(file.into_raw_fd()); - } - ConsoleOutputMode::Pty => { - let (main_fd, sub_fd, path) = - create_pty().map_err(ConsoleDeviceError::CreateConsoleDevice)?; - console_info.debug_main_fd = Some(main_fd.into_raw_fd()); - set_raw_mode(&sub_fd.as_raw_fd(), vmm.original_termios_opt.clone())?; - vmconfig.debug_console.file = Some(path.clone()); - } - ConsoleOutputMode::Tty => { - let out = stdout(); - console_info.debug_main_fd = Some(out.as_raw_fd()); - set_raw_mode(&out, vmm.original_termios_opt.clone())?; - } - ConsoleOutputMode::Null | ConsoleOutputMode::Socket | ConsoleOutputMode::Off => {} - } + // SAFETY: FFI call. Trivially safe. + if unsafe { libc::isatty(stdout.as_raw_fd()) } == 1 { + vmm.console_resize_pipe = Some(Arc::new( + listen_for_sigwinch_on_tty( + stdout.try_clone().unwrap(), + &vmm.seccomp_action, + vmm.hypervisor.hypervisor_type(), + ) + .map_err(ConsoleDeviceError::StartSigwinchListener)?, + )); + } + + // Make sure stdout is in raw mode, if it's a terminal. + set_raw_mode(&stdout, vmm.original_termios_opt.clone())?; + + ConsoleOutput::Tty(Arc::new(stdout)) + } + ConsoleOutputMode::Socket => { + let listener = UnixListener::bind(vmconfig.serial.socket.as_ref().unwrap()) + .map_err(ConsoleDeviceError::CreateConsoleDevice)?; + ConsoleOutput::Socket(Arc::new(listener)) + } + ConsoleOutputMode::Null => ConsoleOutput::Null, + ConsoleOutputMode::Off => ConsoleOutput::Off, + }, + #[cfg(target_arch = "x86_64")] + debug_main_fd: match vmconfig.debug_console.mode { + ConsoleOutputMode::File => { + let file = File::create(vmconfig.debug_console.file.as_ref().unwrap()) + .map_err(ConsoleDeviceError::CreateConsoleDevice)?; + ConsoleOutput::File(Arc::new(file)) + } + ConsoleOutputMode::Pty => { + let (main_fd, sub_fd, path) = + create_pty().map_err(ConsoleDeviceError::CreateConsoleDevice)?; + set_raw_mode(&sub_fd.as_raw_fd(), vmm.original_termios_opt.clone())?; + vmconfig.debug_console.file = Some(path.clone()); + ConsoleOutput::Pty(Arc::new(main_fd)) + } + ConsoleOutputMode::Tty => { + let out = + dup_stdout().map_err(|e| ConsoleDeviceError::CreateConsoleDevice(e.into()))?; + set_raw_mode(&out, vmm.original_termios_opt.clone())?; + ConsoleOutput::Tty(Arc::new(out)) + } + ConsoleOutputMode::Socket => { + return Err(ConsoleDeviceError::NoSocketOptionSupportForConsoleDevice) + } + ConsoleOutputMode::Null => ConsoleOutput::Null, + ConsoleOutputMode::Off => ConsoleOutput::Off, + }, + }; Ok(console_info) } diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index a641d1e61..9b5fc31a1 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -13,7 +13,7 @@ use crate::config::{ ConsoleOutputMode, DeviceConfig, DiskConfig, FsConfig, NetConfig, PmemConfig, UserDeviceConfig, VdpaConfig, VhostMode, VmConfig, VsockConfig, }; -use crate::console_devices::{ConsoleDeviceError, ConsoleInfo}; +use crate::console_devices::{ConsoleDeviceError, ConsoleInfo, ConsoleOutput}; use crate::cpu::{CpuManager, CPU_MANAGER_ACPI_SIZE}; use crate::device_tree::{DeviceNode, DeviceTree}; use crate::interrupt::LegacyUserspaceInterruptManager; @@ -70,7 +70,6 @@ use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::fs::{File, OpenOptions}; use std::io::{self, stdout, IsTerminal, Seek, SeekFrom}; use std::num::Wrapping; -use std::os::fd::RawFd; use std::os::unix::fs::OpenOptionsExt; use std::os::unix::io::{AsRawFd, FromRawFd}; use std::path::PathBuf; @@ -2005,65 +2004,42 @@ impl DeviceManager { fn add_virtio_console_device( &mut self, virtio_devices: &mut Vec, - console_fd: Option, + console_fd: ConsoleOutput, resize_pipe: Option>, ) -> DeviceManagerResult>> { let console_config = self.config.lock().unwrap().console.clone(); - let endpoint = match console_config.mode { - ConsoleOutputMode::File => { - if let Some(file_fd) = console_fd { - // SAFETY: file_fd is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - Endpoint::File(unsafe { File::from_raw_fd(file_fd) }) - } else { - return Err(DeviceManagerError::InvalidConsoleFd); - } + let endpoint = match console_fd { + ConsoleOutput::File(file) => Endpoint::File(file), + ConsoleOutput::Pty(file) => { + self.console_resize_pipe = resize_pipe; + Endpoint::PtyPair(Arc::new(file.try_clone().unwrap()), file) } - ConsoleOutputMode::Pty => { - if let Some(pty_fd) = console_fd { - // SAFETY: pty_fd is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - let file = unsafe { File::from_raw_fd(pty_fd) }; + ConsoleOutput::Tty(stdout) => { + if stdout.is_terminal() { self.console_resize_pipe = resize_pipe; - Endpoint::PtyPair(file.try_clone().unwrap(), file) + } + + // If an interactive TTY then we can accept input + // SAFETY: FFI call. Trivially safe. + if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { + // SAFETY: FFI call to dup. Trivially safe. + let stdin = unsafe { libc::dup(libc::STDIN_FILENO) }; + if stdin == -1 { + return vmm_sys_util::errno::errno_result() + .map_err(DeviceManagerError::DupFd); + } + // SAFETY: stdin is valid and owned solely by us. + let stdin = unsafe { File::from_raw_fd(stdin) }; + Endpoint::FilePair(stdout, Arc::new(stdin)) } else { - return Err(DeviceManagerError::InvalidConsoleFd); + Endpoint::File(stdout) } } - ConsoleOutputMode::Tty => { - if let Some(tty_fd) = console_fd { - // SAFETY: tty_fd is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - let stdout = unsafe { File::from_raw_fd(tty_fd) }; - - if stdout.is_terminal() { - self.console_resize_pipe = resize_pipe; - } - - // If an interactive TTY then we can accept input - // SAFETY: FFI call. Trivially safe. - if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { - // SAFETY: FFI call to dup. Trivially safe. - let stdin = unsafe { libc::dup(libc::STDIN_FILENO) }; - if stdin == -1 { - return vmm_sys_util::errno::errno_result() - .map_err(DeviceManagerError::DupFd); - } - // SAFETY: stdin is valid and owned solely by us. - let stdin = unsafe { File::from_raw_fd(stdin) }; - Endpoint::FilePair(stdout, stdin) - } else { - Endpoint::File(stdout) - } - } else { - return Err(DeviceManagerError::InvalidConsoleFd); - } - } - ConsoleOutputMode::Socket => { + ConsoleOutput::Socket(_) => { return Err(DeviceManagerError::NoSocketOptionSupportForConsoleDevice); } - ConsoleOutputMode::Null => Endpoint::Null, - ConsoleOutputMode::Off => return Ok(None), + ConsoleOutput::Null => Endpoint::Null, + ConsoleOutput::Off => return Ok(None), }; let id = String::from(CONSOLE_DEVICE_NAME); @@ -2127,31 +2103,24 @@ impl DeviceManager { // SAFETY: console_info is Some, so it's safe to unwrap. let console_info = console_info.unwrap(); - let serial_writer: Option> = match serial_config.mode { - ConsoleOutputMode::File | ConsoleOutputMode::Tty => { - if console_info.serial_main_fd.is_none() { - return Err(DeviceManagerError::InvalidConsoleInfo); - } - // SAFETY: serial_main_fd is Some, so it's safe to unwrap. - // SAFETY: serial_main_fd is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - Some(Box::new(unsafe { - File::from_raw_fd(console_info.serial_main_fd.unwrap()) - })) + + let serial_writer: Option> = match console_info.serial_main_fd { + ConsoleOutput::File(ref file) | ConsoleOutput::Tty(ref file) => { + Some(Box::new(Arc::clone(file))) } - ConsoleOutputMode::Off - | ConsoleOutputMode::Null - | ConsoleOutputMode::Pty - | ConsoleOutputMode::Socket => None, + ConsoleOutput::Off + | ConsoleOutput::Null + | ConsoleOutput::Pty(_) + | ConsoleOutput::Socket(_) => None, }; - if serial_config.mode != ConsoleOutputMode::Off { + + if !matches!(console_info.serial_main_fd, ConsoleOutput::Off) { let serial = self.add_serial_device(interrupt_manager, serial_writer)?; - self.serial_manager = match serial_config.mode { - ConsoleOutputMode::Pty | ConsoleOutputMode::Tty | ConsoleOutputMode::Socket => { + self.serial_manager = match console_info.serial_main_fd { + ConsoleOutput::Pty(_) | ConsoleOutput::Tty(_) | ConsoleOutput::Socket(_) => { let serial_manager = SerialManager::new( serial, console_info.serial_main_fd, - serial_config.mode, serial_config.socket, ) .map_err(DeviceManagerError::CreateSerialManager)?; @@ -2174,24 +2143,13 @@ impl DeviceManager { #[cfg(target_arch = "x86_64")] { - let debug_console_config = self.config.lock().unwrap().debug_console.clone(); let debug_console_writer: Option> = - match debug_console_config.mode { - ConsoleOutputMode::File | ConsoleOutputMode::Tty => { - if console_info.debug_main_fd.is_none() { - return Err(DeviceManagerError::InvalidConsoleInfo); - } - // SAFETY: debug_main_fd is Some, so it's safe to unwrap. - // SAFETY: debug_main_fd is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - Some(Box::new(unsafe { - File::from_raw_fd(console_info.debug_main_fd.unwrap()) - })) - } - ConsoleOutputMode::Off - | ConsoleOutputMode::Null - | ConsoleOutputMode::Pty - | ConsoleOutputMode::Socket => None, + match console_info.debug_main_fd { + ConsoleOutput::File(file) | ConsoleOutput::Tty(file) => Some(Box::new(file)), + ConsoleOutput::Off + | ConsoleOutput::Null + | ConsoleOutput::Pty(_) + | ConsoleOutput::Socket(_) => None, }; if let Some(writer) = debug_console_writer { let _ = self.add_debug_console_device(writer)?; diff --git a/vmm/src/serial_manager.rs b/vmm/src/serial_manager.rs index 45726b6c4..2251c20ab 100644 --- a/vmm/src/serial_manager.rs +++ b/vmm/src/serial_manager.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // -use crate::config::ConsoleOutputMode; +use crate::console_devices::ConsoleOutput; #[cfg(target_arch = "aarch64")] use devices::legacy::Pl011; #[cfg(target_arch = "x86_64")] @@ -13,9 +13,8 @@ use serial_buffer::SerialBuffer; use std::fs::File; use std::io::Read; use std::net::Shutdown; -use std::os::fd::RawFd; use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd}; -use std::os::unix::net::{UnixListener, UnixStream}; +use std::os::unix::net::UnixStream; use std::panic::AssertUnwindSafe; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, Ordering}; @@ -112,11 +111,10 @@ pub struct SerialManager { #[cfg(target_arch = "aarch64")] serial: Arc>, epoll_file: File, - in_file: File, + in_file: ConsoleOutput, kill_evt: EventFd, handle: Option>, pty_write_out: Option>, - mode: ConsoleOutputMode, socket_path: Option, } @@ -124,23 +122,14 @@ impl SerialManager { pub fn new( #[cfg(target_arch = "x86_64")] serial: Arc>, #[cfg(target_arch = "aarch64")] serial: Arc>, - main_fd: Option, - mode: ConsoleOutputMode, + mut output: ConsoleOutput, socket: Option, ) -> Result> { let mut socket_path: Option = None; - let in_file = match mode { - ConsoleOutputMode::Pty => { - if let Some(pty_main) = main_fd { - // SAFETY: pty_main is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - unsafe { File::from_raw_fd(pty_main) } - } else { - return Ok(None); - } - } - ConsoleOutputMode::Tty => { + let in_fd = match output { + ConsoleOutput::Pty(ref fd) => fd.as_raw_fd(), + ConsoleOutput::Tty(_) => { // If running on an interactive TTY then accept input // SAFETY: trivially safe if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { @@ -162,22 +151,17 @@ impl SerialManager { return Err(Error::SetNonBlocking(std::io::Error::last_os_error())); } - stdin_clone + output = ConsoleOutput::Tty(Arc::new(stdin_clone)); + fd } else { return Ok(None); } } - ConsoleOutputMode::Socket => { - if let Some(socket_fd) = main_fd { - if let Some(path_in_socket) = socket { - socket_path = Some(path_in_socket.clone()); - } - // SAFETY: socke_fd is guaranteed to be a valid fd from - // pre_create_console_devices() in vmm/src/console_devices.rs - unsafe { File::from_raw_fd(socket_fd) } - } else { - return Ok(None); + ConsoleOutput::Socket(ref fd) => { + if let Some(path_in_socket) = socket { + socket_path = Some(path_in_socket.clone()); } + fd.as_raw_fd() } _ => return Ok(None), }; @@ -193,7 +177,7 @@ impl SerialManager { ) .map_err(Error::Epoll)?; - let epoll_fd_data = if mode == ConsoleOutputMode::Socket { + let epoll_fd_data = if let ConsoleOutput::Socket(_) = output { EpollDispatch::Socket } else { EpollDispatch::File @@ -202,16 +186,16 @@ impl SerialManager { epoll::ctl( epoll_fd, epoll::ControlOptions::EPOLL_CTL_ADD, - in_file.as_raw_fd(), + in_fd, epoll::Event::new(epoll::Events::EPOLLIN, epoll_fd_data as u64), ) .map_err(Error::Epoll)?; let mut pty_write_out = None; - if mode == ConsoleOutputMode::Pty { + if let ConsoleOutput::Pty(ref file) = output { let write_out = Arc::new(AtomicBool::new(false)); pty_write_out = Some(write_out.clone()); - let writer = in_file.try_clone().map_err(Error::FileClone)?; + let writer = file.try_clone().map_err(Error::FileClone)?; let buffer = SerialBuffer::new(Box::new(writer), write_out); serial .as_ref() @@ -227,11 +211,10 @@ impl SerialManager { Ok(Some(SerialManager { serial, epoll_file, - in_file, + in_file: output, kill_evt, handle: None, pty_write_out, - mode, socket_path, })) } @@ -270,15 +253,10 @@ impl SerialManager { } let epoll_fd = self.epoll_file.as_raw_fd(); - let mut in_file = self.in_file.try_clone().map_err(Error::FileClone)?; + let in_file = self.in_file.clone(); let serial = self.serial.clone(); let pty_write_out = self.pty_write_out.clone(); - // SAFETY: from_raw_fd is always called with a valid fd - let listener = unsafe { - UnixListener::from_raw_fd(in_file.try_clone().map_err(Error::FileClone)?.into_raw_fd()) - }; let mut reader: Option = None; - let mode = self.mode.clone(); // In case of PTY, we want to be able to detect a connection on the // other end of the PTY. This is done by detecting there's no event @@ -314,7 +292,7 @@ impl SerialManager { } }; - if mode != ConsoleOutputMode::Socket && num_events == 0 { + if matches!(in_file, ConsoleOutput::Socket(_)) && num_events == 0 { // This very specific case happens when the serial is connected // to a PTY. We know EPOLLHUP is always present when there's nothing // connected at the other end of the PTY. That's why getting no event @@ -338,6 +316,11 @@ impl SerialManager { .shutdown(Shutdown::Both) .map_err(Error::AcceptConnection)?; } + + let ConsoleOutput::Socket(ref listener) = in_file else { + unreachable!(); + }; + // Events on the listening socket will be connection requests. // Accept them, create a reader and a writer. let (unix_stream, _) = @@ -363,8 +346,8 @@ impl SerialManager { EpollDispatch::File => { if event.events & libc::EPOLLIN as u32 != 0 { let mut input = [0u8; 64]; - let count = match mode { - ConsoleOutputMode::Socket => { + let count = match &in_file { + ConsoleOutput::Socket(_) => { if let Some(mut serial_reader) = reader.as_ref() { let count = serial_reader .read(&mut input) @@ -386,9 +369,12 @@ impl SerialManager { 0 } } - _ => in_file - .read(&mut input) - .map_err(Error::ReadInput)?, + ConsoleOutput::Pty(file) | ConsoleOutput::Tty(file) => { + (&**file) + .read(&mut input) + .map_err(Error::ReadInput)? + } + _ => unreachable!(), }; // Replace "\n" with "\r" to deal with Windows SAC (#1170) @@ -444,7 +430,7 @@ impl Drop for SerialManager { if let Some(handle) = self.handle.take() { handle.join().ok(); } - if self.mode == ConsoleOutputMode::Socket { + if let ConsoleOutput::Socket(_) = self.in_file { if let Some(socket_path) = self.socket_path.as_ref() { std::fs::remove_file(socket_path.as_os_str()) .map_err(Error::RemoveUnixSocket)