fix(net): store the checksum an incremental update computes - #1733
Draft
daniel-noland wants to merge 1 commit into
Draft
fix(net): store the checksum an incremental update computes#1733daniel-noland wants to merge 1 commit into
daniel-noland wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
daniel-noland
force-pushed
the
pr/daniel-noland/fuzz-net-checksum
branch
from
August 18, 2026 20:23
56e8187 to
61049ae
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/fuzz-net-checksum
branch
from
August 18, 2026 20:31
61049ae to
dd6e234
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/fuzz-net-checksum
branch
from
August 18, 2026 20:38
dd6e234 to
1ba8747
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/fuzz-net-checksum
branch
from
August 18, 2026 23:17
1ba8747 to
94cdc87
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/fuzz-net-checksum
branch
2 times, most recently
from
August 20, 2026 01:58
a9d9273 to
3268b44
Compare
`Checksum::increment_update_checksum` works out the new checksum and hands it back. It does not store it, despite taking `&mut self`. `EmbeddedTransport::update_checksum` called it and dropped the answer on the floor, under a comment about ignoring errors on a truncated header -- but there is no error to ignore; the return value is the checksum. So no quoted transport header has ever had its checksum updated. NAT rewrites the ports of the packet quoted inside an ICMP error and calls this to keep the checksum in step, and the call has been doing nothing. The only correct uses of the trait method in the tree are in a test, which is presumably how the shape survived. Setting it can fail, on a header too truncated to hold a checksum -- but that is a header the caller could not have read a checksum out of either, and every caller reads one first. Hence discarding that error rather than the value, which is what the comment was reaching for. update_checksum_for_address is new, and folds a change of one of the quoted packet's addresses in. TCP, UDP and ICMPv6 are checksummed over a pseudo-header built from the source and destination addresses, so rewriting one leaves the quoted checksum describing an address that is no longer there. ICMPv4 has no pseudo-header and is left alone. Incremental for the same reason as the rest: a quote is usually truncated, so there is no payload to compute over from scratch. The tests build the one case where both routes are open -- a full header over a known payload -- and hold the fold against a computation from scratch, over two words for v4 and eight for v6. The checksum starts out correct on purpose: an RFC 1624 update is exact given a correct starting value and says nothing given a wrong one. Both fail against the discarded update; the ICMPv4 one passes either way, which is the point of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland
force-pushed
the
pr/daniel-noland/fuzz-net-checksum
branch
from
August 20, 2026 02:32
3268b44 to
d6c577e
Compare
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.
First of five, stacked on #1724. Extracts the finished fuzzing campaign from
pr/daniel-noland/icmp-flow-lock, which was never opened as a PR and haddrifted well behind
main.This one is a production bug, on its own so it can be taken without the rest.
Checksum::increment_update_checksumworks out the new checksum and hands itback; it does not store it, despite taking
&mut self.EmbeddedTransport::update_checksumcalled it and dropped the answer, under acomment about ignoring errors on a truncated header — but there is no error to
ignore, the return value is the checksum. So no quoted transport header has
ever had its checksum updated.
NAT rewrites the ports of the packet quoted inside an ICMP error and calls this
to keep the checksum in step. Receivers that validate the quoted checksum will
reject the error; that is the path PMTUD depends on. Plenty of stacks do not
validate it, which is likely why nothing has complained.
update_checksum_for_addressis new, and folds a change of one of the quotedpacket's addresses in — TCP, UDP and ICMPv6 checksum over a pseudo-header built
from the addresses, so rewriting one leaves the quoted checksum describing an
address that is no longer there. ICMPv4 has no pseudo-header and is left alone.
Verified locally rather than in CI, given the outage:
dataplane-net473/473,fmt --checkandclippy -D warningsclean. Reverting the fix fails exactlya_v4_address_change_matches_a_fresh_tcp_checksumand its v6/UDP twin.🤖 Generated with Claude Code