From 87a4af7926ac8f54d505e1ef65f2e87d18d42fe2 Mon Sep 17 00:00:00 2001 From: Jean-Pierre Smith Date: Sat, 16 Dec 2023 22:38:30 +0100 Subject: [PATCH] refactor: use path buffer provided by caller in recv*_with_path This commit modifies the path types to be generic over the underlying storage. This enables us to store paths in buffers provided by a caller as opposed to needing to allocate storage for paths. --- crates/scion-proto/src/path.rs | 11 +- crates/scion-proto/src/path/dataplane.rs | 101 +++++++++--- crates/scion-proto/src/path/standard.rs | 190 +++++++++++++++-------- crates/scion/src/udp_socket.rs | 110 ++++++++----- crates/scion/tests/test_udp_socket.rs | 4 +- 5 files changed, 288 insertions(+), 128 deletions(-) diff --git a/crates/scion-proto/src/path.rs b/crates/scion-proto/src/path.rs index 4120469..b8e323d 100644 --- a/crates/scion-proto/src/path.rs +++ b/crates/scion-proto/src/path.rs @@ -28,9 +28,9 @@ pub use epic::EpicAuths; /// A SCION end-to-end path with optional metadata. #[derive(Debug, Clone, PartialEq)] -pub struct Path { +pub struct Path { /// The raw bytes to be added as the path header to SCION dataplane packets. - pub dataplane_path: DataplanePath, + pub dataplane_path: DataplanePath, /// The underlay address (IP + port) of the next hop; i.e., the local border router. pub underlay_next_hop: Option, /// The ISD-ASN where the path starts and ends. @@ -40,9 +40,9 @@ pub struct Path { } #[allow(missing_docs)] -impl Path { +impl Path { pub fn new( - dataplane_path: DataplanePath, + dataplane_path: DataplanePath, isd_asn: ByEndpoint, underlay_next_hop: Option, ) -> Self { @@ -76,7 +76,10 @@ impl Path { pub fn is_empty(&self) -> bool { self.dataplane_path.is_empty() } +} +#[allow(missing_docs)] +impl Path { #[tracing::instrument] pub fn try_from_grpc_with_endpoints( mut value: daemon_grpc::Path, diff --git a/crates/scion-proto/src/path/dataplane.rs b/crates/scion-proto/src/path/dataplane.rs index a35afe9..da50571 100644 --- a/crates/scion-proto/src/path/dataplane.rs +++ b/crates/scion-proto/src/path/dataplane.rs @@ -1,5 +1,7 @@ //! Types and functions for SCION dataplane paths. +use std::ops::Deref; + use bytes::{Buf, BufMut, Bytes}; use crate::{ @@ -60,32 +62,26 @@ pub struct UnsupportedPathType(pub u8); /// Dataplane path found in a SCION packet. #[derive(Debug, Clone, PartialEq)] -pub enum DataplanePath { +pub enum DataplanePath { /// The empty path type, used for intra-AS hops. EmptyPath, /// The standard SCION path header. - Standard(StandardPath), + Standard(StandardPath), /// The raw bytes of an unsupported path header type. Unsupported { /// The path's type. path_type: PathType, /// The raw encoded path. - bytes: Bytes, + bytes: T, }, } -impl DataplanePath { - /// Returns a deep copy of the object. - pub fn deep_copy(&self) -> Self { - match self { - Self::EmptyPath => Self::EmptyPath, - Self::Standard(path) => Self::Standard(path.deep_copy()), - Self::Unsupported { path_type, bytes } => Self::Unsupported { - path_type: *path_type, - bytes: Bytes::copy_from_slice(bytes), - }, - } - } +impl DataplanePath { + /// The maximum length of a SCION dataplane path. + /// + /// Computed from the max header length (1020) minus the common header length (12) + /// and the minimum SCION address header length (24). + pub const MAX_LEN: usize = 984; /// Returns the path's type. pub fn path_type(&self) -> PathType { @@ -96,14 +92,69 @@ impl DataplanePath { } } + /// Returns true iff the path is a [`DataplanePath::EmptyPath`]. + pub fn is_empty(&self) -> bool { + matches!(self, Self::EmptyPath) + } +} + +impl DataplanePath +where + T: Deref, +{ + /// Returns the raw binary of the path. + pub fn raw(&self) -> &[u8] { + match self { + DataplanePath::EmptyPath => &[], + DataplanePath::Standard(path) => path.raw(), + DataplanePath::Unsupported { bytes, .. } => bytes.deref(), + } + } + + /// Creates a new DataplanePath by copying this one into the provided backing buffer. + /// + /// # Panics + /// + /// For non-empty paths, this panics if the provided buffer does not have the same + /// length as self.raw(). + pub fn copy_to_slice<'b>(&self, buffer: &'b mut [u8]) -> DataplanePath<&'b mut [u8]> { + match self { + DataplanePath::EmptyPath => DataplanePath::EmptyPath, + DataplanePath::Standard(path) => DataplanePath::Standard(path.copy_to_slice(buffer)), + DataplanePath::Unsupported { path_type, bytes } => { + buffer.copy_from_slice(bytes); + DataplanePath::Unsupported { + path_type: *path_type, + bytes: buffer, + } + } + } + } + /// Reverses the path. - pub fn to_reversed(&self) -> Result { + pub fn to_reversed(&self) -> Result { match self { - Self::EmptyPath => Ok(Self::EmptyPath), - Self::Standard(standard_path) => Ok(Self::Standard(standard_path.to_reversed())), + Self::EmptyPath => Ok(DataplanePath::EmptyPath), + Self::Standard(standard_path) => { + Ok(DataplanePath::Standard(standard_path.to_reversed())) + } Self::Unsupported { path_type, .. } => Err(UnsupportedPathType(u8::from(*path_type))), } } +} + +impl DataplanePath { + /// Returns a deep copy of the object. + pub fn deep_copy(&self) -> Self { + match self { + Self::EmptyPath => Self::EmptyPath, + Self::Standard(path) => Self::Standard(path.deep_copy()), + Self::Unsupported { path_type, bytes } => Self::Unsupported { + path_type: *path_type, + bytes: Bytes::copy_from_slice(bytes), + }, + } + } /// Reverses the path in place. pub fn reverse(&mut self) -> Result<&mut Self, UnsupportedPathType> { @@ -116,10 +167,18 @@ impl DataplanePath { } Ok(self) } +} - /// Returns true iff the path is a [`DataplanePath::EmptyPath`]. - pub fn is_empty(&self) -> bool { - self == &Self::EmptyPath +impl From> for DataplanePath { + fn from(value: DataplanePath<&mut [u8]>) -> Self { + match value { + DataplanePath::EmptyPath => DataplanePath::EmptyPath, + DataplanePath::Standard(path) => DataplanePath::Standard(path.into()), + DataplanePath::Unsupported { path_type, bytes } => DataplanePath::Unsupported { + path_type, + bytes: Bytes::copy_from_slice(bytes), + }, + } } } diff --git a/crates/scion-proto/src/path/standard.rs b/crates/scion-proto/src/path/standard.rs index 54c4402..6bdded7 100644 --- a/crates/scion-proto/src/path/standard.rs +++ b/crates/scion-proto/src/path/standard.rs @@ -1,8 +1,8 @@ //! A standard SCION path. -use std::mem; +use std::{mem, ops::Deref, slice::ChunksExact}; -use bytes::{Buf, BufMut, Bytes, BytesMut}; +use bytes::{Buf, BufMut, Bytes}; use super::DataplanePathErrorKind; use crate::{ @@ -145,6 +145,20 @@ impl PathMetaHeader { .last() .unwrap() } + + fn to_reversed(&self) -> Self { + Self { + current_info_field: InfoFieldIndex(0), + current_hop_field: HopFieldIndex(0), + reserved: PathMetaReserved::default(), + segment_lengths: match self.segment_lengths { + [SegmentLength(0), ..] => [SegmentLength(0); 3], + [s1, SegmentLength(0), ..] => [s1, SegmentLength(0), SegmentLength(0)], + [s1, s2, SegmentLength(0)] => [s2, s1, SegmentLength(0)], + [s1, s2, s3] => [s3, s2, s1], + }, + } + } } impl WireEncode for PathMetaHeader { @@ -225,90 +239,140 @@ const fn nth_field(fields: u32) -> u8 { /// /// Consists of a [`PathMetaHeader`] along with one or more info fields and hop fields. #[derive(Debug, Clone, PartialEq)] -pub struct StandardPath { +pub struct StandardPath { /// The meta information about the stored path. meta_header: PathMetaHeader, /// The raw data containing the meta_header, info, and hop fields. - encoded_path: Bytes, + encoded_path: T, } -impl StandardPath { +impl StandardPath { /// Returns the metadata about the stored path. pub fn meta_header(&self) -> &PathMetaHeader { &self.meta_header } +} - /// Creates a deep copy of this path. - pub fn deep_copy(&self) -> Self { - Self { +impl StandardPath +where + T: Deref, +{ + /// Returns the encoded raw path. + pub fn raw(&self) -> &[u8] { + &self.encoded_path + } + + /// Creates new StandardPath, backed by the provided buffer, by copying this one. + /// + /// # Panics + /// + /// Panics if the provided buffer does not have the same length as self.raw(). + pub fn copy_to_slice<'b>(&self, buffer: &'b mut [u8]) -> StandardPath<&'b mut [u8]> { + buffer.copy_from_slice(&self.encoded_path); + StandardPath { meta_header: self.meta_header.clone(), - encoded_path: Bytes::copy_from_slice(&self.encoded_path), + encoded_path: buffer, } } - /// Returns the encoded raw path. - pub fn raw(&self) -> Bytes { - self.encoded_path.clone() + /// Creates new StandardPath, backed by the provided buffer, by copying and reversing this one. + /// + /// The reversed path is suitable for use from an end-host: its current hop and info field + /// indices are set to 0. + /// + /// # Panics + /// + /// Panics if the provided buffer does not have the same length as self.raw(). + pub fn reverse_to_slice<'b>(&self, buffer: &'b mut [u8]) -> StandardPath<&'b mut [u8]> { + assert_eq!( + buffer.len(), + self.encoded_path.len(), + "destination buffer length is not the same as this path's" + ); + + let mut buf_mut: &mut [u8] = buffer; + let meta_header = self.meta_header.to_reversed(); + meta_header.encode_to_unchecked(&mut buf_mut); + self.write_reversed_info_fields_to(&mut buf_mut); + self.write_reversed_hop_fields_to(&mut buf_mut); + + StandardPath { + meta_header, + encoded_path: buffer, + } } /// Reverses both the raw path and the metadata in the [`Self::meta_header`]. /// - /// Can panic if the meta header is inconsistent with the encoded path or the encoded path - /// itself is inconsistent (e.g., the `current_info_field` points to an empty segment). - pub fn to_reversed(&self) -> Self { - let meta_header = PathMetaHeader { - current_info_field: (self.meta_header.info_fields_count() as u8 - - self.meta_header.current_info_field.get() - - 1) - .into(), - current_hop_field: (self.meta_header.hop_fields_count() as u8 - - self.meta_header.current_hop_field.get() - - 1) - .into(), - reserved: PathMetaReserved::default(), - segment_lengths: match self.meta_header.segment_lengths { - [SegmentLength(0), ..] => [SegmentLength(0); 3], - [s1, SegmentLength(0), ..] => [s1, SegmentLength(0), SegmentLength(0)], - [s1, s2, SegmentLength(0)] => [s2, s1, SegmentLength(0)], - [s1, s2, s3] => [s3, s2, s1], - }, - }; - - let mut encoded_path = BytesMut::with_capacity(self.encoded_path.len()); - meta_header.encode_to_unchecked(&mut encoded_path); - self.write_reversed_info_fields_to(&mut encoded_path); - self.write_reversed_hop_fields_to(&mut encoded_path); + /// The reversed path is suitable for use from an end-host: its current hop and info field + /// indices are set to 0. + pub fn to_reversed(&self) -> StandardPath { + let mut encoded_path = vec![0u8; self.encoded_path.len()]; + let StandardPath { meta_header, .. } = self.reverse_to_slice(&mut encoded_path); - Self { + StandardPath { meta_header, - encoded_path: encoded_path.freeze(), + encoded_path: encoded_path.into(), } } + #[inline] + fn info_fields(&self) -> ChunksExact<'_, u8> { + let start = PathMetaHeader::info_field_offset(0); + let stop = start + self.meta_header.info_fields_count() * PathMetaHeader::INFO_FIELD_LENGTH; + self.encoded_path[start..stop].chunks_exact(PathMetaHeader::INFO_FIELD_LENGTH) + } + + #[inline] + fn hop_fields(&self) -> ChunksExact<'_, u8> { + let start = self.meta_header().hop_field_offset(0); + let stop = start + self.meta_header.hop_fields_count() * PathMetaHeader::HOP_FIELD_LENGTH; + self.encoded_path[start..stop].chunks_exact(PathMetaHeader::HOP_FIELD_LENGTH) + } + /// Writes the info fields to the provided buffer in reversed order. /// /// This also flips the "construction direction flag" for all info fields. - fn write_reversed_info_fields_to(&self, buffer: &mut BytesMut) { - for info_field in (0..self.meta_header.info_fields_count()).rev() { - let offset = PathMetaHeader::info_field_offset(info_field); - let slice = &self - .encoded_path - .slice(offset..offset + PathMetaHeader::INFO_FIELD_LENGTH); - - buffer.put_u8(slice[0] ^ 0b1); // Flip construction direction flag - buffer.put_slice(&slice[1..]); + fn write_reversed_info_fields_to(&self, buffer: &mut &mut [u8]) { + for src_slice in self.info_fields().rev() { + buffer.put_u8(src_slice[0] ^ 0b1); // Flip construction direction flag + buffer.put_slice(&src_slice[1..]); } } /// Writes the hop fields to the provided buffer in reversed order. - fn write_reversed_hop_fields_to(&self, buffer: &mut BytesMut) { - for hop_field in (0..self.meta_header.hop_fields_count()).rev() { - let offset = self.meta_header().hop_field_offset(hop_field); - buffer.put_slice( - &self - .encoded_path - .slice(offset..offset + PathMetaHeader::HOP_FIELD_LENGTH), - ) + fn write_reversed_hop_fields_to(&self, buffer: &mut &mut [u8]) { + for src_slice in self.hop_fields().rev() { + buffer.put_slice(src_slice) + } + } +} + +impl<'b> StandardPath<&'b mut [u8]> { + /// Converts a standard path over a mutable reference to one over an immutable reference. + pub fn freeze(self) -> StandardPath<&'b [u8]> { + StandardPath { + meta_header: self.meta_header, + encoded_path: &*self.encoded_path, + } + } +} + +impl StandardPath { + /// Creates a deep copy of this path. + pub fn deep_copy(&self) -> Self { + Self { + meta_header: self.meta_header.clone(), + encoded_path: Bytes::copy_from_slice(&self.encoded_path), + } + } +} + +impl From> for StandardPath { + fn from(value: StandardPath<&mut [u8]>) -> Self { + StandardPath { + meta_header: value.meta_header, + encoded_path: Bytes::copy_from_slice(value.encoded_path), } } } @@ -386,13 +450,17 @@ mod tests { } #[test] - fn reverse_twice_identity() { + fn reverse_twice_field_identity() { let mut data = $encoded_path; - let header = StandardPath::decode(&mut data).expect("valid decode"); + let path = StandardPath::decode(&mut data).expect("valid decode"); - let reverse_path = header.to_reversed(); - assert!(header != reverse_path); - assert_eq!(header, reverse_path.to_reversed()); + let twice_reversed = path.to_reversed().to_reversed(); + assert!(path.hop_fields().eq(twice_reversed.hop_fields())); + assert!(path.info_fields().eq(twice_reversed.info_fields())); + assert_eq!( + path.meta_header.segment_lengths, + twice_reversed.meta_header.segment_lengths + ); } } }; diff --git a/crates/scion/src/udp_socket.rs b/crates/scion/src/udp_socket.rs index c21c6f8..7d9d4ed 100644 --- a/crates/scion/src/udp_socket.rs +++ b/crates/scion/src/udp_socket.rs @@ -14,7 +14,7 @@ use scion_proto::{ address::SocketAddr, datagram::{UdpDatagram, UdpEncodeError}, packet::{self, ByEndpoint, EncodeError, ScionPacketRaw, ScionPacketUdp}, - path::{Path, UnsupportedPathType}, + path::{DataplanePath, Path, UnsupportedPathType}, reliable::Packet, wire_encoding::WireDecode, }; @@ -110,7 +110,7 @@ impl UdpSocket { /// data is dropped. The returned number of bytes always refers to the amount of data in the UDP /// payload. pub async fn recv(&self, buffer: &mut [u8]) -> Result { - let (packet_len, _) = self.inner.recv_from(buffer).await?; + let (packet_len, _) = self.recv_from(buffer).await?; Ok(packet_len) } @@ -118,7 +118,10 @@ impl UdpSocket { /// /// This behaves like [`Self::recv`] but additionally returns the remote SCION socket address. pub async fn recv_from(&self, buffer: &mut [u8]) -> Result<(usize, SocketAddr), ReceiveError> { - self.inner.recv_from(buffer).await + self.inner + .recv_loop(buffer, None) + .await + .map(|(len, addr, _)| (len, addr)) } /// Receive a SCION UDP packet from a remote endpoint with path information. @@ -128,14 +131,40 @@ impl UdpSocket { /// - the path over which the packet was received. For supported path types, this path is /// already reversed such that it can be used directly to send reply packets; for unsupported /// path types, the path is copied unmodified. - /// - /// Note that copying/reversing the path requires allocating memory; if you do not need the path - /// information, consider using the method [`Self::recv_from`] instead. pub async fn recv_from_with_path( &self, buffer: &mut [u8], ) -> Result<(usize, SocketAddr, Path), ReceiveError> { - self.inner.recv_from_with_path(buffer).await + if buffer.is_empty() { + return Err(ReceiveError::ZeroLengthBuffer); + } + if buffer.len() < DataplanePath::::MAX_LEN { + return Err(ReceiveError::PathBufferTooShort); + } + + // TODO(jsmith): Refactor to accept two buffers and return a Path referring into one. + let split_point = buffer.len() - DataplanePath::::MAX_LEN; + let (buffer, path_buf) = buffer.split_at_mut(split_point); + + let (packet_len, sender, path) = self.inner.recv_loop(buffer, Some(path_buf)).await?; + let Path { + dataplane_path, + underlay_next_hop, + isd_asn, + .. + } = path.expect("non-None path since path_buf was provided"); + + // Explicit match here in case we add other errors to the `reverse` method at some point + let dataplane_path = match dataplane_path.to_reversed() { + Ok(reversed_dataplane) => reversed_dataplane, + Err(UnsupportedPathType(_)) => dataplane_path.into(), + }; + + Ok(( + packet_len, + sender, + Path::new(dataplane_path, isd_asn.into_reversed(), underlay_next_hop), + )) } /// Receive a SCION UDP packet with path information. @@ -143,11 +172,8 @@ impl UdpSocket { /// This behaves like [`Self::recv`] but additionally returns the path over which the packet was /// received. For supported path types, this path is already reversed such that it can be used /// directly to send reply packets; for unsupported path types, the path is copied unmodified. - /// - /// Note that copying/reversing the path requires allocating memory; if you do not need the path - /// information, consider using the method [`Self::recv`] instead. pub async fn recv_with_path(&self, buffer: &mut [u8]) -> Result<(usize, Path), ReceiveError> { - let (packet_len, _, path) = self.inner.recv_from_with_path(buffer).await?; + let (packet_len, _, path) = self.recv_from_with_path(buffer).await?; Ok((packet_len, path)) } @@ -218,6 +244,8 @@ impl UdpSocket { pub enum ReceiveError { #[error("attempted to receive with a zero-length buffer")] ZeroLengthBuffer, + #[error("path buffer too short")] + PathBufferTooShort, } macro_rules! log_err { @@ -293,47 +321,44 @@ impl UdpSocketInner { Ok(()) } - async fn recv_from_with_path( + async fn recv_loop<'p>( &self, buf: &mut [u8], - ) -> Result<(usize, SocketAddr, Path), ReceiveError> { - let (packet_len, sender, mut path) = self.recv_loop(buf).await?; - // Explicit match here in case we add other errors to the `reverse` method at some point - match path.dataplane_path.reverse() { - Ok(_) => { - path.isd_asn.reverse(); - } - Err(UnsupportedPathType(_)) => path.dataplane_path = path.dataplane_path.deep_copy(), - }; - Ok((packet_len, sender, path)) - } - - async fn recv_from(&self, buf: &mut [u8]) -> Result<(usize, SocketAddr), ReceiveError> { - let (packet_len, sender, ..) = self.recv_loop(buf).await?; - Ok((packet_len, sender)) - } - - async fn recv_loop(&self, buf: &mut [u8]) -> Result<(usize, SocketAddr, Path), ReceiveError> { + path_buf: Option<&'p mut [u8]>, + ) -> Result<(usize, SocketAddr, Option>), ReceiveError> { if buf.is_empty() { return Err(ReceiveError::ZeroLengthBuffer); } + if let Some(path_buf) = path_buf.as_ref() { + if path_buf.len() < DataplanePath::::MAX_LEN { + return Err(ReceiveError::PathBufferTooShort); + } + } // Keep a copy of the connection's remote_addr locally, so that the user connecting to a // different destination does not affect what this call should return. let remote_addr = self.state.read().unwrap().remote_address; loop { - let receive_result = { - let stream = &mut *self.stream.lock().await; - stream.receive_packet().await - }; + // Keep the lock until we no longer have a dependency on the internal buffers. + let mut stream = self.stream.lock().await; + let receive_result = stream.receive_packet().await; match receive_result { Ok(packet) => { if let Some((packet_len, sender, path)) = - self.parse_incoming_for(packet, buf, remote_addr) + self.parse_incoming_from(packet, buf, remote_addr) { - return Ok((packet_len, sender, path)); + if let Some(path_buf) = path_buf { + let path_len = path.dataplane_path.raw().len(); + let dataplane_path = + path.dataplane_path.copy_to_slice(&mut path_buf[..path_len]); + let path = + Path::new(dataplane_path, path.isd_asn, path.underlay_next_hop); + return Ok((packet_len, sender, Some(path))); + } else { + return Ok((packet_len, sender, None)); + } } else { continue; } @@ -343,7 +368,12 @@ impl UdpSocketInner { } } - fn parse_incoming_for( + /// Parse a datagram from the provided packet. + /// + /// # Panics + /// + /// Panics if path_buf, when not empty, does not have sufficient length for the SCION path. + fn parse_incoming_from( &self, mut packet: Packet, buf: &mut [u8], @@ -599,7 +629,7 @@ mod tests { }; assert_eq!(endpoints.source.isd_asn(), endpoints.destination.isd_asn()); - let mut buffer = [0u8; 64]; + let mut buffer = vec![0u8; 1500]; let (socket, mut dispatcher) = utils::socket_from(endpoints.source)?; utils::local_send_raw(&mut dispatcher, endpoints, MESSAGE).await?; @@ -643,7 +673,7 @@ mod tests { }; assert_eq!(other_endpoints.source.isd_asn(), endpoints.source.isd_asn()); - let mut buffer = [0u8; 64]; + let mut buffer = vec![0u8; 1500]; let (socket, mut dispatcher) = utils::socket_from(endpoints.source)?; // Write packets to be received @@ -709,7 +739,7 @@ mod tests { }; assert_eq!(endpoints.source.isd_asn(), endpoints.destination.isd_asn()); - let mut buffer = [0u8; 64]; + let mut buffer = vec![0u8; 1500]; let (socket, mut dispatcher) = utils::socket_from(endpoints.source)?; utils::local_send_raw(&mut dispatcher, endpoints, MESSAGE).await?; diff --git a/crates/scion/tests/test_udp_socket.rs b/crates/scion/tests/test_udp_socket.rs index 6cd1726..038d399 100644 --- a/crates/scion/tests/test_udp_socket.rs +++ b/crates/scion/tests/test_udp_socket.rs @@ -55,7 +55,7 @@ macro_rules! test_send_receive_reply { let (socket_source, socket_destination, ..) = get_sockets().await?; socket_source.send(MESSAGE.clone()).await?; - let mut buffer = [0_u8; 100]; + let mut buffer = vec![0_u8; 1500]; let (length, sender) = tokio::time::timeout(TIMEOUT, socket_destination.recv_from(&mut buffer)) .await??; @@ -72,7 +72,7 @@ macro_rules! test_send_receive_reply { let (socket_source, socket_destination, path_forward) = get_sockets().await?; socket_source.send(MESSAGE.clone()).await?; - let mut buffer = [0_u8; 100]; + let mut buffer = vec![0_u8; 1500]; let (length, sender, path) = tokio::time::timeout( TIMEOUT, socket_destination.recv_from_with_path(&mut buffer),