Skip to content

Commit

Permalink
fix(iroh-net): Also check the last packet in MagicSock::poll_recv (#…
Browse files Browse the repository at this point in the history
…2650)

## Description

<!-- A summary of what this pull request achieves and a rough list of
changes. -->

These are the quinn docs for `RecvMeta::stride`:
```rs
/// The size of a single datagram in the associated buffer
///
/// When GRO (Generic Receive Offload) is used this indicates the size of a single
/// datagram inside the buffer. If the buffer is larger, that is if [`len`] is greater
/// then this value, then the individual datagrams contained have their boundaries at
/// `stride` increments from the start. The last datagram could be smaller than
/// `stride`.
///
/// [`len`]: RecvMeta::len
```

So, `meta.len` isn't necessarily evenly divided by `meta.stride`, and
the last packet could be smaller than the stride.

The previous code assumed so, however. I think that's a bug. Not bad
enough that this caused issues, but still bad.

## Breaking Changes

None
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

What are the exact effects of this code having been incorrect before?

I guess one piece is that the metrics for computing the # received bytes
was way off.

Should we test this somehow specifically?
<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- ~~[ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.~~
- ~~[ ] Tests if relevant.~~
- [x] All breaking changes documented.

---------

Co-authored-by: Floris Bruynooghe <[email protected]>
  • Loading branch information
matheus23 and flub authored Aug 26, 2024
1 parent cb1650a commit 54ca9c9
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 8 deletions.
21 changes: 13 additions & 8 deletions iroh-net/src/magicsock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -697,17 +697,23 @@ impl MagicSock {
let mut quic_packets_total = 0;

for (meta, buf) in metas.iter_mut().zip(bufs.iter_mut()).take(msgs) {
let mut start = 0;
let mut is_quic = false;
let mut quic_packets_count = 0;
if meta.len > meta.stride {
trace!(%meta.len, %meta.stride, "GRO datagram received");
inc!(MagicsockMetrics, recv_gro_datagrams);
}

// find disco and stun packets and forward them to the actor
loop {
let end = start + meta.stride;
if end > meta.len {
break;
for packet in buf[..meta.len].chunks_mut(meta.stride) {
if packet.len() < meta.stride {
trace!(
len = %packet.len(),
%meta.stride,
"Last GRO datagram smaller than stride",
);
}
let packet = &buf[start..end];

let packet_is_quic = if stun::is(packet) {
trace!(src = %meta.addr, len = %meta.stride, "UDP recv: stun packet");
let packet2 = Bytes::copy_from_slice(packet);
Expand Down Expand Up @@ -740,9 +746,8 @@ impl MagicSock {
// this makes quinn reliably and quickly ignore the packet as long as
// [`quinn::EndpointConfig::grease_quic_bit`] is set to `false`
// (which we always do in Endpoint::bind).
buf[start] = 0u8;
packet[0] = 0u8;
}
start = end;
}

if is_quic {
Expand Down
3 changes: 3 additions & 0 deletions iroh-net/src/magicsock/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ pub struct Metrics {
pub recv_data_ipv6: Counter,
/// Number of QUIC datagrams received.
pub recv_datagrams: Counter,
/// Number of datagrams received using GRO
pub recv_gro_datagrams: Counter,

// Disco packets
pub send_disco_udp: Counter,
Expand Down Expand Up @@ -90,6 +92,7 @@ impl Default for Metrics {
recv_data_ipv4: Counter::new("recv_data_ipv4"),
recv_data_ipv6: Counter::new("recv_data_ipv6"),
recv_datagrams: Counter::new("recv_datagrams"),
recv_gro_datagrams: Counter::new("recv_gro_packets"),

// Disco packets
send_disco_udp: Counter::new("disco_send_udp"),
Expand Down

0 comments on commit 54ca9c9

Please sign in to comment.