Integrate Mozilla's mtu crate to constrain MTU discovery#2465
Integrate Mozilla's mtu crate to constrain MTU discovery#2465ali-molajani wants to merge 3 commits into
Conversation
This change layers interface MTU detection on top of user configuration rather than replacing it. When creating client connections, we query the interface MTU for the destination IP using Mozilla's mtu crate and apply it as an additional constraint on the MTU discovery upper bound. Key changes: - Add mtu crate dependency to quinn - Query interface MTU when creating client connections in quinn layer - Pass interface MTU constraint through to quinn-proto layer - Apply constraint in MTU discovery SearchState, respecting user config and peer limits (effective_upper_bound = min(user_bound, interface_mtu, peer_limit)) - Add tests for interface MTU constraint behavior The constraint is applied as an additional limit, so user configuration takes precedence if it's more restrictive, maintaining backward compatibility.
ali-molajani
requested review from
Ralith,
djc and
gretchenfrage
as code owners
Ralith
left a comment
Ralith
left a comment
There was a problem hiding this comment.
This is very clean, thanks! I bet we can do better for migrations/incoming connections with some effort, but no reason to block this straightforward improvement on those subtler problems.
|
See some previous discussion at #1978 also. One other limitation in the current implementation is incorrect behavior when the client involuntarily migrates (e.g. a phone moving between wifi and cellular), though the worst case there is that the MTU remains clamped lower than it should be (if it's too large, that's just the status quo again), so probably not a blocker. We might want to explore monitoring the source address of outgoing packets for changes in follow-up work. |
|
//CC @larseggert as the author of the mtu crate. |
| // IPv6: 40 bytes IP header + 8 bytes UDP header = 48 bytes | ||
| let header_overhead = if addr.is_ipv6() { 48 } else { 28 }; | ||
| let udp_payload_size = interface_mtu.saturating_sub(header_overhead); | ||
| // Convert usize to u16, clamping to u16::MAX if too large |
There was a problem hiding this comment.
Seems like you are returning None instead of clamping here, no?
| if pref_addr_cid.is_some() { 2 } else { 1 }, | ||
| ), | ||
| path: PathData::new(remote, allow_mtud, None, 0, now, &config), | ||
| path: PathData::new(remote, allow_mtud, None, 0, now, &config, interface_mtu_constraint), |
There was a problem hiding this comment.
How about we drop the interface_ prefix in names? It seems pretty verbose and it's not like we have a bunch of other MTU constraints right now (can expand more on it in documentation).
| checksum = "7dfdb4953a096c551ce9ace855a604d702e6e62d77fac690575ae347571717f5" | ||
|
|
||
| [[package]] | ||
| name = "bindgen" |
There was a problem hiding this comment.
This seems ugly. Why does mtu need bindgen? Can it be updated to something newer?
There was a problem hiding this comment.
Oof, yeah, I missed that. I would rather not have a bindgen build dependency.
There was a problem hiding this comment.
We need bindgen, because libc and similar don't have the necessary bindings for talking to routing sockets.
https://github.com/mozilla/neqo/blob/042e684f33df3127f485deeb3e122a8963168158/mtu/build.rs#L51-L65
There was a problem hiding this comment.
Could we commit the bindgen output, or vendor handwritten bindings for the necessary interfaces?
There was a problem hiding this comment.
I think we originally tried that. It's a pain, because of course all the BSDs are slightly different.
There was a problem hiding this comment.
could you use one of the https://docs.rs/rtnetlink crates?
There was a problem hiding this comment.
Fyi we are currently using https://docs.rs/netdev/ for these things, which is quite nice and includes support for parsing MTUs from the network interfaces
There was a problem hiding this comment.
Isn't for the first one Linux-only?
For the second, I think I looked at it in the past, and it didn't do MTU. If it does, maybe we can switch to it for neqo, too.
There was a problem hiding this comment.
Ah, I think the issue with netdev was that it doesn't do routing table lookups.
There was a problem hiding this comment.
Ah, I think the issue with netdev was that it doesn't do routing table lookups.
I think it would be great to add that to netdev, the developer has been super responsive to any requests and improvements and it is becoming a really great crate to collect these kinds of informations across OSes
| futures-io = { workspace = true, optional = true } | ||
| rustc-hash = { workspace = true } | ||
| pin-project-lite = { workspace = true } | ||
| mtu = "0.2" |
There was a problem hiding this comment.
I just published mtu v0.3.0: https://crates.io/crates/mtu/0.3.0
| mtu = "0.2" | |
| mtu = "0.3" |
6 participants
This change layers interface MTU detection on top of user configuration rather than replacing it. When creating client connections, we query the interface MTU for the destination IP using Mozilla's mtu crate and apply it as an additional constraint on the MTU discovery upper bound.
Key changes:
The constraint is applied as an additional limit, so user configuration takes precedence if it's more restrictive, maintaining backward compatibility.