Skip to content

Commit 54ca9c9

Browse files
matheus23flub
andauthored
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 <[email protected]>
1 parent cb1650a commit 54ca9c9

File tree

2 files changed

+16
-8
lines changed

2 files changed

+16
-8
lines changed

iroh-net/src/magicsock.rs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -697,17 +697,23 @@ impl MagicSock {
697697
let mut quic_packets_total = 0;
698698

699699
for (meta, buf) in metas.iter_mut().zip(bufs.iter_mut()).take(msgs) {
700-
let mut start = 0;
701700
let mut is_quic = false;
702701
let mut quic_packets_count = 0;
702+
if meta.len > meta.stride {
703+
trace!(%meta.len, %meta.stride, "GRO datagram received");
704+
inc!(MagicsockMetrics, recv_gro_datagrams);
705+
}
703706

704707
// find disco and stun packets and forward them to the actor
705-
loop {
706-
let end = start + meta.stride;
707-
if end > meta.len {
708-
break;
708+
for packet in buf[..meta.len].chunks_mut(meta.stride) {
709+
if packet.len() < meta.stride {
710+
trace!(
711+
len = %packet.len(),
712+
%meta.stride,
713+
"Last GRO datagram smaller than stride",
714+
);
709715
}
710-
let packet = &buf[start..end];
716+
711717
let packet_is_quic = if stun::is(packet) {
712718
trace!(src = %meta.addr, len = %meta.stride, "UDP recv: stun packet");
713719
let packet2 = Bytes::copy_from_slice(packet);
@@ -740,9 +746,8 @@ impl MagicSock {
740746
// this makes quinn reliably and quickly ignore the packet as long as
741747
// [`quinn::EndpointConfig::grease_quic_bit`] is set to `false`
742748
// (which we always do in Endpoint::bind).
743-
buf[start] = 0u8;
749+
packet[0] = 0u8;
744750
}
745-
start = end;
746751
}
747752

748753
if is_quic {

iroh-net/src/magicsock/metrics.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ pub struct Metrics {
2424
pub recv_data_ipv6: Counter,
2525
/// Number of QUIC datagrams received.
2626
pub recv_datagrams: Counter,
27+
/// Number of datagrams received using GRO
28+
pub recv_gro_datagrams: Counter,
2729

2830
// Disco packets
2931
pub send_disco_udp: Counter,
@@ -90,6 +92,7 @@ impl Default for Metrics {
9092
recv_data_ipv4: Counter::new("recv_data_ipv4"),
9193
recv_data_ipv6: Counter::new("recv_data_ipv6"),
9294
recv_datagrams: Counter::new("recv_datagrams"),
95+
recv_gro_datagrams: Counter::new("recv_gro_packets"),
9396

9497
// Disco packets
9598
send_disco_udp: Counter::new("disco_send_udp"),

0 commit comments

Comments
 (0)