Automatic MTU detection linux

[Serock3]

Fixes DES-581.

---

This change is 

[linear]

DES-581 Automatic MTU detection for WireGuard on Linux

[dlon]

Reviewed 7 of 10 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

talpid-wireguard/src/unix.rs line 37 at r2 (raw file):

    if unsafe { libc::ioctl(sock.as_raw_fd(), libc::SIOCSIFMTU, &ifr) } < 0 {
        let e = std::io::Error::last_os_error();
        log::error!("{}", e.display_chain_with_msg("libc::ioctl failed"));

Nit: Could be more specific, e.g. "SIOCSIFMTU failed".

[dlon]

Reviewed 1 of 10 files at r1.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @Serock3)

---

talpid-wireguard/src/lib.rs line 1070 at r3 (raw file):

        // todo!("Handle manual MTU lower that minimum")
    }
    let in_between = ((x_start + 1)..x_end).filter(|x| x % step_size == 0);

Could probably use step_by here?

---

talpid-wireguard/src/ping_monitor/icmp.rs line 65 at r3 (raw file):

    /// Creates a new `Pinger`.
    ///
    /// `mtu_discover` sets the `IP_MTU_DISCOVER` flag for the socker, implying no fragementation is

I guess this is no longer relevant.

[Serock3]

Reviewable status: 6 of 11 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

talpid-wireguard/src/lib.rs line 1070 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could probably use step_by here?

That will not produce evenly divisible numbers, which I think looks nicer. Since you're starting at 576 you would get 596, 616, 636 and so on.

---

talpid-wireguard/src/unix.rs line 37 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Could be more specific, e.g. "SIOCSIFMTU failed".

Done!

---

talpid-wireguard/src/ping_monitor/icmp.rs line 65 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I guess this is no longer relevant.

Missed that, good catch! Fixed.

[dlon]

Reviewed 2 of 3 files at r6, 1 of 2 files at r7, 2 of 2 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

talpid-wireguard/src/lib.rs line 1070 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

That will not produce evenly divisible numbers, which I think looks nicer. Since you're starting at 576 you would get 596, 616, 636 and so on.

Do they really need to be divisible? A bit wasteful to compute the mod for every single number, IMO. :)

How about

let x_start = step_size + x_start - (x_start % step_size);

, plus step_by?

---

talpid-wireguard/src/ping_monitor/icmp.rs line 77 at r8 (raw file):

            .map_err(Error::SocketOp)?;

        // nix::sys::socket::setsockopt(fd, opt, val) // TODO: deleteme

Don't forget to remove this as well.

[Serock3]

Reviewable status: 9 of 11 files reviewed, all discussions resolved (waiting on @dlon)

---

talpid-wireguard/src/lib.rs line 1070 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Do they really need to be divisible? A bit wasteful to compute the mod for every single number, IMO. :)

How about

let x_start = step_size + x_start - (x_start % step_size);

, plus step_by?

I think they should be divisible, otherwise it looks lite the MTU detection honed in on some very specific number when in reality it checks in 20 bytes steps. Also, I think it is more likely that the true max MTU is a round number.

I think I found a nice solution though, have a look in the diff.

---

talpid-wireguard/src/ping_monitor/icmp.rs line 77 at r8 (raw file):

Previously, dlon (David Lönnhager) wrote…

Don't forget to remove this as well.

Done

[Serock3]

Reviewable status: 9 of 11 files reviewed, all discussions resolved (waiting on @dlon)

[dlon]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

talpid-wireguard/src/lib.rs line 1070 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I think they should be divisible, otherwise it looks lite the MTU detection honed in on some very specific number when in reality it checks in 20 bytes steps. Also, I think it is more likely that the true max MTU is a round number.

I think I found a nice solution though, have a look in the diff.

That is nice!

[dlon]

Reviewed 1 of 1 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Serock3)

---

talpid-wireguard/src/lib.rs line 1041 at r10 (raw file):

        .map_ok(|(packet, _rtt)| {
            let surge_ping::IcmpPacket::V4(packet) = packet else {
                panic!("ICMP ping response was not of IPv4 type");

I think unreachable! would be slightly clearer.

---

talpid-wireguard/src/lib.rs line 1052 at r10 (raw file):

        .next()
        .await
        .expect("At least one pings should be sent")

Probably a bit nitpicky and subjective, but you could use map_err here instead of match.

[dlon]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)

---

talpid-wireguard/src/lib.rs line 1052 at r10 (raw file):

        .next()
        .await
        .expect("At least one pings should be sent")

I should have thought about this earlier, but is it really safe to unwrap here? Will this not panic if the first ping fails for whatever reason?

[dlon]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Serock3)

---

talpid-wireguard/src/lib.rs line 1052 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

I should have thought about this earlier, but is it really safe to unwrap here? Will this not panic if the first ping fails for whatever reason?

Never mind, this looks fine.

[Serock3]

Reviewable status: 10 of 11 files reviewed, all discussions resolved (waiting on @dlon)

---

talpid-wireguard/src/lib.rs line 1041 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think unreachable! would be slightly clearer.

Agree. Done.

---

talpid-wireguard/src/lib.rs line 1052 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

Probably a bit nitpicky and subjective, but you could use map_err here instead of match.

Done

[MarkusPettersson98]

Very nice work! Please see my comments, they should be easily resolvable 😊

Reviewed 1 of 10 files at r1, 1 of 3 files at r5, 1 of 2 files at r7, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Serock3)

---

talpid-tunnel/src/lib.rs line 28 at r7 (raw file):

pub const MIN_IPV4_MTU: u16 = 576;
/// Smallest allowed MTU for IPv6 in bytes
pub const MIN_IPV6_MTU: u16 = 1280;

I feel like this is general+useful enough to define in talpid-types to promote re-use / easier refactoring. A lot more of our crates depend on talpid-types compared to talpid-tunnel 😊

Code quote:

/// Size of IPv4 header in bytes
pub const IPV4_HEADER_SIZE: u16 = 20;
/// Size of IPv6 header in bytes
pub const IPV6_HEADER_SIZE: u16 = 40;
/// Size of wireguard header in bytes
pub const WIREGUARD_HEADER_SIZE: u16 = 40;
/// Size of ICMP header in bytes
pub const ICMP_HEADER_SIZE: u16 = 8;
/// Smallest allowed MTU for IPv4 in bytes
pub const MIN_IPV4_MTU: u16 = 576;
/// Smallest allowed MTU for IPv6 in bytes
pub const MIN_IPV6_MTU: u16 = 1280;

---

talpid-wireguard/src/lib.rs line 418 at r12 (raw file):

                        };
                    } else {
                        log::info!("MTU {verified_mtu} verified to not drop packets");

I think it would be fine to lower the log level here to either debug or trace 😊

Code quote:

log::info!("MTU {verified_mtu} verified to not drop packets");

---

talpid-wireguard/src/lib.rs line 993 at r12 (raw file):

///
/// The detection works by sending evenly spread out range of pings between 576 and the given
/// current tunnel MTU, and returning the maximum packet size that war returned within a timeout.

Typo: was

Code quote:

war

---

talpid-wireguard/src/lib.rs line 1031 at r12 (raw file):

                log::trace!("Sending ICMP ping of total size {mtu}");
                client
                    .pinger(IpAddr::V4(gateway), PingIdentifier(111)) //? Is a static identified ok, or should we randomize?

Could we resolve this question at this point? 😊

Code quote:

.pinger(IpAddr::V4(gateway), PingIdentifier(111)) //? Is a static identified ok, or should we randomize?

---

talpid-wireguard/src/lib.rs line 1062 at r12 (raw file):

    ping_stream
        .timeout(PING_OFFSET_TIMEOUT) // Start a new, sorter, timeout

Typo: shorter*

Code quote:

sorter

[Serock3]

Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-wireguard/src/lib.rs line 418 at r12 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I think it would be fine to lower the log level here to either debug or trace 😊

Changed it to debug

---

talpid-wireguard/src/lib.rs line 1031 at r12 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Could we resolve this question at this point? 😊

Done. Also changed identifier to be zero.

[MarkusPettersson98]

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 2 of 2 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 5 of 5 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

talpid-wireguard/Cargo.toml line 84 at r16 (raw file):

[dev-dependencies]
proptest = "1.4"

Seems like there are some trailing characters here 😊

Code quote:

proptest = "1.4"

[MarkusPettersson98]

Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

[MarkusPettersson98]

Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved