From 54ca9c942fa7ce6e43825d1374483a049f4d4c13 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= <philipp.krueger1@gmail.com>
Date: Mon, 26 Aug 2024 16:41:38 +0200
Subject: [PATCH] fix(iroh-net): Also check the last packet in
 `MagicSock::poll_recv` (#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 <flub@n0.computer>
---
 iroh-net/src/magicsock.rs         | 21 +++++++++++++--------
 iroh-net/src/magicsock/metrics.rs |  3 +++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/iroh-net/src/magicsock.rs b/iroh-net/src/magicsock.rs
index ef9da018fb..ecaf2712bc 100644
--- a/iroh-net/src/magicsock.rs
+++ b/iroh-net/src/magicsock.rs
@@ -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);
@@ -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 {
diff --git a/iroh-net/src/magicsock/metrics.rs b/iroh-net/src/magicsock/metrics.rs
index 0f3b8b40f1..5c0a97148f 100644
--- a/iroh-net/src/magicsock/metrics.rs
+++ b/iroh-net/src/magicsock/metrics.rs
@@ -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,
@@ -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"),