internal/socket: reply from the address a datagram arrived on - #24
Conversation
A wildcard-bound socket has no fixed source address: the kernel picks one per reply by route. On a multi-homed host, or one whose peer is reached over a bridge or a container network, that is not always the address the peer sent to. A client on the same host as a node running with the host network, for instance, reaches the node's LAN address over the CNI bridge, and the reply leaves from the bridge's address. The QUIC client then receives packets from an address it never dialed, starts path validation, and fails with "unexpected PATH_RESPONSE frame"; the connection never comes up. When the socket is bound to the unspecified address, the IP transport now enables IP_PKTINFO / IPV6_RECVPKTINFO, records the local address each remote's datagrams arrive at, and sends to that remote with a packet-info control message naming it as the source. This is what quinn-udp does for the Rust iroh (RecvMeta::dst_ip, Transmit::src_ip). On Linux the GSO send path carries the message next to UDP_SEGMENT. A socket bound to a specific address is unchanged. A platform without packet info (Windows through x/net) keeps the old reads and writes. If the kernel refuses a recorded source, the send falls back to the kernel's choice and the entry is dropped until the peer reaches the socket again. The table is bounded and cleared when full: a peer not in it is answered as before. Tests send from 127.0.0.1 to a LAN address of the host and check the reply comes from that LAN address, plain and, on Linux, segmented.
|
Thanks for your contribution, reviewing now. |
|
This looks good but a few issues:
Retracted — I was wrong about this, sorry. I'd inferred the gap from the
|
…ction Build the packet-info message once, when an arrival address is recorded, and hand senders the shared slice; before, every send to a known peer allocated three times. Parse received control messages with x/sys/unix, which walks them in place, instead of x/net, which allocated per datagram. The GSO path merges into a stack buffer. A full table now drops the remote heard from longest ago instead of being cleared.
|
latest commit should fix 1 and 3 |
There was a problem hiding this comment.
🟡 Changes recommended
Critical BSD/Darwin support issues and moderate interface-index, cache-race, test-capability, and GSO fallback issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates wildcard-bound UDP sockets to reply from the address on which each datagram arrived, improving QUIC behavior on multihomed and containerized hosts.
Changes:
- Tracks arrival addresses with packet-info control messages and bounded LRU storage.
- Applies recorded source addresses to regular and Linux GSO sends.
- Adds platform, integration, LRU, and benchmark coverage.
File summaries
| File | Description | Review notes |
|---|---|---|
internal/socket/transport_gso_linux.go |
Integrates packet-info source selection with GSO writes. | Moderate (3 votes): add stale-source fallback before classifying GSO writes as blackholed. |
internal/socket/ip.go |
Tracks packet destinations and sends replies with source-aware packet info. | Moderate (1 vote): preserve IPv4 interface index. Moderate (1 vote): prevent a failed send from deleting a newer concurrent entry. |
internal/socket/ip_pktinfo_v6_test.go |
Tests IPv6 and dual-stack packet-info behavior. | — |
internal/socket/ip_pktinfo_unix.go |
Parses Unix ancillary packet-info data. | — |
internal/socket/ip_pktinfo_test.go |
Tests IPv4 arrival-address replies. | Moderate (2 votes): make the assertion capability-aware when packet info is unavailable. |
internal/socket/ip_pktinfo_sys_pktinfo.go |
Defines OS-specific packet-info constants. | Critical (1 vote): use the correct Darwin constant separately from Linux. |
internal/socket/ip_pktinfo_sys_other.go |
Provides packet-info constants for other Unix systems. | Critical (1 vote): avoid enabling unsupported BSD control-message parsing or implement the BSD format. |
internal/socket/ip_pktinfo_other.go |
Provides the non-Unix parser fallback. | — |
internal/socket/ip_pktinfo_lru_test.go |
Tests bounded cache behavior. | — |
internal/socket/ip_pktinfo_linux_test.go |
Tests source preservation for segmented Linux sends. | — |
internal/socket/ip_pktinfo_bench_test.go |
Benchmarks packet-info handling. | — |
Review details
Suppressed comments (2)
internal/socket/ip.go:173
recordpreserves the IPv4 receive interface inla.ifIndex, but this message discards it. For IPv4 link-local addresses and policy-routed multihomed sockets,IP_PKTINFOneeds the interface index as well as the source address to keep the reply on the arrival interface; the existing qng packet-info builder preserves both. IncludeIfIndex: la.ifIndexhere.
return (&ipv4.ControlMessage{Src: la.addr.AsSlice()}).Marshal()
internal/socket/ip.go:290
packetInfoForreleaseslocalMubefore this write, so a concurrent receive can replace the entry with a newer arrival address before the write fails.forgetLocal(dst)then deletes that newer entry, causing the next reply to use route-selected source despite having just observed the peer on the new address. Delete only if the entry still corresponds to the control message used for this attempt (or otherwise version the entry).
t.forgetLocal(dst)
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // sysIPPktinfo matches nothing: the BSDs report an IPv4 destination through | ||
| // IP_RECVDSTADDR, which x/net cannot pair with an IPv4 source on send. IPv6 | ||
| // packet info (RFC 3542) is recognized. | ||
| const sysIPPktinfo = -1 |
There was a problem hiding this comment.
Accurate, leaving open. On BSD enablePacketInfo succeeds via IP_RECVDSTADDR, so the socket pays for ReadMsgUDP while IPv4 never records — behavior is correct (replies fall back to the kernel's choice, as before this PR), just wasted work. Fix is to count a family as enabled only when parsePacketInfo can read it; no BSD host to verify on, so it lands separately.
IpTransport.send drops an arrival address the kernel refuses and lets the kernel choose instead; WriteMsgUDP did not, and on Linux it carries every ECN-marked packet. A stale entry there silently blackholed a peer until it sent to us again: the datagram never left, and the caller was told it had.
The BSDs report an IPv4 destination through IP_RECVDSTADDR, which the transport does not use, so a reply there leaves from the address the kernel picks and the test failed rather than skipped. sysIPPktinfo now names that condition on every platform.
|
pushed two small fixes to address copilot feedback. |
Problem
A wildcard-bound socket has no fixed source address: the kernel picks one per reply by route. On a multi-homed host, or one whose peer is reached over a bridge or a container network, that is not always the address the peer sent to.
Concrete case: a node runs on a Kubernetes worker with
hostNetwork, bound to0.0.0.0:4434and advertising the host's LAN address. A client pod on the same worker dials that LAN address. Its packet reaches the node over the CNI bridge, so the route back to the pod goes over the bridge too, and the reply leaves from the bridge's address rather than the LAN address:The QUIC client receives packets from an address it never dialed, starts path validation, and fails:
Clients on other hosts are unaffected, since their packets arrive and leave over the uplink, so this only shows up when a client shares a host with a node.
Change
When the socket is bound to the unspecified address,
IpTransportenablesIP_PKTINFO/IPV6_RECVPKTINFO, records the local address each remote's datagrams arrive at, and sends to that remote with a packet-info control message naming it as the source. This mirrors what quinn-udp does for the Rust iroh (RecvMeta::dst_ip,Transmit::src_ip). On Linux, the GSO send path inMagicConn.WriteMsgUDPcarries the message next toUDP_SEGMENT.x/net(Windows) keeps the old reads and writes.Tests
TestIpTransportRepliesFromArrivalAddress(all platforms): a client bound to127.0.0.1sends to a LAN address of the host through a wildcard-boundMagicConnand checks that the reply comes from that LAN address. By route the reply would come from127.0.0.1, so the test fails without the change. Skips when the host has no non-loopback IPv4 address.TestMagicConnWriteMsgUDPKeepsArrivalAddress(Linux): the same with a segmentedWriteMsgUDP, checking every segment's source.Verified on macOS (arm64) and Linux (arm64, in Docker);
go vetfor linux, darwin, windows and freebsd; the full test suite on macOS; and a downstream consumer built and tested against this branch.