kata-containers/cloud-hypervisor
1
0
Fork 0
mirror of https://github.com/cloud-hypervisor/cloud-hypervisor.git synced 2026-08-05 02:19:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

vmm: fix console IO safety

Browse Source 
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<File>, 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 <hi@alyssa.is>
This commit is contained in:
Alyssa Ross
2024-09-20 21:14:02 +02:00
committed by Rob Bradford
parent a5df86698b
commit 287887c99c
4 changed files with 213 additions and 264 deletions
Download Patch File Download Diff File Expand all files Collapse all files
vmm/src/device_manager.rs
+45 -87
View File
@@ -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<MetaVirtioDevice>,
console_fd: Option<RawFd>,
console_fd: ConsoleOutput,
resize_pipe: Option<Arc<File>>,
) -> DeviceManagerResult<Option<Arc<virtio_devices::ConsoleResizer>>> {
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<Box<dyn io::Write + Send>> = 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<Box<dyn io::Write + Send>> = 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<Box<dyn io::Write + Send>> =
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)?;