Skip to content

Commit 3f9be0a

Browse files
camshaftcamshaft
authored
fix(s2n-quic-transport): determine GSO capability from the segment_len (#1299)
1 parent 701e90f commit 3f9be0a

File tree

11 files changed

+144
-79
lines changed

11 files changed

+144
-79
lines changed

‎quic/s2n-quic-core/src/io/tx.rs

+68-16
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ pub enum Error {
5454
/// The provided message did not write a payload
5555
EmptyPayload,
5656

57+
/// The provided buffer was too small for the desired payload
58+
UndersizedBuffer,
59+
5760
/// The transmission queue is at capacity
5861
AtCapacity,
5962
}
@@ -96,10 +99,47 @@ pub trait Message {
9699
fn ipv6_flow_label(&mut self) -> u32;
97100

98101
/// Returns true if the packet can be used in a GSO packet
99-
fn can_gso(&self) -> bool;
102+
fn can_gso(&self, segment_len: usize) -> bool;
100103

101104
/// Writes the payload of the message to an output buffer
102-
fn write_payload(&mut self, buffer: &mut [u8], gso_offset: usize) -> usize;
105+
fn write_payload(&mut self, buffer: PayloadBuffer, gso_offset: usize) -> Result<usize, Error>;
106+
}
107+
108+
#[derive(Debug)]
109+
pub struct PayloadBuffer<'a>(&'a mut [u8]);
110+
111+
impl<'a> PayloadBuffer<'a> {
112+
#[inline]
113+
pub fn new(bytes: &'a mut [u8]) -> Self {
114+
Self(bytes)
115+
}
116+
117+
/// # Safety
118+
///
119+
/// This function should only be used in the case that the writer has its own safety checks in place
120+
#[inline]
121+
pub unsafe fn into_mut_slice(self) -> &'a mut [u8] {
122+
self.0
123+
}
124+
125+
#[track_caller]
126+
#[inline]
127+
pub fn write(&mut self, bytes: &[u8]) -> Result<usize, Error> {
128+
if bytes.is_empty() {
129+
return Err(Error::EmptyPayload);
130+
}
131+
132+
if let Some(buffer) = self.0.get_mut(0..bytes.len()) {
133+
buffer.copy_from_slice(bytes);
134+
Ok(bytes.len())
135+
} else {
136+
debug_assert!(
137+
false,
138+
"tried to write more bytes than was available in the buffer"
139+
);
140+
Err(Error::UndersizedBuffer)
141+
}
142+
}
103143
}
104144

105145
impl<Handle: path::Handle, Payload: AsRef<[u8]>> Message for (Handle, Payload) {
@@ -121,19 +161,16 @@ impl<Handle: path::Handle, Payload: AsRef<[u8]>> Message for (Handle, Payload) {
121161
0
122162
}
123163

124-
fn can_gso(&self) -> bool {
125-
true
164+
fn can_gso(&self, segment_len: usize) -> bool {
165+
segment_len >= self.1.as_ref().len()
126166
}
127167

128-
fn write_payload(&mut self, buffer: &mut [u8], _gso_offset: usize) -> usize {
129-
let payload = self.1.as_ref();
130-
let len = payload.len();
131-
if let Some(buffer) = buffer.get_mut(..len) {
132-
buffer.copy_from_slice(payload);
133-
len
134-
} else {
135-
0
136-
}
168+
fn write_payload(
169+
&mut self,
170+
mut buffer: PayloadBuffer,
171+
_gso_offset: usize,
172+
) -> Result<usize, Error> {
173+
buffer.write(self.1.as_ref())
137174
}
138175
}
139176

@@ -158,9 +195,24 @@ mod tests {
158195
assert_eq!(message.ecn(), Default::default());
159196
assert_eq!(message.delay(), Default::default());
160197
assert_eq!(message.ipv6_flow_label(), 0);
161-
assert_eq!(message.write_payload(&mut buffer[..], 0), 3);
198+
assert_eq!(
199+
message.write_payload(PayloadBuffer::new(&mut buffer[..]), 0),
200+
Ok(3)
201+
);
202+
}
203+
204+
#[test]
205+
#[should_panic]
206+
fn message_tuple_undersized_test() {
207+
let remote_address = SocketAddressV4::new([127, 0, 0, 1], 80).into();
208+
let local_address = SocketAddressV4::new([192, 168, 0, 1], 3000).into();
209+
let tuple = path::Tuple {
210+
remote_address,
211+
local_address,
212+
};
213+
let mut message = (tuple, [1u8, 2, 3]);
162214

163-
// assert an empty buffer doesn't panic
164-
assert_eq!(message.write_payload(&mut [][..], 0), 0);
215+
// assert an undersized buffer panics in debug
216+
let _ = message.write_payload(PayloadBuffer::new(&mut [][..]), 0);
165217
}
166218
}

‎quic/s2n-quic-platform/src/io/testing/network.rs

+1-7
Original file line numberDiff line numberDiff line change
@@ -348,13 +348,7 @@ impl io::tx::Entry for Packet {
348348
{
349349
self.path.remote_address = message.path_handle().remote_address;
350350
self.ecn = message.ecn();
351-
let len = message.write_payload(&mut self.payload, 0);
352-
353-
if len == 0 {
354-
return Err(io::tx::Error::EmptyPayload);
355-
}
356-
357-
Ok(len)
351+
message.write_payload(io::tx::PayloadBuffer::new(&mut self.payload), 0)
358352
}
359353

360354
fn payload(&self) -> &[u8] {

‎quic/s2n-quic-platform/src/message/mmsg.rs

+1-6
Original file line numberDiff line numberDiff line change
@@ -203,12 +203,7 @@ impl tx::Entry for Message {
203203
) -> Result<usize, tx::Error> {
204204
let payload = MessageTrait::payload_mut(self);
205205

206-
let len = message.write_payload(payload, 0);
207-
208-
// don't send empty payloads
209-
if len == 0 {
210-
return Err(tx::Error::EmptyPayload);
211-
}
206+
let len = message.write_payload(tx::PayloadBuffer::new(payload), 0)?;
212207

213208
unsafe {
214209
debug_assert!(len <= payload.len());

‎quic/s2n-quic-platform/src/message/msg.rs

+1-6
Original file line numberDiff line numberDiff line change
@@ -534,12 +534,7 @@ impl tx::Entry for Message {
534534
) -> Result<usize, tx::Error> {
535535
let payload = MessageTrait::payload_mut(self);
536536

537-
let len = message.write_payload(payload, 0);
538-
539-
// don't send empty payloads
540-
if len == 0 {
541-
return Err(tx::Error::EmptyPayload);
542-
}
537+
let len = message.write_payload(tx::PayloadBuffer::new(payload), 0)?;
543538

544539
unsafe {
545540
debug_assert!(len <= payload.len());

‎quic/s2n-quic-platform/src/message/queue/slice.rs

+15-5
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ impl<'a, Message: message::Message, B> Slice<'a, Message, B> {
156156
let prev_message = &mut self.messages[gso.index];
157157
// check to make sure the message can be GSO'd and can be included in the same
158158
// GSO payload as the previous message
159-
if !(message.can_gso() && prev_message.can_gso(&mut message)) {
159+
if !(message.can_gso(gso.size) && prev_message.can_gso(&mut message)) {
160160
self.flush_gso();
161161
return Ok(Err(message));
162162
}
@@ -178,16 +178,26 @@ impl<'a, Message: message::Message, B> Slice<'a, Message, B> {
178178

179179
// allow the message to write up to `gso.size` bytes
180180
let buffer = &mut message::Message::payload_mut(prev_message)[payload_len..];
181+
let buffer = tx::PayloadBuffer::new(buffer);
181182

182-
match message.write_payload(buffer, gso.count) {
183-
0 => {
183+
match message.write_payload(buffer, gso.count).and_then(|size| {
184+
// we don't want to send empty packets
185+
if size == 0 {
186+
Err(tx::Error::EmptyPayload)
187+
} else {
188+
Ok(size)
189+
}
190+
}) {
191+
Err(err) => {
184192
unsafe {
185193
// revert the len to what it was before
186194
prev_message.set_payload_len(payload_len);
187195
}
188-
Err(tx::Error::EmptyPayload)
196+
Err(err)
189197
}
190-
size => {
198+
Ok(size) => {
199+
debug_assert_ne!(size, 0, "payloads should never be empty");
200+
191201
unsafe {
192202
debug_assert!(
193203
gso.size >= size,

‎quic/s2n-quic-platform/src/message/simple.rs

+1-6
Original file line numberDiff line numberDiff line change
@@ -181,12 +181,7 @@ impl tx::Entry for Message {
181181
) -> Result<usize, tx::Error> {
182182
let payload = MessageTrait::payload_mut(self);
183183

184-
let len = message.write_payload(payload, 0);
185-
186-
// don't send empty payloads
187-
if len == 0 {
188-
return Err(tx::Error::EmptyPayload);
189-
}
184+
let len = message.write_payload(tx::PayloadBuffer::new(payload), 0)?;
190185

191186
unsafe {
192187
debug_assert!(len <= payload.len());

‎quic/s2n-quic-transport/src/connection/close_sender.rs

+13-11
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,8 @@ impl<'a, Config: endpoint::Config, Pub: event::ConnectionPublisher> tx::Message
152152
}
153153

154154
#[inline]
155-
fn can_gso(&self) -> bool {
156-
true
155+
fn can_gso(&self, segment_len: usize) -> bool {
156+
segment_len >= self.packet.len()
157157
}
158158

159159
#[inline]
@@ -162,9 +162,11 @@ impl<'a, Config: endpoint::Config, Pub: event::ConnectionPublisher> tx::Message
162162
}
163163

164164
#[inline]
165-
fn write_payload(&mut self, buffer: &mut [u8], gso_offset: usize) -> usize {
166-
let len = self.packet.len();
167-
165+
fn write_payload(
166+
&mut self,
167+
mut buffer: tx::PayloadBuffer,
168+
gso_offset: usize,
169+
) -> Result<usize, tx::Error> {
168170
//= https://www.rfc-editor.org/rfc/rfc9000#section-10.2.1
169171
//# | Note: Allowing retransmission of a closing packet is an
170172
//# | exception to the requirement that a new packet number be used
@@ -173,7 +175,7 @@ impl<'a, Config: endpoint::Config, Pub: event::ConnectionPublisher> tx::Message
173175
//# | control, which are not expected to be relevant for a closed
174176
//# | connection. Retransmitting the final packet requires less
175177
//# | state.
176-
buffer[..len].copy_from_slice(self.packet);
178+
let len = buffer.write(self.packet)?;
177179

178180
self.path.on_bytes_transmitted(len);
179181
*self.transmission = TransmissionState::Idle;
@@ -184,7 +186,7 @@ impl<'a, Config: endpoint::Config, Pub: event::ConnectionPublisher> tx::Message
184186
gso_offset,
185187
});
186188

187-
len
189+
Ok(len)
188190
}
189191
}
190192

@@ -361,9 +363,9 @@ mod tests {
361363
// transmit an initial packet
362364
assert!(sender.can_transmit(path.transmission_constraint()));
363365
let now = s2n_quic_platform::time::now();
364-
sender
366+
let _ = sender
365367
.transmission(&mut path, now, &mut publisher)
366-
.write_payload(&mut buffer, 0);
368+
.write_payload(tx::PayloadBuffer::new(&mut buffer), 0);
367369

368370
for (gap, packet_size) in events {
369371
// get the next timer event
@@ -383,9 +385,9 @@ mod tests {
383385
for _ in 0..3 {
384386
let interest = sender.get_transmission_interest();
385387
if interest.can_transmit(path.transmission_constraint()) {
386-
sender
388+
let _ = sender
387389
.transmission(&mut path, now, &mut publisher)
388-
.write_payload(&mut buffer, 0);
390+
.write_payload(tx::PayloadBuffer::new(&mut buffer), 0);
389391
transmission_count += 1;
390392
}
391393
}

‎quic/s2n-quic-transport/src/connection/transmission.rs

+20-4
Original file line numberDiff line numberDiff line change
@@ -72,16 +72,31 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<
7272
}
7373

7474
#[inline]
75-
fn can_gso(&self) -> bool {
75+
fn can_gso(&self, segment_len: usize) -> bool {
76+
if let Some(min_packet_len) = self.context.min_packet_len {
77+
if segment_len < min_packet_len {
78+
return false;
79+
}
80+
}
81+
7682
// If a packet can be GSO'd it means it's limited to the previously written packet
7783
// size. This becomes a problem for MTU probes where they will likely exceed that amount.
7884
// As such, if we're probing we want to let the IO layer know to not GSO the current
7985
// packet.
8086
!self.context.transmission_mode.is_mtu_probing()
8187
}
8288

83-
fn write_payload(&mut self, buffer: &mut [u8], gso_offset: usize) -> usize {
89+
fn write_payload(
90+
&mut self,
91+
buffer: tx::PayloadBuffer,
92+
gso_offset: usize,
93+
) -> Result<usize, tx::Error> {
8494
let space_manager = &mut self.space_manager;
95+
let buffer = unsafe {
96+
// the WriterContext has its own checks for buffer capacity so convert `buffer` into a
97+
// `&mut [u8]`
98+
buffer.into_mut_slice()
99+
};
85100

86101
let mtu = self
87102
.context
@@ -361,9 +376,10 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<
361376
len: datagram_len as u16,
362377
gso_offset,
363378
});
379+
Ok(datagram_len)
380+
} else {
381+
Err(tx::Error::EmptyPayload)
364382
}
365-
366-
datagram_len
367383
}
368384
}
369385

‎quic/s2n-quic-transport/src/endpoint/retry.rs

+8-6
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,16 @@ impl<Path: path::Handle> tx::Message for &Transmission<Path> {
157157
}
158158

159159
#[inline]
160-
fn can_gso(&self) -> bool {
161-
true
160+
fn can_gso(&self, segment_len: usize) -> bool {
161+
segment_len >= self.as_ref().len()
162162
}
163163

164164
#[inline]
165-
fn write_payload(&mut self, buffer: &mut [u8], _gso_offset: usize) -> usize {
166-
let packet = self.as_ref();
167-
buffer[..packet.len()].copy_from_slice(packet);
168-
packet.len()
165+
fn write_payload(
166+
&mut self,
167+
mut buffer: tx::PayloadBuffer,
168+
_gso_offset: usize,
169+
) -> Result<usize, tx::Error> {
170+
buffer.write(self.as_ref())
169171
}
170172
}

‎quic/s2n-quic-transport/src/endpoint/stateless_reset.rs

+8-6
Original file line numberDiff line numberDiff line change
@@ -144,14 +144,16 @@ impl<Path: path::Handle> tx::Message for &Transmission<Path> {
144144
}
145145

146146
#[inline]
147-
fn can_gso(&self) -> bool {
148-
true
147+
fn can_gso(&self, segment_len: usize) -> bool {
148+
segment_len >= self.as_ref().len()
149149
}
150150

151151
#[inline]
152-
fn write_payload(&mut self, buffer: &mut [u8], _gso_offset: usize) -> usize {
153-
let packet = self.as_ref();
154-
buffer[..packet.len()].copy_from_slice(packet);
155-
packet.len()
152+
fn write_payload(
153+
&mut self,
154+
mut buffer: tx::PayloadBuffer,
155+
_gso_offset: usize,
156+
) -> Result<usize, tx::Error> {
157+
buffer.write(self.as_ref())
156158
}
157159
}

‎quic/s2n-quic-transport/src/endpoint/version.rs

+8-6
Original file line numberDiff line numberDiff line change
@@ -259,15 +259,17 @@ impl<Path: path::Handle> tx::Message for &Transmission<Path> {
259259
}
260260

261261
#[inline]
262-
fn can_gso(&self) -> bool {
263-
true
262+
fn can_gso(&self, segment_len: usize) -> bool {
263+
segment_len >= self.as_ref().len()
264264
}
265265

266266
#[inline]
267-
fn write_payload(&mut self, buffer: &mut [u8], _gso_offset: usize) -> usize {
268-
let packet = self.as_ref();
269-
buffer[..packet.len()].copy_from_slice(packet);
270-
packet.len()
267+
fn write_payload(
268+
&mut self,
269+
mut buffer: tx::PayloadBuffer,
270+
_gso_offset: usize,
271+
) -> Result<usize, tx::Error> {
272+
buffer.write(self.as_ref())
271273
}
272274
}
273275

0 commit comments

Comments
 (0)