Skip to content

Commit 3a1c871

Browse files
westgatewestgate
authored andcommitted
S185: Unsafe evolution — remove redundant Send/Sync impls, evolve DMA fd to OwnedFd
- Remove redundant `unsafe impl Send/Sync` from LockedMemory (auto-derived from AlignedAlloc) and `unsafe impl Send` from Bar0Access (auto-derived from SafeMmapRegion via memmap2's MmapInner: Send+Sync) - Add compile-time Send/Sync assertions on AlignedAlloc, HugePageMemory, DeviceMmap, LockedMemory, Bar0Access for regression protection - Evolve akida-driver DmaBuffer from RawFd to OwnedFd — stronger fd validity guarantees, uses dma_map/dma_unmap directly instead of dma_map_fd/dma_unmap_fd - Deprecate dma_map_fd/dma_unmap_fd in favor of BorrowedFd/OwnedFd variants - Net: -3 unsafe impl blocks, stronger ownership model for DMA fd handling Made-with: Cursor
1 parent 567c38c commit 3a1c871

9 files changed

Lines changed: 71 additions & 25 deletions

File tree

crates/core/hw-safe/src/aligned_alloc.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,13 @@ impl Drop for AlignedAlloc {
161161
// as_slice/as_mut_slice.
162162
unsafe impl Send for AlignedAlloc {}
163163
unsafe impl Sync for AlignedAlloc {}
164+
#[allow(dead_code, reason = "compile-time trait bound assertion")]
165+
const _: () = {
166+
fn assert_send_sync<T: Send + Sync>() {}
167+
fn check() {
168+
assert_send_sync::<AlignedAlloc>();
169+
}
170+
};
164171

165172
impl std::fmt::Debug for AlignedAlloc {
166173
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

crates/core/hw-safe/src/device_mmap.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,13 @@ impl Drop for DeviceMmap {
136136
unsafe impl Send for DeviceMmap {}
137137
// SAFETY: Reads via &self; writes via &mut self. Borrow checker enforces.
138138
unsafe impl Sync for DeviceMmap {}
139+
#[allow(dead_code, reason = "compile-time trait bound assertion")]
140+
const _: () = {
141+
fn assert_send_sync<T: Send + Sync>() {}
142+
fn check() {
143+
assert_send_sync::<DeviceMmap>();
144+
}
145+
};
139146

140147
impl std::fmt::Debug for DeviceMmap {
141148
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

crates/core/hw-safe/src/huge_page.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,13 @@ impl Drop for HugePageMemory {
156156
unsafe impl Send for HugePageMemory {}
157157
// SAFETY: Reads via &self; writes require &mut self.
158158
unsafe impl Sync for HugePageMemory {}
159+
#[allow(dead_code, reason = "compile-time trait bound assertion")]
160+
const _: () = {
161+
fn assert_send_sync<T: Send + Sync>() {}
162+
fn check() {
163+
assert_send_sync::<HugePageMemory>();
164+
}
165+
};
159166

160167
impl std::fmt::Debug for HugePageMemory {
161168
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

crates/core/hw-safe/src/locked_memory.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,15 @@ impl Drop for LockedMemory {
103103
}
104104
}
105105

106-
// SAFETY: Same justification as AlignedAlloc — exclusive ownership.
107-
unsafe impl Send for LockedMemory {}
108-
unsafe impl Sync for LockedMemory {}
106+
// Send + Sync auto-derived: LockedMemory contains only AlignedAlloc (which is
107+
// Send + Sync) — no additional raw pointers or non-threadsafe fields.
108+
#[allow(dead_code, reason = "compile-time trait bound assertion")]
109+
const _: () = {
110+
fn assert_send_sync<T: Send + Sync>() {}
111+
fn check() {
112+
assert_send_sync::<LockedMemory>();
113+
}
114+
};
109115

110116
impl std::fmt::Debug for LockedMemory {
111117
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

crates/core/hw-safe/src/vfio_dma.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,8 @@ pub unsafe fn dma_unmap(container_fd: BorrowedFd<'_>, unmap: &VfioDmaUnmap) -> s
135135

136136
/// Map a host buffer into the device IOMMU using a raw file descriptor.
137137
///
138-
/// Convenience wrapper that combines [`BorrowedFd::borrow_raw`] and [`dma_map`]
139-
/// into a single call, eliminating the duplicate `unsafe` at each call site.
138+
/// Prefer [`dma_map`] with `OwnedFd`/`BorrowedFd` for stronger fd validity
139+
/// guarantees. This wrapper exists for callers that only have a `RawFd`.
140140
///
141141
/// # Errors
142142
///
@@ -147,6 +147,7 @@ pub unsafe fn dma_unmap(container_fd: BorrowedFd<'_>, unmap: &VfioDmaUnmap) -> s
147147
/// Same invariants as [`dma_map`], plus:
148148
/// - `fd` must be a valid, open VFIO container file descriptor that remains
149149
/// open for the duration of this call.
150+
#[deprecated(note = "prefer dma_map() with BorrowedFd/OwnedFd for fd safety")]
150151
pub unsafe fn dma_map_fd(fd: RawFd, map: &VfioDmaMap) -> std::io::Result<()> {
151152
// SAFETY: caller guarantees fd is valid and open.
152153
let borrowed = unsafe { BorrowedFd::borrow_raw(fd) };
@@ -156,8 +157,8 @@ pub unsafe fn dma_map_fd(fd: RawFd, map: &VfioDmaMap) -> std::io::Result<()> {
156157

157158
/// Remove a device IOMMU mapping using a raw file descriptor.
158159
///
159-
/// Convenience wrapper that combines [`BorrowedFd::borrow_raw`] and [`dma_unmap`]
160-
/// into a single call, eliminating the duplicate `unsafe` at each call site.
160+
/// Prefer [`dma_unmap`] with `OwnedFd`/`BorrowedFd` for stronger fd validity
161+
/// guarantees. This wrapper exists for callers that only have a `RawFd`.
161162
///
162163
/// # Errors
163164
///
@@ -168,6 +169,7 @@ pub unsafe fn dma_map_fd(fd: RawFd, map: &VfioDmaMap) -> std::io::Result<()> {
168169
/// Same invariants as [`dma_unmap`], plus:
169170
/// - `fd` must be a valid, open VFIO container file descriptor that remains
170171
/// open for the duration of this call.
172+
#[deprecated(note = "prefer dma_unmap() with BorrowedFd/OwnedFd for fd safety")]
171173
pub unsafe fn dma_unmap_fd(fd: RawFd, unmap: &VfioDmaUnmap) -> std::io::Result<()> {
172174
// SAFETY: caller guarantees fd is valid and open.
173175
let borrowed = unsafe { BorrowedFd::borrow_raw(fd) };

crates/core/nvpmu/src/bar0.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,15 @@ impl Bar0Access {
106106
}
107107
}
108108

109-
// SAFETY: Bar0Access delegates all pointer operations to SafeMmapRegion
110-
// which is Send. Moving between threads doesn't invalidate the mapping.
111-
unsafe impl Send for Bar0Access {}
109+
// Send auto-derived: Bar0Access contains SafeMmapRegion (Send via memmap2's
110+
// MmapInner: Send + Sync) and String (Send). No raw pointers.
111+
#[allow(dead_code, reason = "compile-time trait bound assertion")]
112+
const _: () = {
113+
fn assert_send<T: Send>() {}
114+
fn check() {
115+
assert_send::<Bar0Access>();
116+
}
117+
};
112118

113119
impl hw_learn::applicator::RegisterAccess for Bar0Access {
114120
fn read_u32(&self, offset: u64) -> std::result::Result<u32, String> {

crates/core/nvpmu/src/dma.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ impl Drop for DmaBuffer {
120120

121121
// SAFETY: container_fd still valid (DmaBuffer dropped before container);
122122
// unmap matches prior dma_map for this IOVA range.
123+
#[allow(deprecated, reason = "nvpmu stores RawFd — evolve to OwnedFd in future")]
123124
let _ = unsafe { vfio_dma::dma_unmap_fd(self.container_fd, &unmap) };
124125

125126
// LockedMemory / HugePageMemory handle their own cleanup via Drop.
@@ -158,6 +159,7 @@ impl DmaAllocator {
158159

159160
// SAFETY: container_fd is a valid VFIO container fd held for the lifetime of
160161
// DmaAllocator; vaddr from LockedMemory/HugePageMemory; IOVA range unused.
162+
#[allow(deprecated, reason = "nvpmu stores RawFd — evolve to OwnedFd in future")]
161163
unsafe { vfio_dma::dma_map_fd(self.container_fd, &dma_map) }
162164
.map_err(|e| NvPmuError::Hardware(format!("VFIO DMA map failed: {e}")))
163165
}

crates/neuromorphic/akida-driver/src/backends/vfio/dma.rs

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
// SPDX-License-Identifier: AGPL-3.0-only
2-
#![allow(unsafe_code)] // VFIO DMA ioctls + BorrowedFd::borrow_raw(container_fd)
2+
#![allow(unsafe_code)] // VFIO DMA map/unmap ioctls require unsafe
33
//! DMA buffer management for VFIO NPU backend
44
//!
55
//! Provides page-aligned, mlock'd, IOMMU-mapped memory buffers for
66
//! zero-copy data transfer between host and NPU hardware.
77
88
use crate::error::{AkidaError, Result};
9-
use std::os::fd::RawFd;
9+
use std::os::fd::{AsFd, OwnedFd};
1010

1111
use toadstool_hw_safe::LockedMemory;
12-
use toadstool_hw_safe::vfio_dma::{VfioDmaMap, VfioDmaUnmap, dma_map_fd, dma_unmap_fd, flags};
12+
use toadstool_hw_safe::vfio_dma::{VfioDmaMap, VfioDmaUnmap, dma_map, dma_unmap, flags};
1313

1414
/// DMA buffer for fast host-to-device data transfer.
1515
///
@@ -20,7 +20,7 @@ pub struct DmaBuffer {
2020
mem: LockedMemory,
2121
iova: u64,
2222
size: usize,
23-
container_fd: RawFd,
23+
container_fd: OwnedFd,
2424
}
2525

2626
impl DmaBuffer {
@@ -29,7 +29,7 @@ impl DmaBuffer {
2929
/// # Errors
3030
///
3131
/// Returns an error if allocation, mlock, or IOMMU DMA mapping fails.
32-
pub(crate) fn new(container_fd: RawFd, size: usize, iova: u64) -> Result<Self> {
32+
pub(crate) fn new(container_fd: OwnedFd, size: usize, iova: u64) -> Result<Self> {
3333
if size == 0 {
3434
return Err(AkidaError::transfer_failed("DMA buffer size must be > 0"));
3535
}
@@ -59,9 +59,9 @@ impl DmaBuffer {
5959
dma_map_arg.flags
6060
);
6161

62-
// SAFETY: container_fd from VFIO container open (valid and not closed);
62+
// SAFETY: container_fd is the VFIO container (OwnedFd guarantees validity);
6363
// map.vaddr points at mem's allocation for size bytes; IOVA range chosen by caller.
64-
if let Err(e) = unsafe { dma_map_fd(container_fd, &dma_map_arg) } {
64+
if let Err(e) = unsafe { dma_map(container_fd.as_fd(), &dma_map_arg) } {
6565
tracing::warn!("DMA map failed: {e}");
6666
return Err(AkidaError::transfer_failed(format!(
6767
"Failed to map DMA: {e}"
@@ -115,9 +115,9 @@ impl Drop for DmaBuffer {
115115
size: self.size as u64,
116116
};
117117

118-
// SAFETY: DmaBuffer dropped before VfioBackend (parent), so container_fd is still
119-
// valid and open; iova/size match the prior dma_map_fd call.
120-
let _ = unsafe { dma_unmap_fd(self.container_fd, &dma_unmap_arg) };
118+
// SAFETY: container_fd is OwnedFd (guaranteed valid until Drop completes);
119+
// iova/size match the prior dma_map call for this buffer.
120+
let _ = unsafe { dma_unmap(self.container_fd.as_fd(), &dma_unmap_arg) };
121121

122122
tracing::debug!("Freed DMA buffer at iova={:#x}", self.iova);
123123
}
@@ -128,18 +128,23 @@ mod tests {
128128
use super::*;
129129
use toadstool_hw_safe::vfio_dma::{VfioDmaMap, VfioDmaUnmap};
130130

131+
fn dummy_fd() -> OwnedFd {
132+
use std::fs::File;
133+
let f = File::open("/dev/null").expect("/dev/null");
134+
f.into()
135+
}
136+
131137
#[test]
132138
fn test_dma_buffer_new_size_zero() {
133-
let result = DmaBuffer::new(-1, 0, 0);
139+
let result = DmaBuffer::new(dummy_fd(), 0, 0);
134140
assert!(result.is_err());
135141
let err = result.unwrap_err();
136142
assert!(err.to_string().contains("size must be > 0"));
137143
}
138144

139145
#[test]
140146
fn test_dma_buffer_iova_size_accessors() {
141-
// We can't create a real DmaBuffer without VFIO, but we can test the size zero path
142-
let result = DmaBuffer::new(0, 0, 0x1000);
147+
let result = DmaBuffer::new(dummy_fd(), 0, 0x1000);
143148
assert!(result.is_err());
144149
}
145150

crates/neuromorphic/akida-driver/src/backends/vfio/mod.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ use crate::error::{AkidaError, Result};
2525
use crate::mmio::{Bar, MappedRegion, regs};
2626
use std::fs::OpenOptions;
2727
use std::os::fd::{AsFd, OwnedFd};
28-
use std::os::unix::io::AsRawFd;
2928
use toadstool_hw_safe::vfio_setup;
3029
use types::PollConfig;
3130
use types::ioctls;
@@ -87,7 +86,12 @@ impl VfioBackend {
8786
let iova = self.next_iova;
8887
let aligned_size = size.div_ceil(4096) * 4096;
8988
self.next_iova += aligned_size as u64;
90-
DmaBuffer::new(self.container.as_raw_fd(), aligned_size, iova)
89+
let fd: OwnedFd = self
90+
.container
91+
.try_clone()
92+
.map_err(|e| AkidaError::transfer_failed(format!("dup container fd: {e}")))?
93+
.into();
94+
DmaBuffer::new(fd, aligned_size, iova)
9195
}
9296

9397
#[expect(

0 commit comments

Comments
 (0)