[inetstack] Compute Default MSS from MTU - #1638
Open
SWARAJ SINGH (dedsec-terminal) wants to merge 1 commit into
Open
SWARAJ SINGH (dedsec-terminal) wants to merge 1 commit into
SWARAJ SINGH (dedsec-terminal) wants to merge 1 commit into
Conversation
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR fix?
Closes #221.
Related to #178 (MTU Discovery): this does not implement discovery. It only stops the stack from ignoring the MTU that is already configured.
What does this PR do?
DEFAULT_MSSwas hard-coded to1450, regardless of the MTU the runtime was configured with. It is now derived from the MTU as RFC 793 and RFC 6691 prescribe:The Ethernet header is deliberately excluded (an MTU bounds the IP datagram), and the minimum header sizes are used because that is what the stack actually emits on data segments.
MSS_HEADER_OVERHEADcarries a comment noting that this must grow if the RFC 7323 timestamp option is ever added.Details:
src/inetstack/consts.rs: newMSS_HEADER_OVERHEAD,DEFAULT_MTU, andconst fn default_mss_from_mtu(). The helper useschecked_suband clamps toMIN_MSS..=MAX_MSS, so a degenerate MTU can neither underflow nor produce a value that does not fit the 16-bit MSS option.DEFAULT_MSSis now derived fromDEFAULT_MTU(1500) instead of being an unrelated hard-coded 1450, so the two defaults cannot disagree.src/inetstack/config/tcp.rs:TcpConfig::new()derives the advertised MSS from the configured MTU when nomssis given. An explicitly configuredmssstill wins, but it now returnsFail(ERANGE)instead of panicking on an out-of-range value, and logs a warning when it is larger than the MTU can carry.TcpConfig::get_effective_mss(), used by the active and passive open paths.Three latent MSS bugs fixed along the way
active_open.rsandpassive_open.rsstored the received MSS option straight intoSender::mss, which is what caps segment size incalculate_open_window_bytes(). A peer sitting on a jumbo link could therefore make our sender build segments larger than our own interface can carry. RFC 1122, Section 4.2.2.6 requiresmin(our MSS, offered MSS), which is whatget_effective_mss()now implements (falling back toFALLBACK_MSS= 536 when the peer sends no MSS option, as before).max_body_size = config.mss()? - MAX_HEADER_SIZE, but the MSS is already a payload size, so the headers were subtracted twice (1500 -> 1366 instead of 1460). It also failed outright at startup when nomsswas configured. It now derivesmax_body_sizefrom the MTU with a saturating subtraction, matching what Catpowder (Linux) already does.mss: 1500next tomtu: 1500, i.e. a segment that can never fit in a frame. That line is now a comment showing the option is optional.Behaviour change
With only
mtu: 1500configured and no explicitmss, the advertised MSS becomes 1460 (was 1450). That is the point of the issue, but it is wire-visible, so calling it out.Nothing existing changes behaviour:
src/inetstack/test_helpers/*.yamlandtests/rust/common/*.yamlall pinmss:explicitly, so no current test is affected.tests/rust/common/*.yamlstill carries the same oddmss: 1500/mtu: 1500pair as the templates did - it now only produces a warning. Happy to clean that up in a follow-up if you would rather keep this diff focused.Tests
Three new unit tests in
src/inetstack/config/tcp.rs:test_tcp_config_mss_derived_from_mtu- 1500 -> 1460, 9000 -> 8960, 4000 -> 3960, and clamping to 536 for MTUs of 576 and 20.test_tcp_config_mss_overrides_mtu- explicit MSS precedence, the no-MTU/no-MSS fallback, andERANGEformss: 535andmss: 65536.test_tcp_config_effective_mss- no option -> 536, smaller option honoured, larger option clamped.An end-to-end handshake test for the clamping would need new config fixtures (a peer advertising a larger MSS than our MTU), which I left out to keep this reviewable. Let me know if you want it and I will add it.
Not covered here
The real interface MTU is still not discoverable from the inetstack:
catniponly asserts that DPDK's MTU matches the config value, andsender.rsstill carriesTODO: Revisit this once we support path MTU discovery. That is #178.