perf: rewrite the Recalculate*Checksum inner loop over a wide one's-complement sum

[wiresock]

What

All four CNdisApi::Recalculate*Checksum functions (IP, ICMP, TCP, UDP) shared one inner loop: two bytes per iteration, byte loads, shift+mask, accumulating big-endian 16-bit words. This replaces it with inet_checksum::sum16_be — a wide one's-complement sum accumulating 32-bit loads into four independent uint64_t accumulators.

Measured on an out-of-order x64 core (QueryThreadCycleTime, 1330 B TCP segments): 0.21–0.26 cycles/byte versus the old loop's 1.09–1.67 — a 5.1–6.4× speedup. On a VPN data path that recomputes L4 checksums on every packet in both directions, the old loop was the single largest per-byte cost outside the cipher.

Why it is safe

The result is bit-identical, not approximately equal:

• The Internet checksum is byte-order independent up to one byte swap of the folded result (RFC 1071 §2(B)), so the hot loop can accumulate native little-endian words and swap only the final 16 bits.
• The 0x0000-versus-0xFFFF folding boundary cannot diverge: a folded sum is zero only for an all-zero span in both formulations, and every nonzero case is forced by congruence mod 0xFFFF.
• A uint64_t absorbs 2^32 32-bit additions, so the loop needs no carry handling and no intrinsics — it compiles unchanged for x86, x64 and ARM64 (all three verified).

Proven by an exhaustive differential test against the old loop — 8 alignments × 3001 lengths plus boundary fills (0x00/0xFF, lengths 0/1/2/3 and up to 65515) — run under AddressSanitizer with allocations sized so any read past the span faults. A pinned regression suite lives in the consuming client repo (kp_pkf_client_ut/inet_checksum_test.cpp), testing against a frozen verbatim copy of the old loop.

Two behavioural fixes that ride along

1. The odd-length pad-byte write is gone. The old loop wrote a zero byte into the packet buffer one past the payload, then read it back. sum16_be uses a masked tail load and never writes anywhere. Nothing depended on the write — but note for anyone byte-diffing captures across builds: one receiver-ignored Ethernet-trailer byte can now differ on odd-L4 frames.

2. RecalculateICMPChecksum gains the length validation its TCP and UDP siblings received in 20aa90f. It computed ntohs(ip_len) - ip_hl*4 unchecked, so a forged or offload-mangled ip_len underflowed the length and drove the checksum loop — and, worse, the pad-byte write — up to ~64 KB past m_IBuffer. The guard is the exact shape of the TCP/UDP one. The only sensitive in-tree caller pattern (synthesizing ICMP unreachable from an over-MTU frame) passes the guard: the synthesized ip_len is ≤ 96 while m_Length still holds the larger original at call time.

Testing

• Exhaustive differential vs the old implementation under ASan (above)
• Builds clean: x64, Win32, ARM64
• Consuming client (kp_pkf_client): 28/28 unit tests, including the new pinned checksum suite

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[Copilot]

Pull request overview

This PR refactors CNdisApi::Recalculate*Checksum (IP/ICMP/TCP/UDP) to use a new wide one’s-complement accumulation routine (inet_checksum::sum16_be) to significantly speed up checksum recomputation, while also removing the prior odd-length “pad-byte write” behavior and adding missing ICMP length validation.

Changes:

• Replace the byte-pair checksum inner loops with inet_checksum::sum16_be in IP/ICMP/TCP/UDP recalculation.
• Remove the odd-length pad-byte write into the packet buffer by switching to a masked-tail load approach.
• Add IP/ICMP length validation in RecalculateICMPChecksum consistent with existing TCP/UDP guards.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
ndisapi/ndisapi.cpp	Switches checksum accumulation to inet_checksum::sum16_be and adds ICMP length validation while removing pad-byte writes.
ndisapi/inet_checksum.h	Introduces a new wide Internet-checksum accumulation helper used by the recalculation routines.

Comments suppressed due to low confidence (2)

ndisapi/inet_checksum.h:68

• If you add the INET_CHECKSUM_MEMCPY compatibility macro (so older standard libraries that don't expose std::memcpy still compile), the 16-byte loop should use it consistently.

            std::memcpy(&w0, data + i, 4);
            std::memcpy(&w1, data + i + 4, 4);
            std::memcpy(&w2, data + i + 8, 4);
            std::memcpy(&w3, data + i + 12, 4);

ndisapi/inet_checksum.h:78

• Same compatibility concern for std::memcpy in the 4-byte tail loop: use the INET_CHECKSUM_MEMCPY macro consistently so the header compiles on older MSVC/STL combinations.

            std::memcpy(&w, data + i, 4);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

ndisapi/inet_checksum.h:43

• In the non-legacy branch, inet_checksum.h uses size_t in the sum16_be signature/body but doesn’t include a header that guarantees size_t is declared. Depending on the standard library/compile mode, relying on <cstring> for size_t can fail. Add an explicit <stddef.h> (or <cstddef>) include in the modern branch to make this header self-sufficient.

#include <cstdint>
#include <cstring>
#define INET_CHECKSUM_MEMCPY ::std::memcpy

[wiresock]

For the record, the three comments Copilot suppressed as low-confidence were reviewed as well:

• Both round-1 suppressed comments (use INET_CHECKSUM_MEMCPY consistently in the 16-byte block and the 4-byte tail loop) reviewed the pre-compat revision and were already addressed by 447f7dd — every memcpy site in the header goes through the macro.
• The round-2 suppressed comment (modern branch relies on <cstring> for global size_t, which the standard leaves unspecified — only std::size_t is guaranteed) is technically correct and fixed in fbe17d2: <stddef.h> now sits above the branch split, guaranteeing ::size_t for both. Latent rather than live — every MSVC/libstdc++/libc++ injects it in practice — but the header claims self-sufficiency, so it should hold without relying on that.

Both compat branches re-verified after the change: the exhaustive ASan differential (8 alignments × 3001 lengths + boundary fills) passes bit-identically through the modern and forced-legacy paths, and the consuming client's pinned regression suite stays green.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.