diff --git a/vm/devices/support/fs/fuse/src/lib.rs b/vm/devices/support/fs/fuse/src/lib.rs index 16bd97807d..051966abe9 100644 --- a/vm/devices/support/fs/fuse/src/lib.rs +++ b/vm/devices/support/fs/fuse/src/lib.rs @@ -21,6 +21,7 @@ pub use reply::ReplySender; pub use request::FuseOperation; pub use request::Request; pub use request::RequestReader; +pub use session::DEFAULT_MAX_WRITE; pub use session::Session; pub use session::SessionInfo; diff --git a/vm/devices/support/fs/fuse/src/session.rs b/vm/devices/support/fs/fuse/src/session.rs index 9ce3e09799..2c1c7008a9 100644 --- a/vm/devices/support/fs/fuse/src/session.rs +++ b/vm/devices/support/fs/fuse/src/session.rs @@ -39,6 +39,18 @@ const DEFAULT_MAX_PAGES: u32 = 256; // a difference. const PAGE_SIZE: u32 = 4096; +/// The default maximum FUSE write size (and read/readdir buffer size) +/// advertised to the client: 256 pages of 4 KiB (1 MiB). +/// +/// In the FUSE protocol the server dictates `max_write` in the FUSE_INIT +/// reply; the guest does not propose it and simply adopts the offered value. +/// Transports that bounce I/O through a limited DMA window (e.g. a guest +/// forced to use the Linux swiotlb, which has a hard 256 KiB per-mapping +/// cap) should therefore advertise a smaller value via +/// [`Session::with_max_write`] so the guest never builds request buffers the +/// transport cannot map. +pub const DEFAULT_MAX_WRITE: u32 = DEFAULT_MAX_PAGES * PAGE_SIZE; + /// A FUSE session for a file system. /// /// Handles negotiation and dispatching requests to the file system. @@ -48,11 +60,40 @@ pub struct Session { // a lock, since operations mostly don't need to access the SessionInfo. initialized: atomic::AtomicBool, info: RwLock, + // The maximum write size to offer during FUSE_INIT negotiation. This is a + // hard upper bound: the file system's init callback may lower it, but any + // attempt to raise it is clamped back to this value (see `dispatch_init`). + default_max_write: u32, } impl Session { /// Create a new `Session`. + /// + /// The maximum write size advertised to the client defaults to + /// [`DEFAULT_MAX_WRITE`]. Use [`Session::with_max_write`] to advertise a + /// smaller value when the transport requires it. pub fn new(fs: T) -> Self + where + T: 'static + Fuse + Send + Sync, + { + Self::with_max_write(fs, DEFAULT_MAX_WRITE) + } + + /// Create a new `Session` that advertises the specified maximum write + /// size (in bytes) to the client. + /// + /// In the FUSE protocol the server dictates `max_write`; the guest adopts + /// whatever the server offers. This value is a hard upper bound: the file + /// system's `init` callback may lower the advertised `max_write`, but + /// cannot raise it above the value passed here. + /// + /// This is intended for transports that cannot satisfy the default + /// [`DEFAULT_MAX_WRITE`] in a single DMA mapping. For example, a guest + /// forced to bounce I/O through the Linux swiotlb has a hard 256 KiB + /// per-mapping limit, so any request buffer larger than that fails to + /// map. Advertising a matching `max_write` keeps the guest from building + /// request buffers the transport cannot handle. + pub fn with_max_write(fs: T, max_write: u32) -> Self where T: 'static + Fuse + Send + Sync, { @@ -60,6 +101,7 @@ impl Session { fs: Box::new(fs), initialized: atomic::AtomicBool::new(false), info: RwLock::new(SessionInfo::default()), + default_max_write: max_write, } } @@ -493,9 +535,16 @@ impl Session { info.want2 = DEFAULT_FLAGS2 & init.flags2; } info.time_gran = 1; - info.max_write = DEFAULT_MAX_PAGES * PAGE_SIZE; + info.max_write = self.default_max_write; self.fs.init(&mut info); + // The filesystem's `init` callback may lower `max_write`, but it must + // never raise it above the session's negotiated maximum: that value is + // the transport's per-mapping limit (e.g. a bounce-buffered transport's + // swiotlb cap), and exceeding it reintroduces the large-request mapping + // failures the limit exists to prevent. + info.max_write = info.max_write.min(self.default_max_write); + assert!(info.want & !info.capable == 0); // If the filesystem cleared FUSE_INIT_EXT from want, force want2 to // zero so we never reply with flags2 the kernel won't expect. @@ -1198,7 +1247,66 @@ mod tests { ); } - /// Creates a FUSE_LOOKUP request with a name that's too long (256 bytes, exceeds NAME_MAX of 255) + #[test] + fn init_with_max_write_negotiates_smaller_max_write() { + // A bounce-buffered transport (e.g. swiotlb, with a 256 KiB + // per-mapping limit) builds the session with a reduced max_write so + // the guest never produces request buffers it cannot map. + const SWIOTLB_MAX_WRITE: u32 = 256 * 1024; + let flags = 0x003FFFFB | FUSE_MAX_PAGES; + let request_data = make_init_request(7, 39, 131072, flags, 0); + + let fs = InitCapturingFs::default(); + let session = Session::with_max_write(fs, SWIOTLB_MAX_WRITE); + + let mut sender = CapturingSender::default(); + session.dispatch( + Request::new(request_data.as_slice()).unwrap(), + &mut sender, + None, + ); + + assert!(session.is_initialized()); + + // The reply must advertise the reduced max_write (256 KiB) and the + // matching max_pages (64), rather than the 1 MiB / 256 default. + let (_hdr, init_out) = sender.parse_init_reply(); + assert_eq!(init_out.max_write, SWIOTLB_MAX_WRITE); + assert_eq!(init_out.max_pages, 64); + } + + #[test] + fn init_filesystem_cannot_raise_max_write_above_session_limit() { + // The session imposes a reduced max_write (e.g. a bounce-buffered + // transport's per-mapping limit). A filesystem `init` callback that + // tries to raise max_write above that limit must be clamped back down, + // otherwise the transport-imposed cap would be defeated. + const SWIOTLB_MAX_WRITE: u32 = 256 * 1024; + let flags = 0x003FFFFB | FUSE_MAX_PAGES; + let request_data = make_init_request(7, 39, 131072, flags, 0); + + let fs = InitCapturingFs { + max_write_override: Some(1024 * 1024), + ..Default::default() + }; + let session = Session::with_max_write(fs, SWIOTLB_MAX_WRITE); + + let mut sender = CapturingSender::default(); + session.dispatch( + Request::new(request_data.as_slice()).unwrap(), + &mut sender, + None, + ); + + assert!(session.is_initialized()); + + // The reply must still advertise the session's cap, not the larger + // value the filesystem requested. + let (_hdr, init_out) = sender.parse_init_reply(); + assert_eq!(init_out.max_write, SWIOTLB_MAX_WRITE); + assert_eq!(init_out.max_pages, 64); + } + fn make_lookup_name_too_long() -> Vec { let mut data = vec![0u8; 297]; // 40 byte header + 256 byte name + 1 null terminator diff --git a/vm/devices/virtio/virtiofs/src/virtio.rs b/vm/devices/virtio/virtiofs/src/virtio.rs index a66fbe4eff..ef3185f402 100644 --- a/vm/devices/virtio/virtiofs/src/virtio.rs +++ b/vm/devices/virtio/virtiofs/src/virtio.rs @@ -47,6 +47,41 @@ const DEFAULT_NUM_REQUEST_QUEUES: u32 = 2; /// costs a guest MSI-X vector plus a kernel worker thread. const MAX_REQUEST_QUEUES: u32 = 8; +/// Options controlling how a [`VirtioFsDevice`] is created. +/// +/// Use [`VirtioFsDeviceOptions::default`] for the in-VMM defaults, then +/// override individual fields as needed. +#[derive(Debug, Clone, Copy)] +pub struct VirtioFsDeviceOptions { + /// Number of FUSE request queues. Clamped to `[1, MAX_REQUEST_QUEUES]`. + pub num_request_queues: u32, + /// Maximum size, in bytes, of a single DMA mapping the transport can + /// perform, or `None` if the transport imposes no additional limit + /// (the in-VMM path, where DMA targets guest memory directly). + /// + /// Bounce-buffered transports have a hard per-mapping cap: a guest forced + /// to use the Linux swiotlb can only map 256 KiB at a time + /// (`IO_TLB_SEGSIZE * IO_TLB_SIZE`), regardless of the total swiotlb + /// window. Without a limit, the guest virtio-fs driver builds single + /// physically-contiguous request buffers up to the negotiated FUSE + /// `max_write` (1 MiB by default), which the transport then fails to map, + /// producing cascading I/O errors. Setting this to the transport's + /// per-mapping limit clamps the negotiated `max_write` accordingly. + /// + /// Must be non-zero when `Some`; a zero limit is a misconfiguration and + /// will panic during device construction. + pub max_dma_mapping_size: Option, +} + +impl Default for VirtioFsDeviceOptions { + fn default() -> Self { + Self { + num_request_queues: DEFAULT_NUM_REQUEST_QUEUES, + max_dma_mapping_size: None, + } + } +} + /// PCI configuration space values for virtio-fs devices. #[repr(C)] #[derive(IntoBytes, Immutable, KnownLayout)] @@ -71,7 +106,12 @@ pub struct VirtioFsDevice { shared_memory_region: Option>, #[inspect(skip)] notify_corruption: Arc, - num_request_queues: u32, + /// The maximum write size offered to the FUSE client during negotiation, + /// and a hard upper bound on it: a filesystem `init` callback may lower + /// the value actually negotiated, but never raise it above this. Clamped + /// below the 1 MiB default for bounce-buffered transports (see + /// [`VirtioFsDeviceOptions::max_dma_mapping_size`]). + max_write: u32, } impl VirtioFsDevice { @@ -92,13 +132,13 @@ impl VirtioFsDevice { where Fs: 'static + fuse::Fuse + Send + Sync, { - Self::with_num_request_queues( + Self::with_options( driver_source, tag, fs, shmem_size, notify_corruption, - DEFAULT_NUM_REQUEST_QUEUES, + VirtioFsDeviceOptions::default(), ) } @@ -115,7 +155,49 @@ impl VirtioFsDevice { where Fs: 'static + fuse::Fuse + Send + Sync, { - let num_request_queues = num_request_queues.clamp(1, MAX_REQUEST_QUEUES); + Self::with_options( + driver_source, + tag, + fs, + shmem_size, + notify_corruption, + VirtioFsDeviceOptions { + num_request_queues, + ..Default::default() + }, + ) + } + + /// Creates a new `VirtioFsDevice` with the given [`VirtioFsDeviceOptions`]. + /// + /// This is the most general constructor; [`Self::new`] and + /// [`Self::with_num_request_queues`] are thin wrappers over it. + pub fn with_options( + driver_source: &VmTaskDriverSource, + tag: &str, + fs: Fs, + shmem_size: u64, + notify_corruption: Option>, + options: VirtioFsDeviceOptions, + ) -> Self + where + Fs: 'static + fuse::Fuse + Send + Sync, + { + let num_request_queues = options.num_request_queues.clamp(1, MAX_REQUEST_QUEUES); + + // On a bounce-buffered transport, clamp the negotiated max_write to the + // transport's per-mapping DMA limit so the guest never builds request + // buffers it cannot map. The in-VMM path leaves this unset and keeps + // the full default. + let max_write = match options.max_dma_mapping_size { + Some(limit) => { + // A zero per-mapping limit is a misconfiguration: it would + // negotiate a max_write of 0, producing an unusable device. + assert!(limit > 0, "max_dma_mapping_size must be non-zero"); + fuse::DEFAULT_MAX_WRITE.min(limit) + } + None => fuse::DEFAULT_MAX_WRITE, + }; let mut config = VirtioFsDeviceConfig { tag: [0; 36], @@ -136,12 +218,12 @@ impl VirtioFsDevice { task_name: format!("virtiofs-{}", tag).into(), driver: driver_source.simple(), config, - fs: Arc::new(fuse::Session::new(fs)), + fs: Arc::new(fuse::Session::with_max_write(fs, max_write)), workers: Vec::new(), shmem_size, shared_memory_region: None, notify_corruption, - num_request_queues, + max_write, } } } @@ -154,7 +236,7 @@ impl VirtioDevice for VirtioFsDevice { .with_ring_event_idx(true) .with_ring_indirect_desc(true) .with_ring_packed(true), - max_queues: 1 + self.num_request_queues as u16, + max_queues: 1 + self.config.num_request_queues as u16, device_register_length: self.config.as_bytes().len() as u32, shared_memory: DeviceTraitsSharedMemory { id: 0, @@ -439,10 +521,20 @@ mod tests { (device, tmpdir) } + fn make_device_with_options( + driver: &DefaultDriver, + options: VirtioFsDeviceOptions, + ) -> (VirtioFsDevice, tempfile::TempDir) { + let tmpdir = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new(tmpdir.path(), None).unwrap(); + let driver_source = VmTaskDriverSource::new(SingleDriverBackend::new(driver.clone())); + let device = VirtioFsDevice::with_options(&driver_source, "testfs", fs, 0, None, options); + (device, tmpdir) + } + #[async_test] async fn new_uses_default_num_request_queues(driver: DefaultDriver) { let (device, _tmp) = make_device(&driver, None); - assert_eq!(device.num_request_queues, DEFAULT_NUM_REQUEST_QUEUES); assert_eq!(device.config.num_request_queues, DEFAULT_NUM_REQUEST_QUEUES); assert_eq!( device.traits().max_queues, @@ -453,7 +545,6 @@ mod tests { #[async_test] async fn with_num_request_queues_clamps_above_max(driver: DefaultDriver) { let (device, _tmp) = make_device(&driver, Some(1000)); - assert_eq!(device.num_request_queues, MAX_REQUEST_QUEUES); assert_eq!(device.config.num_request_queues, MAX_REQUEST_QUEUES); assert_eq!(device.traits().max_queues, 1 + MAX_REQUEST_QUEUES as u16); } @@ -463,7 +554,6 @@ mod tests { // A request for zero queues must be clamped up to one so the device // always exposes at least one request virtqueue. let (device, _tmp) = make_device(&driver, Some(0)); - assert_eq!(device.num_request_queues, 1); assert_eq!(device.config.num_request_queues, 1); // 1 hiprio queue + 1 request queue. assert_eq!(device.traits().max_queues, 2); @@ -472,8 +562,58 @@ mod tests { #[async_test] async fn with_num_request_queues_accepts_value_in_range(driver: DefaultDriver) { let (device, _tmp) = make_device(&driver, Some(3)); - assert_eq!(device.num_request_queues, 3); assert_eq!(device.config.num_request_queues, 3); assert_eq!(device.traits().max_queues, 4); } + + #[async_test] + async fn new_uses_default_max_write(driver: DefaultDriver) { + // The in-VMM path (no DMA mapping limit) keeps the full default. + let (device, _tmp) = make_device(&driver, None); + assert_eq!(device.max_write, fuse::DEFAULT_MAX_WRITE); + } + + #[async_test] + async fn with_options_clamps_max_write_for_bounce_buffered(driver: DefaultDriver) { + // A bounce-buffered (swiotlb) transport has a 256 KiB per-mapping + // limit, so the negotiated max_write must be clamped down from the + // 1 MiB default. + const SWIOTLB_MAX_WRITE: u32 = 256 * 1024; + let (device, _tmp) = make_device_with_options( + &driver, + VirtioFsDeviceOptions { + max_dma_mapping_size: Some(SWIOTLB_MAX_WRITE), + ..Default::default() + }, + ); + assert_eq!(device.max_write, SWIOTLB_MAX_WRITE); + } + + #[async_test] + async fn with_options_does_not_raise_max_write_above_default(driver: DefaultDriver) { + // A transport limit larger than the default must not increase + // max_write beyond the default. + let (device, _tmp) = make_device_with_options( + &driver, + VirtioFsDeviceOptions { + max_dma_mapping_size: Some(4 * 1024 * 1024), + ..Default::default() + }, + ); + assert_eq!(device.max_write, fuse::DEFAULT_MAX_WRITE); + } + + #[async_test] + #[should_panic(expected = "max_dma_mapping_size must be non-zero")] + async fn with_options_rejects_zero_dma_mapping_size(driver: DefaultDriver) { + // A zero per-mapping limit is a misconfiguration and must be rejected + // early rather than producing a device with max_write of 0. + let _ = make_device_with_options( + &driver, + VirtioFsDeviceOptions { + max_dma_mapping_size: Some(0), + ..Default::default() + }, + ); + } }