incremental checksums: net helpers by daniel-noland · Pull Request #1849 · githedgehog/dataplane

daniel-noland · 2026-09-23T02:30:07Z

No description provided.

coderabbitai · 2026-09-23T02:30:14Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Essentials

Run ID: bbffe4c5-3ad1-4145-bb0b-ff7a5f5dffb0

📥 Commits

Reviewing files that changed from the base of the PR and between 7d57087 and 43db9d6.

📒 Files selected for processing (21)

.duvet/config.toml
.duvet/requirements/www.rfc-editor.org/rfc/rfc6935/section-1.toml
.duvet/requirements/www.rfc-editor.org/rfc/rfc6935/section-5.toml
.duvet/snapshot.txt
.duvet/specifications/www.rfc-editor.org/rfc/rfc6935.txt
.duvet/specifications/www.rfc-editor.org/rfc/rfc8200.txt
dataplane/src/packet_processor/ipforward.rs
nat/src/masquerade/icmp_handling.rs
nat/src/masquerade/nf.rs
nat/src/masquerade/packet.rs
nat/src/masquerade/test.rs
nat/src/portfw/icmp_handling.rs
nat/src/portfw/packet.rs
nat/src/portfw/test.rs
nat/src/static_nat/fuzz.rs
nat/src/static_nat/nf.rs
nat/src/static_nat/test.rs
nat/src/test.rs
net/src/headers/mod.rs
net/src/packet/test_utils.rs
net/src/udp/checksum.rs

🚧 Files skipped from review as they are similar to previous changes (1)

net/src/udp/checksum.rs

Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.

📝 Walkthrough

Walkthrough

The changes add incremental transport checksum updates for address and 16-bit field changes. NAT paths use these updates and request full checksum refresh when required. The changes also define computed-zero UDP encoding, add IPv6 UDP zero-checksum detection, and allow IPv6 packets to proceed through VXLAN encapsulation.

Changes

Transport checksum updates

Layer / File(s)	Summary
Checksum rules and update APIs `net/src/udp/checksum.rs`, `net/src/headers/embedded.rs`, `net/src/headers/mod.rs`, `.duvet/config.toml`, `.duvet/requirements/...`, `.duvet/snapshot.txt`, `.duvet/specifications/...`	`UdpChecksum::from_computed` maps computed zero to `0xFFFF`. `Net` and `Transport` add incremental checksum updates, and `Headers` detects IPv6 UDP headers with a zero checksum. Duvet files add RFC 6935 and RFC 8200 material.
NAT checksum updates `nat/src/masquerade/packet.rs`, `nat/src/masquerade/nf.rs`, `nat/src/portfw/packet.rs`, `nat/src/static_nat/nf.rs`	Masquerade, port forwarding, and static NAT update address and transport checksums incrementally. They request a full refresh when incremental updates cannot be applied, including for zero-checksum IPv6 UDP packets.
Quoted packet and outer ICMP checksums `nat/src/masquerade/icmp_handling.rs`, `nat/src/portfw/icmp_handling.rs`, `nat/src/masquerade/test.rs`, `nat/src/portfw/test.rs`, `nat/src/test.rs`	ICMP error handlers request checksum refresh after translating quoted packets. Tests check outer ICMP checksums and port-forwarding translation of a quoted destination.
Checksum validation and edge cases `net/src/headers/mod.rs`, `net/src/packet/test_utils.rs`, `nat/src/masquerade/packet.rs`, `nat/src/masquerade/test.rs`, `nat/src/portfw/packet.rs`, `nat/src/static_nat/fuzz.rs`, `nat/src/static_nat/test.rs`	Tests compare incremental checksums with full recomputation and cover UDP zero encoding, ICMPv4 fallback, disabled checksums, and rejected address-family changes. Packet test utilities add ICMP builders and a checksum assertion helper.
IPv6 VXLAN encapsulation `dataplane/src/packet_processor/ipforward.rs`	`vxlan_encap` no longer reaches an unreachable branch for IPv6 packets when checksum refresh is not requested. A test checks that encapsulation proceeds and the packet is not marked done.

Suggested reviewers: qmonnet

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 43db9

The checksum and encapsulation changes have no established merge-blocking issue in the supplied evidence; proceed with normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 90 functions across 16 files. (6 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No pull request description was provided, so the relationship between the changes and the stated objectives cannot be assessed from the description.	Add a concise description of the incremental checksum helpers, checksum-aware NAT updates, IPv6 UDP zero-checksum handling, and related tests.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: adding network helpers for incremental checksum updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 71.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 90 functions across 16 files. (6 skipped: 6 unsupported.)

Fix all pre-merge checks with AI

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-09-23T02:44:36Z

Codecov Report

❌ Patch coverage is 89.28939% with 104 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
net/src/headers/mod.rs	89.55%	32 Missing and 13 partials ⚠️
nat/src/masquerade/packet.rs	82.46%	19 Missing and 8 partials ⚠️
nat/src/portfw/packet.rs	90.38%	7 Missing and 3 partials ⚠️
nat/src/static_nat/nf.rs	84.31%	3 Missing and 5 partials ⚠️
nat/src/masquerade/test.rs	83.87%	3 Missing and 2 partials ⚠️
nat/src/static_nat/fuzz.rs	86.36%	3 Missing ⚠️
nat/src/static_nat/test.rs	96.62%	3 Missing ⚠️
net/src/packet/test_utils.rs	88.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

RFC 1624 computes 0x0000 when the new one's-complement sum is zero. A full recompute agrees unless every covered word is zero: all-zero data sums to +0, whose checksum is 0xFFFF, and a stored 0x0000 fails validation. An echo reply with identifier 1, sequence 0 and no payload reaches it when the identifier is translated to 0. Only ICMPv4 can get there. Every other transport has a pseudo-header with a non-zero protocol number and length in it, and UDP already writes a computed 0 as 0xFFFF. Which of the two cases applies depends on the payload, which `Transport` cannot see, so the update refuses and leaves it to a full recompute. `increment_checksum_for_u16` now returns whether it updated the checksum, as `increment_checksum_for_address` already does. Reported in review of #1849. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

`increment_checksum_for_address` returned early for ICMPv4, which has no pseudo-header, before checking the address families. An ICMPv4 transport given a v4-to-v6 change therefore reported the update as done, contradicting the documented contract that a version change is refused and left to a full recompute. Check the families first. Reported in review of #1849. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

RFC 1624 computes 0x0000 when the new one's-complement sum is zero. A full recompute agrees unless every covered word is zero: all-zero data sums to +0, whose checksum is 0xFFFF, and a stored 0x0000 fails validation. An echo reply with identifier 1, sequence 0 and no payload reaches it when the identifier is translated to 0. Only ICMPv4 can get there. Every other transport has a pseudo-header with a non-zero protocol number and length in it, and UDP already writes a computed 0 as 0xFFFF. Which of the two cases applies depends on the payload, which `Transport` cannot see, so the update refuses and leaves it to a full recompute. `increment_checksum_for_u16` now returns whether it updated the checksum, as `increment_checksum_for_address` already does. Reported in review of #1849. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

`increment_checksum_for_address` returned early for ICMPv4, which has no pseudo-header, before checking the address families. An ICMPv4 transport given a v4-to-v6 change therefore reported the update as done, contradicting the documented contract that a version change is refused and left to a full recompute. Check the families first. Reported in review of #1849. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Two moderate logic issues remain in checksum handling and test packet construction.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

Preserve zero UDP checksums only for IPv4 or supported tunnels · New
Reject cross-family ICMP protocols in packet builders · New

What changed in this PR

Adds incremental checksum helpers for transport fields and IP addresses, with UDP zero-checksum normalization and expanded ICMP fixtures/tests.

Changes:

Adds UDP checksum conversion.
Adds incremental checksum updates for transport fields and IP addresses.
Extends embedded checksum handling and test coverage.

File	Summary
`net/src/udp/checksum.rs`	Adds UDP checksum wire-value conversion.
`net/src/packet/test_utils.rs`	Adds ICMP fixtures; cross-family protocol combinations need handling.
`net/src/headers/mod.rs`	Adds incremental helpers and tests; IPv6 UDP zero checksums need correct handling.
`net/src/headers/embedded.rs`	Reuses UDP conversion and exposes address-word helpers.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Move UDP zero normalization to `UdpChecksum::from_computed` for reuse by `Transport`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Add RFC 1624 helpers for port, address, and ICMP identifier changes. Preserve disabled IPv4 UDP checksums and store computed zeros as 0xFFFF. `Headers::has_zero_ipv6_udp_checksum` flags the IPv6 zero, which only a full recompute can repair. Request full recomputation for ambiguous ICMPv4 zero results. The address helper is generic over `IpAddress`, so an IP version change cannot be expressed and the update cannot fail. It folds the whole address in one step. `Net::try_set_source_updating_checksum` and `try_set_destination_updating_checksum` set an address and fold it into the transport checksum under the version match `Net` already does. Compare updates with full recomputation in Bolero properties. A further property covers what a checksum-correct packet never reaches: unchanged values, a missing transport, and disabled UDP checksums. Test the zero-checksum and version-mismatch boundaries. NAT integration follows in #1809. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Address the four unresolved moderate checksum-handling issues before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Avoid applying IPv4 deltas to ICMPv6 checksums · New

Resolved since last review (2)

Reject cross-family ICMP protocols in packet builders
Preserve zero UDP checksums only for IPv4 or supported tunnels

+                if let Some(transport) = transport {
+                    transport.increment_checksum_for_address(old, addr);
+                }


mvachhar

@daniel-noland I'll leave it to you to decide if we should have duvet enforcement for these two changes.

mvachhar · 2026-09-24T02:11:33Z

+
+    /// Convert a computed checksum to its wire value.
+    ///
+    /// Send a computed zero as `0xFFFF`, since a stored zero disables the checksum (RFC 768).


Do we want a duvet assertion here?

mvachhar · 2026-09-24T02:12:25Z

+    ///
+    /// Returns [`NetError::InvalidIpVersion`], changing nothing, if the IP version of `addr` does
+    /// not match the IP version of the network header.
+    pub fn try_set_source_updating_checksum(


Duvet for the checksum RFC?

Vendor RFC 6935 and RFC 8200. Cite RFC 6935 section 5, which requires a computed UDP checksum of zero to be sent as 0xFFFF, at `UdpChecksum::from_computed` and at the test that drives an update to zero. RFC 8200 section 8.1 carries the same rule in lowercase, so duvet extracts no requirements from it. It is vendored so the report shows it was considered, and so later citations of it need no new specification. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Build ICMPv4 and ICMPv6 echo requests so NAT tests can exercise identifier and checksum updates. Each builder rejects the other family's ICMP. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Skip header checksum updates for IPv6, which has no header checksum. Add a regression test for VXLAN encapsulation without a refresh request. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Use `Transport` for ICMPv4 and ICMPv6 identifier updates so checksum helpers can dispatch by version. Identifier errors now use `TransportError::UnsupportedIdentifier`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Apply address, port, and ICMP identifier deltas with the new `Transport` helpers, requesting full recomputation when needed, including for an IPv6 UDP checksum of zero, which cannot take a delta. Compare SNAT and DNAT results with full recomputation for TCP and ICMP. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Let `snat` and `dnat` decide when a refresh is needed. Check through the network function that translations produce correct checksums without requesting full recomputation. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Apply address and port checksum deltas in both translation directions. Keep ICMPv4 checksums unchanged for address-only updates and adjust ICMPv6 pseudo-header checksums. Request full recomputation for an IPv6 UDP checksum of zero, which cannot take a delta. Compare translated checksums with full recomputation in tests. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Fold address changes as they are set, borrowing the network and transport headers together through `Headers::net_and_transport_mut`. Capture port deltas before translation and apply them after the `TcpUdpMut` borrow ends. Keep full recomputation for ICMP errors, whose checksums cover the quote, and for an IPv6 UDP checksum of zero, which cannot take a delta. Check checksum validity in the marking properties and add a test through `StaticNat::process`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Only set `checksum_refresh` when recomputation is needed; leave existing requests intact when an update succeeds incrementally. `Packet::update_checksums` clears the flag after recomputation. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Request full recomputation in masquerade and port-forwarding ICMP error handlers because the outer checksum covers the modified quote. A truncated TCP checksum or disabled UDP checksum can prevent inner checksum updates from preserving the outer sum. Add a shared assertion for current checksums or pending refresh requests and apply it to ICMP error tests, including a port-forwarding test through the ICMP error handler. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

daniel-noland mentioned this pull request Sep 23, 2026

incremental checksums: NAT #1809

Merged