Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions lib/opte/src/ddi/kstat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,26 @@ impl From<alloc::ffi::NulError> for Error {
Self::NulChar
}
}

// SAFETY: KStatNamed<T> 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.
Comment on lines +310 to +327
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be more concisely summarised, after applying the other suggested change, as a statement that:

  • 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.
  • 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 as you point out above.

#[cfg(all(not(feature = "std"), not(test)))]
unsafe impl<T: KStatProvider> Send for KStatNamed<T> {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, currently, this bound is far too broad (i.e., if T is not Send or Sync, neither should KStatNamed be). I believe the correct narrowing might be that we impose, higher in this file, that pub trait KStatProvider: Send + Sync, since the data associated with each kstat will need to be accessed in at least one other kernel thread if there's a kstat invocation.

#[cfg(all(not(feature = "std"), not(test)))]
unsafe impl<T: KStatProvider> Sync for KStatNamed<T> {}
2 changes: 1 addition & 1 deletion lib/opte/src/engine/flow_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl Ttl {
}

/// A policy for expiring flow table entries over time.
pub trait ExpiryPolicy<S: Dump>: fmt::Debug {
pub trait ExpiryPolicy<S: Dump>: 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<S>, now: Moment) -> bool;
Expand Down
10 changes: 5 additions & 5 deletions lib/opte/src/engine/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -742,7 +742,7 @@ pub enum GenDescError {

pub type GenDescResult = ActionResult<Arc<dyn ActionDesc>, 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.
Expand Down Expand Up @@ -772,7 +772,7 @@ pub enum GenHtError {

pub type GenHtResult = ActionResult<HdrTransform, GenHtError>;

pub trait StaticAction: Display {
pub trait StaticAction: Display + Send + Sync {
fn gen_ht(
&self,
dir: Direction,
Expand All @@ -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
Expand Down Expand Up @@ -843,7 +843,7 @@ impl From<smoltcp::wire::Error> 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.
Expand Down
8 changes: 4 additions & 4 deletions lib/opte/src/engine/snat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ impl From<GenIcmpErr> for GenDescError {
}
}

impl<T: ConcreteIpAddr + 'static> SNat<T> {
impl<T: ConcreteIpAddr + 'static + Send + Sync> SNat<T> {
pub fn new(addr: T) -> Self {
SNat {
priv_ip: addr,
Expand Down Expand Up @@ -299,7 +299,7 @@ impl Display for SNat<Ipv6Addr> {
}
}

impl<T: ConcreteIpAddr + 'static> StatefulAction for SNat<T>
impl<T: ConcreteIpAddr + 'static + Send + Sync> StatefulAction for SNat<T>
where
SNat<T>: Display,
{
Expand Down Expand Up @@ -367,7 +367,7 @@ pub struct SNatDesc<T: ConcreteIpAddr> {

pub const SNAT_NAME: &str = "SNAT";

impl<T: ConcreteIpAddr> ActionDesc for SNatDesc<T> {
impl<T: ConcreteIpAddr + Send + Sync> ActionDesc for SNatDesc<T> {
fn gen_ht(&self, dir: Direction) -> HdrTransform {
match dir {
// Outbound traffic needs its source IP and source port
Expand Down Expand Up @@ -425,7 +425,7 @@ pub struct SNatIcmpEchoDesc<T: ConcreteIpAddr> {

pub const SNAT_ICMP_ECHO_NAME: &str = "SNAT_ICMP_ECHO";

impl<T: ConcreteIpAddr> ActionDesc for SNatIcmpEchoDesc<T> {
impl<T: ConcreteIpAddr + Send + Sync> ActionDesc for SNatIcmpEchoDesc<T> {
// 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 {
Expand Down
2 changes: 1 addition & 1 deletion lib/opte/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down