datapath-windows: execute set() for IPv6, SCTP, ARP, ND and MPLS by fw2568 · Pull Request #53 · dbosoft/ovs

fw2568 · 2026-06-11T16:28:59Z

The Windows kernel datapath silently ignored OVS_ACTION_ATTR_SET for the IPv6, SCTP, ARP, ND and MPLS key types: the action switch had no case for them, so the packet egressed unmodified while the set still reported NDIS_STATUS_SUCCESS. set(ipv6(...)) in particular — OVN logical-router IPv6 next-hop rewrites and DSCP/flow-label/hop-limit changes — was dropped on the floor.

This adds per-type inline rewrite helpers modelled on the existing IPv4/TCP/UDP path:

IPv6 reuses OvsUpdateAddressAndPortForIpv6 for the 128-bit address and L4/ICMPv6 checksum fixup, then writes traffic class, flow label and hop limit.
SCTP recomputes the CRC32c over the transport payload via a new table-driven Crc32c.{c,h} that is safe at DISPATCH_LEVEL.
ARP / ND / MPLS rewrite their respective fields (ND also re-derives the ICMPv6 checksum).

A failed rewrite now propagates instead of reporting false success.

Validated live on the eryph stack: set(ipv6) bidirectional address rewrite with correct ICMPv6 checksums (end-to-end ping returns only when both directions are correct), and IPv6 end-to-end through production OVN on the rebuilt driver.

The kernel datapath silently no-op'd OVS_ACTION_ATTR_SET for the OVS_KEY_ATTR_IPV6/SCTP/ARP/ND/MPLS key types: the action switch had no case for them, so the packet egressed unmodified while the set still reported NDIS_STATUS_SUCCESS. Any set(ipv6(...)) -- OVN logical-router IPv6 next-hop rewrites, DSCP/flow-label/hop-limit changes -- was dropped on the floor. Add per-type inline rewrite helpers modelled on the existing IPv4/TCP/UDP path. OvsUpdateIPv6Header reuses OvsUpdateAddressAndPortForIpv6 for the 128-bit address and L4/ICMPv6 checksum fixup, then writes traffic class, flow label and hop limit. OvsUpdateSctpPorts recomputes the SCTP CRC32c over the transport payload via a new table-driven Crc32c.{c,h} that is safe at DISPATCH_LEVEL. OvsUpdateArpHeader, OvsUpdateNdHeader and OvsUpdateMplsHeader rewrite their respective fields. A failed rewrite now propagates instead of reporting false success.

Copilot

Pull request overview

This PR fixes a functional gap in the Windows kernel datapath where OVS_ACTION_ATTR_SET silently ignored several key types by adding explicit per-key rewrite implementations (IPv6, SCTP, ARP, ND, MPLS) and wiring them into the set-action switch, including a new CRC32c implementation needed for SCTP checksum updates.

Changes:

Add CRC32c (Castagnoli) helper (Crc32c.{c,h}) and include it in the Windows driver project.
Implement inline set-action rewrite helpers for IPv6 / SCTP / ARP / ND / MPLS and invoke them from OvsExecuteSetAction().
Update public action helper declarations in Actions.h.

Reviewed changes

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

Show a summary per file

File	Description
datapath-windows/ovsext/ovsext.vcxproj	Adds the new CRC32c source/header to the Windows driver build.
datapath-windows/ovsext/Crc32c.h	Declares a DISPATCH_LEVEL-safe CRC32c API for SCTP.
datapath-windows/ovsext/Crc32c.c	Implements table-driven CRC32c used by SCTP port rewrite.
datapath-windows/ovsext/Actions.h	Exposes new per-key set-action rewrite helper prototypes.
datapath-windows/ovsext/Actions.c	Adds IPv6/SCTP/ARP/ND/MPLS set-action execution paths and helpers.

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

Address review of the set() execution paths: - set(mpls) wrote the LSE at l3Offset, but the parser advances l3Offset past the whole label stack, so it corrupted the inner L3 header instead of the label. Write at l3Offset - MPLS_HLEN (the label adjacent to L3, i.e. the topmost for the single-label stacks in scope) and reject a set(mpls) misapplied to a non-MPLS packet. - set(ipv6), set(nd) and set(arp) under-checked the L3 type. layers->isIcmp is set for IPv4 ICMP too, so require isIPv6 before reinterpreting the L3 header as IPv6 and writing the icmp6Key union; guard set(arp) on the ARP ethertype. - Clamp the ICMPv6 checksum span and the SCTP CRC32c span to the captured L4 bytes (the SCTP length derived from the IP header, not the frame), so IPv6 extension headers, a lying payload_len, or L2 frame padding cannot walk past the NBL. - Pull the full packet contiguous for the ICMPv6 set(ipv6) path so the shared checksum recompute stays within the contiguous buffer. - Align Crc32c.c to the Apache license header; clarify the byte-order and isTx comments.

fw2568 · 2026-06-11T17:15:12Z

Addressed Copilot's review plus an independent cross-model (Sonnet) review in commit 39bb16ae2.

Cross-model review caught a bug both the author and Copilot missed:

set(mpls) wrote the LSE at the wrong offset. The parser advances l3Offset past the whole MPLS label stack (OvsExtractLayers, Flow.c:2744), so writing at l3Offset corrupted the inner L3 header on every set(mpls) while leaving the label untouched. Now writes at l3Offset - MPLS_HLEN (the label adjacent to L3 = topmost for the single-label stacks in scope), with a non-MPLS-packet guard.

Copilot findings:

ICMPv6 checksum could read past the contiguous buffer when payload_len counts IPv6 extension headers / exceeds the captured L4 bytes → now clamped to the captured length in both OvsUpdateNdHeader and (via a full-packet pull) the set(ipv6) ICMPv6 path.
OvsUpdateNdHeader guarded only on isIcmp, which is set for IPv4 ICMP too → now also requires isIPv6 (and the same guard added to set(ipv6)/set(arp) for symmetry) to prevent L3/union type confusion.
Crc32c.c license header aligned to Apache 2.0 (Gary S. Brown table attribution kept as a note); byte-order comment clarified.

Also from the cross-model review:

SCTP CRC32c length now derived from the IP header (like userspace dp_packet_l4_size) and clamped to captured bytes, so L2 frame padding isn't folded into the checksum.
Documented the intentional conservative isTx=FALSE choice and the ar_op byte-order no-op cast.

Open/deferred: the ND option-walk still stops (rather than fails) on a malformed option — harmless since the target rewrite + recompute remain valid; and there is no kernel-side unit harness to add datapath tests against (the existing test-dpif-windows.c mocks the kernel). Rebuilds clean under /W4 /WX /kernel.

fw2568 · 2026-06-11T17:40:20Z

MPLS fix now live-validated (before/after on the kernel datapath).

Getting set(mpls) to the datapath is subtle: push_mpls + set_field:label folds into push_mpls(label=…) and push+set+pop optimizes to a bare output, so neither exercises the executor. Only MPLS label-swap emits a standalone action — mpls,mpls_label=100,actions=set_field:200->mpls_label,output traces to Datapath actions: set(mpls(label=200…)).

Injected a crafted MPLS frame (label 100, inner 10.0.0.100 > .101 ICMP) through that flow and captured the egress on the receiver with tcpdump -e mpls:

Before the fix: MPLS (label 100) — label unchanged, and the inner IP header's first word was overwritten (4500001c → 000c8140, the label-200 LSE), i.e. the LSE was written into the inner IP. Corrupted packet.
After the fix: MPLS (label 200, [S], ttl 64) 10.0.0.100 > 10.0.0.101: ICMP echo request, id 4660 — label correctly swapped, inner IP fully intact.

Verified by hot-swapping the fixed driver between the two runs. So set(mpls) was provably broken and is now provably correct on the wire.

Copilot

Pull request overview

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

Second review round: - OvsUpdateAddressAndPortForIpv6 (also the IPv6 NAT path) recomputed the ICMPv6 checksum over ipHdr->payload_len bytes while only pulling the IPv6 fixed header contiguous; payload_len counts extension headers l4Offset skips, so the walk could read past the buffer. Pull the whole packet for the ICMPv6 case and clamp the span to the captured L4 length. - set(nd): only rewrite the target / link-layer options for neighbor solicitation / advertisement with code 0 (mirroring OvsParseIcmpV6); fail on a malformed option list rather than checksum a half-validated packet; derive the ICMPv6 checksum length as payload_len minus extension headers, clamped to the captured bytes. - set(sctp): bail when the IP-derived L4 length is shorter than the SCTP header. - set(ipv6): store ipv6Label in the host-order 20-bit form the flow-key extractor uses, so a re-extract of the rewritten packet matches.

fw2568 · 2026-06-11T22:08:12Z

Addressed the second review round in d0984d598, including a pre-existing issue Copilot's pass surfaced:

Existing code (IPv6 NAT path too): OvsUpdateAddressAndPortForIpv6 recomputed the ICMPv6 checksum over payload_len bytes while only pulling the IPv6 fixed header contiguous — an OOB read for multi-MDL or extension-header packets that also affects ct(nat) for IPv6 ICMP. Now pulls the whole packet for the ICMPv6 case and clamps the span to the captured L4 length (payload_len − ext_hdr_len, bounded by captured bytes).
set(nd): only rewrites target/link-layer options for NS/NA with code==0 (mirroring OvsParseIcmpV6); fails on a malformed option list instead of checksumming a half-validated packet; checksum length is now extension-header-aware and clamped.
set(sctp): bails when the IP-derived L4 length is shorter than the SCTP header.
set(ipv6): stores ipv6Label in the host-order 20-bit form the flow-key extractor uses, so a re-extract of the rewritten packet matches (recirc/hash consistency).

Rebuilt clean /W4 /WX /kernel. Regression-checked live: the set(ipv6) bidirectional VIP test (which drives the hardened ICMPv6 helper on ICMPv6 echo) still passes 4/4, 0% loss.

Copilot

Pull request overview

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

…extractor - set(ipv6) wrote traffic class and the flow label by casting the start of IPv6Hdr to a 32-bit word and masking. IPv6Hdr is a bitfield layout (priority:4/version:4 + flow_lbl[3]) the extractor reads byte-wise, so the word cast carried an alignment/endianness assumption. Write the fields byte-wise as the inverse of the extractor instead, leaving version intact. - set(arp) keyed the opcode unconditionally and always mirrored the addresses. Match the extractor (Flow.c): key the opcode only when it fits in 8 bits, and the src/dst/sha/tha only for request/reply, zeroing them otherwise, so a re-extract of the rewritten packet matches the key.

fw2568 · 2026-06-11T22:47:09Z

Addressed the third review round in 58f3ba4c8, and live-validated the IPv6 field rewrite this time.

set(ipv6) TC/flow-label: replaced the 32-bit word cast over IPv6Hdr with a byte-wise write that is the exact inverse of the extractor (priority + flow_lbl[0..2]), leaving version untouched — no alignment/endianness/bitfield assumption.
set(arp): the key now mirrors the extractor exactly — opcode keyed only when it fits in 8 bits, and src/dst/sha/tha only for request/reply (zeroed otherwise).

Live test of the byte-wise write (isolated OVS, set_field on ipv6_label/ip_dscp/nw_ttl, captured on the receiver):

IP6 (class 0x84, flowlabel 0xabcde, hlim 9, ...) ICMP6, echo request  [icmp6 sum ok]
0x0000: 684a bcde 0040 3a09 ...

Raw bytes confirm every field: byte0 0x68 = version 6 + TC-high-nibble 0x8; flow_lbl[0]=0x4a gives TC-low-nibble 0x4 (→ class 0x84) and label-high-nibble 0xa; flow_lbl[1..2]=bcde → flowlabel 0xabcde; byte7 0x09 → hlim 9; ICMPv6 checksum still valid and the peer replies. set(ipv6) is now validated for both address rewrite (earlier) and TC/flow-label/hop-limit (here).

fw2568 · 2026-06-12T06:37:59Z

All five set() types are now functionally validated on the wire (live datapath, capture on the receiving guest):

set(ipv6) — bidirectional address rewrite with valid ICMPv6 checksum (VIP ping), and a TC/flow-label/hop-limit rewrite verified byte-for-byte (class 0x84, flowlabel 0xabcde, hlim 9, sum ok).
set(mpls) — label-swap before/after: buggy build wrote the LSE into the inner IP; fixed build swaps the label (100→200) leaving the inner IP intact.
set(arp) — captured ARP payload shows sha=02:11:22:33:44:55 and spa=10.0.0.250 both rewritten, target fields untouched.
set(nd) — captured NS shows the target rewritten to fd00::dead with the ICMPv6 checksum correctly recomputed ([icmp6 sum ok]).
set(sctp) — injected an SCTP frame with a valid CRC32c, rewrote the dest port 2000→3000; the captured checksum (944ae016) matches an independent CRC32c computation over the rewritten packet exactly (0x16e04a94, little-endian), proving the CRC recompute and the IP-derived length (so L2 padding isn't folded in).

The set-field datapath emission is non-obvious (push+set folds into push_mpls(label), push+set+pop optimizes away), so each test was confirmed via ofproto/trace to actually emit the set(...) action before capturing.

Copilot

Pull request overview

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

- set(mpls) wrote l3Offset - MPLS_HLEN, which is the innermost (BOS) label on a multi-label stack, while the flow key carries the topmost (outermost) label. Locate the topmost label at the end of L2 (derived the same way the parser does, accounting for an in-packet 802.1Q tag) so single- and multi-label stacks both rewrite the correct label. - The IPv6/SCTP L4-length math derived a span by unsigned subtraction of IP length fields; a malformed header could underflow and rely on wraparound + MIN to stay in bounds. Make the subtractions saturating and fail set(sctp) outright on an inconsistent IP length, in OvsUpdateSctpPorts, OvsUpdateNdHeader and OvsUpdateAddressAndPortForIpv6.

fw2568 · 2026-06-12T07:15:42Z

Round 4 addressed in 8dc62c94d, with the MPLS fix validated on a multi-label stack.

set(mpls) multi-label: this was a real bug I'd documented rather than fixed — l3Offset - MPLS_HLEN is the innermost (BOS) label, but the flow key carries the topmost. Now locates the topmost label at the end of L2 (derived the same way OvsExtractLayers does, including an in-packet 802.1Q tag). Live test with a 2-label frame (outer 100, inner 50):
```
MPLS (label 200, tc 0, ttl 64)       <- topmost swapped 100->200
    (label 50, tc 0, [S], ttl 64)    <- inner label unchanged
    IP 10.0.0.100 > 10.0.0.101: ICMP echo request   <- inner IP intact
```
Single-label is unchanged (same byte). The old code would have rewritten the inner label here.
Unsigned underflow in the L4-length math (OvsUpdateSctpPorts, OvsUpdateNdHeader, OvsUpdateAddressAndPortForIpv6): the IP-length-derived spans now use saturating subtraction, and set(sctp) fails outright on an inconsistent IP length, instead of relying on wraparound + MIN() to stay in bounds.

Copilot

Pull request overview

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

- The IPv6/IPv4 address-rewrite helpers computed the contiguous span as `l4Offset + isTcp ? sizeof(tcp) : sizeof(udp)`, which parses as `(l4Offset + isTcp) ? ...` and collapses hdrSize to a small constant, so OvsGetHeaderBySize pulls too few bytes (a latent out-of-bounds read on multi-MDL packets). Parenthesize the ternary in OvsUpdateAddressAndPort, OvsUpdateAddressAndPortForIpv6 and OvsUpdateIPv4Header (OvsUpdateIPv6Header was already correct). - set(nd) gated on layers->isIcmp, which OvsExtractLayers sets only when OvsParseIcmpV6 fully succeeds (and which is set for IPv4 ICMP too). Gate on IPv6 + the ICMPv6 L4 proto from the flow key instead; the on-wire ICMPv6 type/code are already validated before the rewrite.

fw2568 · 2026-06-12T07:56:18Z

Round 5 addressed in b027bffea.

hdrSize precedence bug — l4Offset + isTcp ? sizeof(tcp) : sizeof(udp) parses as (l4Offset + isTcp) ? ..., collapsing hdrSize to a small constant and pulling too few bytes contiguous (latent OOB on multi-MDL). It was present in three pre-existing helpers — OvsUpdateAddressAndPort, OvsUpdateAddressAndPortForIpv6, OvsUpdateIPv4Header — all now parenthesized (OvsUpdateIPv6Header was already correct). No behavioral change on single-MDL packets (the read was already within the contiguous buffer); the fix prevents the OOB when the headers span MDLs.
set(nd) guard — now gates on IPv6 + the ICMPv6 L4 proto from the flow key rather than layers->isIcmp (which is only set when OvsParseIcmpV6 fully succeeds, and is also set for IPv4 ICMP); the on-wire ICMPv6 type/code are still validated before the rewrite.

Regression-tested live on the rebuilt driver: set(nd) still rewrites the NS target with a valid ICMPv6 checksum, and set(ipv6) bidirectional VIP is 4/4. (The precedence fix's multi-MDL benefit isn't separately exercisable here — packet-out and guest traffic produce single-MDL frames — but it's a no-op for single-MDL and correct by inspection.)

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

OvsParseIcmpV6 treats a second source- or target-link-layer-address option as malformed and fails. OvsUpdateNdHeader walked the options without that check, so it would rewrite every occurrence and leave the key reflecting only the last. Track whether each option type has been seen and fail on a duplicate, to match the parser and avoid rewriting a malformed payload.

fw2568 · 2026-06-12T10:59:11Z

Round 6 addressed in 3ca22acd2: set(nd) now tracks whether a source/target link-layer-address option has been seen and fails on a duplicate, matching OvsParseIcmpV6 (which treats duplicate SLL/TLL as malformed).

Validated live by injecting two crafted NS frames through the rewrite:

single SLL → forwarded, target rewritten, SLL preserved, [icmp6 sum ok].
duplicate SLL → only the first option is touched and the second is left unchanged (d2:ab:4f:cd:59:10), i.e. the walk stopped at the duplicate and returned failure. The old code would have rewritten both. (For normal datapath traffic that failure becomes a drop via goto dropit; ovs-ofctl packet-out doesn't honor a failed set the same way, so the frame still egresses there, but the duplicate detection is what's being verified.)

Copilot

Pull request overview

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

fw2568 requested a review from Copilot June 11, 2026 16:30

Copilot started reviewing on behalf of fw2568 June 11, 2026 16:30 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread datapath-windows/ovsext/Crc32c.c Outdated

Comment thread datapath-windows/ovsext/Actions.c Outdated

Comment thread datapath-windows/ovsext/Actions.c Outdated

Comment thread datapath-windows/ovsext/Actions.c

Comment thread datapath-windows/ovsext/Actions.c Outdated

fw2568 requested a review from Copilot June 11, 2026 21:42

Copilot started reviewing on behalf of fw2568 June 11, 2026 21:43 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread datapath-windows/ovsext/Actions.c

Comment thread datapath-windows/ovsext/Actions.c

Comment thread datapath-windows/ovsext/Actions.c

Comment thread datapath-windows/ovsext/Actions.c Outdated

fw2568 requested a review from Copilot June 11, 2026 22:26

Copilot started reviewing on behalf of fw2568 June 11, 2026 22:27 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread datapath-windows/ovsext/Actions.c Outdated

Comment thread datapath-windows/ovsext/Actions.c Outdated

fw2568 requested a review from Copilot June 12, 2026 06:52

Copilot started reviewing on behalf of fw2568 June 12, 2026 06:52 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread datapath-windows/ovsext/Actions.c

Comment thread datapath-windows/ovsext/Actions.c Outdated

Comment thread datapath-windows/ovsext/Actions.c

Comment thread datapath-windows/ovsext/Actions.c Outdated

fw2568 requested a review from Copilot June 12, 2026 07:23

Copilot started reviewing on behalf of fw2568 June 12, 2026 07:23 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread datapath-windows/ovsext/Actions.c Outdated

Comment thread datapath-windows/ovsext/Actions.c Outdated

fw2568 requested a review from Copilot June 12, 2026 10:06

Copilot started reviewing on behalf of fw2568 June 12, 2026 10:07 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread datapath-windows/ovsext/Actions.c

fw2568 requested a review from Copilot June 12, 2026 19:57

Copilot started reviewing on behalf of fw2568 June 12, 2026 19:58 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

fw2568 merged commit 4c0a86e into dbosoft-main Jun 12, 2026
2 checks passed

Conversation

fw2568 commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fw2568 commented Jun 11, 2026

Uh oh!

fw2568 commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fw2568 commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

fw2568 commented Jun 11, 2026

Uh oh!

fw2568 commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fw2568 commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

fw2568 commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

fw2568 commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants