From 8c02648ac9f8081e0d97da8211ea76b94ce82196 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Thu, 25 Aug 2022 10:50:05 +0200 Subject: [PATCH] vmm: device_manager: Update virtio-console for proper PTY support Given the virtio-console is now able to buffer its output when no PTY is connected on the other end, the device manager code is updated to enable this. Moving the endpoint type from FilePair to PtyPair enables the proper codepath in the virtio-console implementation, as well as updating the PTY resize code, and forcing the PTY to always be non-blocking. The non-blocking behavior is required to avoid blocking the guest that would be waiting on the virtio-console driver. When receiving an EWOULDBLOCK error, the output will simply be redirected to the temporary buffer so that it can be later flushed. The PTY resize logic has been slightly modified to ensure the PTY file descriptors are closed. It avoids the child process to keep a hold onto the PTY device, which would have caused the PTY to believe something is connected on the other end, which would have prevented the detection of any new connection on the PTY. Fixes #4521 Signed-off-by: Sebastien Boeuf --- vmm/src/device_manager.rs | 19 ++++++++++--------- vmm/src/sigwinch_listener.rs | 20 +++++++++++++++----- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 8fa420083..83e48365a 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -483,7 +483,7 @@ const DEVICE_MANAGER_ACPI_SIZE: usize = 0x10; const TIOCSPTLCK: libc::c_int = 0x4004_5431; const TIOCGTPEER: libc::c_int = 0x5441; -pub fn create_pty(non_blocking: bool) -> io::Result<(File, File, PathBuf)> { +pub fn create_pty() -> io::Result<(File, File, PathBuf)> { // Try to use /dev/pts/ptmx first then fall back to /dev/ptmx // This is done to try and use the devpts filesystem that // could be available for use in the process's namespace first. @@ -492,7 +492,7 @@ pub fn create_pty(non_blocking: bool) -> io::Result<(File, File, PathBuf)> { // See https://www.kernel.org/doc/Documentation/filesystems/devpts.txt // for further details. - let custom_flags = libc::O_NOCTTY | if non_blocking { libc::O_NONBLOCK } else { 0 }; + let custom_flags = libc::O_NONBLOCK; let main = match OpenOptions::new() .read(true) .write(true) @@ -1864,7 +1864,7 @@ impl DeviceManager { self.modify_mode(f.as_raw_fd(), |t| unsafe { cfmakeraw(t) }) } - fn listen_for_sigwinch_on_tty(&mut self, pty: &File) -> std::io::Result<()> { + fn listen_for_sigwinch_on_tty(&mut self, pty_main: File, pty_sub: File) -> std::io::Result<()> { let seccomp_filter = get_seccomp_filter( &self.seccomp_action, Thread::PtyForeground, @@ -1872,7 +1872,7 @@ impl DeviceManager { ) .unwrap(); - match start_sigwinch_listener(seccomp_filter, pty) { + match start_sigwinch_listener(seccomp_filter, pty_main, pty_sub) { Ok(pipe) => { self.console_resize_pipe = Some(Arc::new(pipe)); } @@ -1903,18 +1903,19 @@ impl DeviceManager { let file = pty.main.try_clone().unwrap(); self.console_pty = Some(Arc::new(Mutex::new(pty))); self.console_resize_pipe = resize_pipe.map(Arc::new); - Endpoint::FilePair(file.try_clone().unwrap(), file) + Endpoint::PtyPair(file.try_clone().unwrap(), file) } else { let (main, mut sub, path) = - create_pty(false).map_err(DeviceManagerError::ConsolePtyOpen)?; + create_pty().map_err(DeviceManagerError::ConsolePtyOpen)?; self.set_raw_mode(&mut sub) .map_err(DeviceManagerError::SetPtyRaw)?; self.config.lock().unwrap().console.file = Some(path.clone()); let file = main.try_clone().unwrap(); assert!(resize_pipe.is_none()); - self.listen_for_sigwinch_on_tty(&sub).unwrap(); + self.listen_for_sigwinch_on_tty(main.try_clone().unwrap(), sub) + .unwrap(); self.console_pty = Some(Arc::new(Mutex::new(PtyPair { main, path }))); - Endpoint::FilePair(file.try_clone().unwrap(), file) + Endpoint::PtyPair(file.try_clone().unwrap(), file) } } ConsoleOutputMode::Tty => { @@ -2010,7 +2011,7 @@ impl DeviceManager { self.serial_pty = Some(Arc::new(Mutex::new(pty))); } else { let (main, mut sub, path) = - create_pty(true).map_err(DeviceManagerError::SerialPtyOpen)?; + create_pty().map_err(DeviceManagerError::SerialPtyOpen)?; self.set_raw_mode(&mut sub) .map_err(DeviceManagerError::SetPtyRaw)?; self.config.lock().unwrap().serial.file = Some(path.clone()); diff --git a/vmm/src/sigwinch_listener.rs b/vmm/src/sigwinch_listener.rs index 0c3491668..4b9cac650 100644 --- a/vmm/src/sigwinch_listener.rs +++ b/vmm/src/sigwinch_listener.rs @@ -57,9 +57,11 @@ fn unblock_all_signals() -> io::Result<()> { Ok(()) } -fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, tty: &File) -> ! { +fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, pty: File) -> ! { TX.with(|opt| opt.replace(Some(tx))); + let pty_fd = pty.into_raw_fd(); + unsafe { close(STDIN_FILENO); close(STDOUT_FILENO); @@ -78,10 +80,13 @@ fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, tty: &File) -> ! assert_ne!(setsid(), -1); // Set the tty to be this process's controlling terminal. - assert_ne!(ioctl(tty.as_raw_fd(), TIOCSCTTY, 0), -1); + assert_ne!(ioctl(pty_fd, TIOCSCTTY, 0), -1); // Become the foreground process group of the tty. - assert_ne!(tcsetpgrp(tty.as_raw_fd(), getpgrp()), -1); + assert_ne!(tcsetpgrp(pty_fd, getpgrp()), -1); + + // Close the PTY fd + assert_ne!(close(pty_fd), -1); } notify(); @@ -109,7 +114,11 @@ fn sigwinch_listener_main(seccomp_filter: BpfProgram, tx: File, tty: &File) -> ! exit(0); } -pub fn start_sigwinch_listener(seccomp_filter: BpfProgram, pty: &File) -> io::Result { +pub fn start_sigwinch_listener( + seccomp_filter: BpfProgram, + pty_main: File, + pty_sub: File, +) -> io::Result { let mut pipe = [-1; 2]; if unsafe { pipe2(pipe.as_mut_ptr(), O_CLOEXEC) } == -1 { return Err(io::Error::last_os_error()); @@ -125,7 +134,8 @@ pub fn start_sigwinch_listener(seccomp_filter: BpfProgram, pty: &File) -> io::Re -1 => return Err(io::Error::last_os_error()), 0 => { drop(rx); - sigwinch_listener_main(seccomp_filter, tx, pty); + drop(pty_main); + sigwinch_listener_main(seccomp_filter, tx, pty_sub); } _ => (), }