From 654a3c73c89de782dc861b15e99efa6b34903928 Mon Sep 17 00:00:00 2001 From: Sean Link Date: Thu, 13 Nov 2025 09:27:35 -0700 Subject: [PATCH 1/4] Add Send + Sync bounds to resolve Arc clippy warnings - Add Send + Sync bounds to LogProvider trait - Add Send + Sync bounds to ExpiryPolicy trait - Add Send + Sync bounds to action traits: MetaAction, StaticAction, StatefulAction, HairpinAction, ActionDesc - Add Send + Sync bounds to SNAT implementation trait bounds - Add unsafe Send + Sync implementations for KStatNamed Fixes #785: Port should require Send + Sync bounds This minimal implementation only adds bounds where compilation errors occurred, ensuring Arc> and Arc are Send + Sync. --- lib/opte/src/ddi/kstat.rs | 23 +++++++++++++++++++++++ lib/opte/src/engine/flow_table.rs | 2 +- lib/opte/src/engine/rule.rs | 10 +++++----- lib/opte/src/engine/snat.rs | 8 ++++---- lib/opte/src/lib.rs | 2 +- 5 files changed, 34 insertions(+), 11 deletions(-) diff --git a/lib/opte/src/ddi/kstat.rs b/lib/opte/src/ddi/kstat.rs index f6496ff9..23affee4 100644 --- a/lib/opte/src/ddi/kstat.rs +++ b/lib/opte/src/ddi/kstat.rs @@ -306,3 +306,26 @@ impl From for Error { Self::NulChar } } + +// SAFETY: KStatNamed is safe for Send + Sync under the following conditions: +// +// 1. Read-only sharing: After creation, the `ksp` pointer is never mutated - only +// passed to kstat_delete() in Drop. Multiple threads can safely read the same +// immutable pointer value. +// +// 2. Kernel manages concurrency: The kstat framework handles concurrent access to +// the underlying kernel structures via its own internal mechanisms. +// +// 3. Atomic statistics: Individual stats in `vals` use AtomicU64, ensuring each +// counter update is atomic and thread-safe. +// +// 4. Intentional design trade-off: We explicitly do NOT set ks_lock (see struct +// comment), accepting that different threads may see inconsistent snapshots +// of the stats as a group, while individual values remain uncorrupted. +// +// 5. No shared mutable state: The raw pointer represents a kernel resource with +// a clear lifecycle (create -> install -> delete) with no Rust-side mutation. +#[cfg(all(not(feature = "std"), not(test)))] +unsafe impl Send for KStatNamed {} +#[cfg(all(not(feature = "std"), not(test)))] +unsafe impl Sync for KStatNamed {} diff --git a/lib/opte/src/engine/flow_table.rs b/lib/opte/src/engine/flow_table.rs index fa70cc0e..cbbb7d42 100644 --- a/lib/opte/src/engine/flow_table.rs +++ b/lib/opte/src/engine/flow_table.rs @@ -64,7 +64,7 @@ impl Ttl { } /// A policy for expiring flow table entries over time. -pub trait ExpiryPolicy: fmt::Debug { +pub trait ExpiryPolicy: fmt::Debug + Send + Sync { /// Returns whether the given flow should be removed, given current flow /// state, the time a packet was last received, and the current time. fn is_expired(&self, entry: &FlowEntry, now: Moment) -> bool; diff --git a/lib/opte/src/engine/rule.rs b/lib/opte/src/engine/rule.rs index 141b31e4..4247adb8 100644 --- a/lib/opte/src/engine/rule.rs +++ b/lib/opte/src/engine/rule.rs @@ -173,7 +173,7 @@ where /// An Action Descriptor holds the information needed to create the /// [`HdrTransform`] which implements the desired action. An /// ActionDesc is created by a [`StatefulAction`] implementation. -pub trait ActionDesc { +pub trait ActionDesc: Send + Sync { /// Generate the [`HdrTransform`] which implements this descriptor. fn gen_ht(&self, dir: Direction) -> HdrTransform; @@ -742,7 +742,7 @@ pub enum GenDescError { pub type GenDescResult = ActionResult, GenDescError>; -pub trait StatefulAction: Display { +pub trait StatefulAction: Display + Send + Sync { /// Generate a an [`ActionDesc`] based on the [`InnerFlowId`] and /// [`ActionMeta`]. This action may also add, remove, or modify /// metadata to communicate data to downstream actions. @@ -772,7 +772,7 @@ pub enum GenHtError { pub type GenHtResult = ActionResult; -pub trait StaticAction: Display { +pub trait StaticAction: Display + Send + Sync { fn gen_ht( &self, dir: Direction, @@ -795,7 +795,7 @@ pub type ModMetaResult = ActionResult<(), String>; /// metadata in some way. That is, it has no transformation to make on /// the packet, only add/modify/remove metadata for use by later /// layers. -pub trait MetaAction: Display { +pub trait MetaAction: Display + Send + Sync { /// Return the predicates implicit to this action. /// /// Return both the header [`Predicate`] list and @@ -843,7 +843,7 @@ impl From for GenBtError { /// /// For example, you could use this to hairpin an ARP Reply in response /// to a guest's ARP request. -pub trait HairpinAction: Display { +pub trait HairpinAction: Display + Send + Sync { /// Generate a [`Packet`] to hairpin back to the source. The /// `meta` argument holds the packet metadata, including any /// modifications made by previous layers up to this point. diff --git a/lib/opte/src/engine/snat.rs b/lib/opte/src/engine/snat.rs index e8e4f121..d3bbff22 100644 --- a/lib/opte/src/engine/snat.rs +++ b/lib/opte/src/engine/snat.rs @@ -218,7 +218,7 @@ impl From for GenDescError { } } -impl SNat { +impl SNat { pub fn new(addr: T) -> Self { SNat { priv_ip: addr, @@ -299,7 +299,7 @@ impl Display for SNat { } } -impl StatefulAction for SNat +impl StatefulAction for SNat where SNat: Display, { @@ -367,7 +367,7 @@ pub struct SNatDesc { pub const SNAT_NAME: &str = "SNAT"; -impl ActionDesc for SNatDesc { +impl ActionDesc for SNatDesc { fn gen_ht(&self, dir: Direction) -> HdrTransform { match dir { // Outbound traffic needs its source IP and source port @@ -425,7 +425,7 @@ pub struct SNatIcmpEchoDesc { pub const SNAT_ICMP_ECHO_NAME: &str = "SNAT_ICMP_ECHO"; -impl ActionDesc for SNatIcmpEchoDesc { +impl ActionDesc for SNatIcmpEchoDesc { // SNAT needs to generate an additional transform for ICMP traffic in // order to treat the Echo Identifier as a psuedo ULP port. fn gen_ht(&self, dir: Direction) -> HdrTransform { diff --git a/lib/opte/src/lib.rs b/lib/opte/src/lib.rs index 6de57220..6c62d544 100644 --- a/lib/opte/src/lib.rs +++ b/lib/opte/src/lib.rs @@ -200,7 +200,7 @@ mod opte_provider { /// /// Logging levels are provided by [`LogLevel`]. These levels will map /// to the underlying provider with varying degrees of success. -pub trait LogProvider { +pub trait LogProvider: Send + Sync { /// Log a message at the specified level. fn log(&self, level: LogLevel, msg: &str); } From 72ba0d8c29ffcf228d6e063ac2014778a07a5645 Mon Sep 17 00:00:00 2001 From: Sean Link Date: Fri, 14 Nov 2025 09:12:19 -0700 Subject: [PATCH 2/4] fix: ran cargo fmt in lib/opte --- lib/opte/src/ddi/kstat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/opte/src/ddi/kstat.rs b/lib/opte/src/ddi/kstat.rs index 23affee4..0d317d09 100644 --- a/lib/opte/src/ddi/kstat.rs +++ b/lib/opte/src/ddi/kstat.rs @@ -313,7 +313,7 @@ impl From for Error { // passed to kstat_delete() in Drop. Multiple threads can safely read the same // immutable pointer value. // -// 2. Kernel manages concurrency: The kstat framework handles concurrent access to +// 2. Kernel manages concurrency: The kstat framework handles concurrent access to // the underlying kernel structures via its own internal mechanisms. // // 3. Atomic statistics: Individual stats in `vals` use AtomicU64, ensuring each From 3bc9c4c92f436f2ab54cb9620aaf33fbd8ab2de2 Mon Sep 17 00:00:00 2001 From: Sean Link Date: Thu, 1 Jan 2026 13:40:21 -0700 Subject: [PATCH 3/4] addressing PR comments --- lib/opte/src/ddi/kstat.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/lib/opte/src/ddi/kstat.rs b/lib/opte/src/ddi/kstat.rs index 0d317d09..c172e7e6 100644 --- a/lib/opte/src/ddi/kstat.rs +++ b/lib/opte/src/ddi/kstat.rs @@ -61,7 +61,7 @@ cfg_if! { /// ``` /// /// To register a provider see [`KStatNamed`]. -pub trait KStatProvider { +pub trait KStatProvider: Send + Sync { const NUM_FIELDS: u32; type Snap; @@ -307,24 +307,14 @@ impl From for Error { } } -// SAFETY: KStatNamed is safe for Send + Sync under the following conditions: +// SAFETY: KStatNamed is safe for Send + Sync because: // -// 1. Read-only sharing: After creation, the `ksp` pointer is never mutated - only -// passed to kstat_delete() in Drop. Multiple threads can safely read the same -// immutable pointer value. +// 1. The underlying T must itself be Send + Sync because it is fully visible to any +// user of the struct. We meet this through the new type bound on KStatProvider. // -// 2. Kernel manages concurrency: The kstat framework handles concurrent access to -// the underlying kernel structures via its own internal mechanisms. -// -// 3. Atomic statistics: Individual stats in `vals` use AtomicU64, ensuring each -// counter update is atomic and thread-safe. -// -// 4. Intentional design trade-off: We explicitly do NOT set ks_lock (see struct -// comment), accepting that different threads may see inconsistent snapshots -// of the stats as a group, while individual values remain uncorrupted. -// -// 5. No shared mutable state: The raw pointer represents a kernel resource with -// a clear lifecycle (create -> install -> delete) with no Rust-side mutation. +// 2. ksp is itself safe to move between threads, since KSTAT(9S) imposes no MT +// constraints on callers. ksp is never exposed via a &ref (nor is it used by any +// methods taking &self), and is only used during drop. #[cfg(all(not(feature = "std"), not(test)))] unsafe impl Send for KStatNamed {} #[cfg(all(not(feature = "std"), not(test)))] From ac11156eec876300b1405acc43c7b2d3fd0d1cae Mon Sep 17 00:00:00 2001 From: Sean Link Date: Thu, 1 Jan 2026 13:45:18 -0700 Subject: [PATCH 4/4] Adding missing number in comment --- lib/opte/src/ddi/kstat.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/opte/src/ddi/kstat.rs b/lib/opte/src/ddi/kstat.rs index c172e7e6..63b2f413 100644 --- a/lib/opte/src/ddi/kstat.rs +++ b/lib/opte/src/ddi/kstat.rs @@ -313,8 +313,11 @@ impl From for Error { // user of the struct. We meet this through the new type bound on KStatProvider. // // 2. ksp is itself safe to move between threads, since KSTAT(9S) imposes no MT -// constraints on callers. ksp is never exposed via a &ref (nor is it used by any +// constraints on callers. +// +// 3. ksp is never exposed via a &ref (nor is it used by any // methods taking &self), and is only used during drop. +// #[cfg(all(not(feature = "std"), not(test)))] unsafe impl Send for KStatNamed {} #[cfg(all(not(feature = "std"), not(test)))]