From b875003342d40419b74d88be7213a4d2f37b840d Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sun, 30 Aug 2026 07:58:29 +0000 Subject: [PATCH] [bug] Avoid holding the pipeline lock across management uart writes `handle_management_message` locked the P4 pipeline before dispatching the request. For dump and radix requests, for example, it kept that lock while waiting for the guest to drain the 1-byte UART FIFO. In turn, if a queue notify message could send a vCPU into the `process_guest_packet` fn, where it waited on the same lock, that vCPU would not finish its PIO exit. This would lead to the guest not draining the UART, and, boom, deadlock. ## The Fix Now, the dump path copies table state while under the lock and writes it after unlocking happens. Table updates still hold the lock while changing the pipeline. UART writes now have a deadline, where if a response times out partway through, the handler will try to write the missing newline while waiting for the next request. *Note*: I ran into this issue on a long-running voxel session. The pre-fix binary reproduces it within minutes under a scadm polling loop, while the fixed one does not (A/B testing). --- lib/propolis/src/hw/virtio/softnpu.rs | 118 +++++++++++++++++++++----- 1 file changed, 96 insertions(+), 22 deletions(-) diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs index 66546c543..170e6a0c2 100644 --- a/lib/propolis/src/hw/virtio/softnpu.rs +++ b/lib/propolis/src/hw/virtio/softnpu.rs @@ -8,7 +8,7 @@ use std::{ io::{Result, Write}, sync::{Arc, Mutex}, thread::{sleep, spawn}, - time::Duration, + time::{Duration, Instant}, }; use crate::{ @@ -244,15 +244,23 @@ impl SoftNpu { log: Logger, ) { info!(log, "management handler thread started"); + let mut needs_resync = false; loop { let r = ManagementMessageReader::new(uart.clone(), log.clone()); - let msg = r.read(); + let msg = r.read(&mut needs_resync); info!(log, "received management message: {:#?}", msg); let pipeline = pipeline.clone(); let uart = uart.clone(); let log = log.clone(); - handle_management_message(msg, pipeline, uart, radix, log.clone()); + handle_management_message( + msg, + pipeline, + uart, + radix, + &mut needs_resync, + log.clone(), + ); info!(log, "handled management message"); } } @@ -664,18 +672,79 @@ fn read_buf(mem: &MemCtx, chain: &mut Chain, buf: &mut [u8]) -> usize { }) } +/// Write each byte of `buf` to the uart, yielding while the one-byte FIFO +/// is full. Gives up once `deadline` passes. +/// +/// Returns the number of bytes written. +fn write_with_deadline(uart: &LpcUart, buf: &[u8], deadline: Instant) -> usize { + for (i, b) in buf.iter().enumerate() { + if Instant::now() >= deadline { + return i; + } + while !uart.write(*b) { + if Instant::now() >= deadline { + return i; + } + // If we cannot write to the uart, yield and come back once + // scheduled again. + std::thread::yield_now(); + } + } + buf.len() +} + +/// Write a response buffer to the management uart, yielding while the guest +/// drains the FIFO. This gives up once a deadline passes. So, a guest that +/// stopped reading the management tty cannot block this thread. +/// +/// A timed out write can leave a partial, unterminated frame in the tty; +/// `needs_resync` makes the next call write a newline first to terminate it. +/// +/// Returns true if the full buffer was written. +fn write_management_response( + uart: &LpcUart, + buf: &[u8], + needs_resync: &mut bool, + log: &Logger, +) -> bool { + // The management protocol has no client side timeout to inherit: scadm + // reads the tty with a blocking loop and a 10 KiB buffer, which also + // bounds the largest usable response. A guest that hits this deadline + // stopped reading. + const WRITE_TIMEOUT: Duration = Duration::from_secs(30); + let deadline = Instant::now() + WRITE_TIMEOUT; + + if *needs_resync { + if write_with_deadline(uart, b"\n", deadline) != 1 { + warn!(log, "management uart write timed out, dropping response"); + return false; + } + *needs_resync = false; + } + + let written = write_with_deadline(uart, buf, deadline); + if written == buf.len() { + return true; + } + if written > 0 { + *needs_resync = true; + } + warn!(log, "management uart write timed out, dropping response"); + false +} + /// Handle ASIC management messages from the guest using the loaded program. fn handle_management_message( msg: ManagementRequest, pipeline: Arc>>, uart: Arc, radix: usize, + needs_resync: &mut bool, log: Logger, ) { - let mut pl_opt = pipeline.lock().unwrap(); - match msg { ManagementRequest::TableAdd(tm) => { + let mut pl_opt = pipeline.lock().unwrap(); let pl = match &mut *pl_opt { Some(pl) => pl, None => return, @@ -689,6 +758,7 @@ fn handle_management_message( ); } ManagementRequest::TableRemove(tm) => { + let mut pl_opt = pipeline.lock().unwrap(); let pl = match &mut *pl_opt { Some(pl) => pl, None => return, @@ -705,16 +775,21 @@ fn handle_management_message( let mut buf: Vec = Vec::new(); buf.extend_from_slice(radix.to_string().as_bytes()); buf.push(b'\n'); - for b in &buf { - while !uart.write(*b) { - std::thread::yield_now(); - } + if write_management_response(&uart, &buf, needs_resync, &log) { + info!(log, "wrote: {:?}", buf.len()); } - info!(log, "wrote: {:?}", buf.len()); } ManagementRequest::DumpRequest => { info!(log, "dumping state"); + // Collect the table state under the pipeline lock, then serialize + // and write the response after releasing it. Holding the lock + // across the uart write loop below can deadlock the whole guest. + // For example, if the guest stops draining the management tty, this + // thread spins with the lock held while a vcpu servicing a queue + // notify blocks on the same lock in process_guest_packet. That + // vcpu is stuck in its exit and nothing ever drains the tty. let result = { + let mut pl_opt = pipeline.lock().unwrap(); let pl = match &mut *pl_opt { Some(pl) => &pl.1, None => return, @@ -726,7 +801,9 @@ fn handle_management_message( for id in pl.get_table_ids() { let entries = pl.get_table_entries(id); - result.insert(id, entries); + // The table ids borrow from the pipeline, so own them to + // let the map outlive the lock. + result.insert(id.to_owned(), entries); } result }; @@ -734,26 +811,20 @@ fn handle_management_message( let buf = match serde_json::to_string(&result) { Ok(j) => { let mut buf = j.as_bytes().to_vec(); - info!(log, "writing: {}", j); + info!(log, "writing: {j}"); // Add trailing newline for proper tty handling. buf.push(b'\n'); buf } Err(e) => { - warn!(log, "failed to serialize table state: {}", e); + warn!(log, "failed to serialize table state: {e}"); b"{}\n".to_vec() } }; - for b in &buf { - while !uart.write(*b) { - // If we cannot write to the uart, yield and come back once - // scheduled again. - std::thread::yield_now(); - } + if write_management_response(&uart, &buf, needs_resync, &log) { + info!(log, "management wrote: {}", buf.len()); } - - info!(log, "management wrote: {}", buf.len()); } } } @@ -773,12 +844,15 @@ impl ManagementMessageReader { Self { uart, log } } - fn read(&self) -> ManagementRequest { + fn read(&self, needs_resync: &mut bool) -> ManagementRequest { loop { let mut buf = vec![0; 10240]; let mut i = 0; let mut in_message = false; loop { + if *needs_resync && self.uart.write(b'\n') { + *needs_resync = false; + } let x = match self.uart.read() { Some(b) => b, None => {