netstack: let ICMP echo handlers consume requests#13227
Merged
Conversation
IPv4 echo requests are delivered to the transport dispatcher before netstack builds the in-stack echo reply, but a per-stack default handler cannot tell the ICMP endpoint that it has taken ownership of the request. This leaves embedders that install a custom ICMP handler with no direct way to suppress the built-in reply. Add a transport dispatcher extension that reports whether the per-stack default transport handler consumed the packet, and use that signal in the IPv4 and IPv6 ICMP echo paths. For IPv6, deliver echo requests through the same endpoint/default-handler path before deciding whether to synthesize the built-in reply. Keep ordinary endpoint delivery distinct from default-handler ownership so registered ICMP endpoints and raw sockets can observe echo requests without suppressing the normal reply path. This also preserves the IPv4 temporary-address behavior introduced by the earlier echo-handling fix. Signed-off-by: Zi Li <zi.li@linux.dev> Signed-off-by: Amaindex <amaindex@outlook.com>
39b150a to
ecd253a
Compare
…ndler PiperOrigin-RevId: 919185728
ecd253a to
b8389ea
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.
netstack: let ICMP echo handlers consume requests
Overview
This change makes ICMP Echo Request handling follow the usual
SetTransportProtocolHandlerdefault-handler contract: if an ICMP default handler returns true, the stack treats the request as consumed and does not also synthesize a built-in Echo Reply.The default behavior is preserved when there is no ICMP default handler, or when the handler returns
false.The intended scope is small:
LocalAddressTemporarybehavior from netstack: allow defaultHandler respond ICMPv4Echo in promiscuous mode #11609Echo Requestdefault-handler contract for IPv6ICMPv6 NDPand other ICMP control paths unchangedRelated discussion:
Context
TCP and UDP default handlers already have a clear ownership signal:
ICMP Echois different today becauseEcho Requests are intercepted by IPv4/IPv6 network endpoints before the built-in reply is generated.That makes it possible for an embedder to receive an
Echo Requestand still get a second reply from netstack.The earlier discussion started in #8657, where @fredwangwang reported that
ICMP Echocould still be answered by the stack even when an ICMP handler was installed withSetTransportProtocolHandler. @deepcode2019 noted the same need, and @kevinGC suggested that a tested PR would be welcome.#11609 took a narrow first step for IPv4 promiscuous-mode temporary addresses. After it merged, @ericpauley pointed out that restoring the old behavior could require reimplementing ICMP in a custom handler, and @dyhkwong noted the remaining IPv4/IPv6 inconsistency.
This change addresses that follow-up without widening the behavior change beyond Echo Request default-handler ownership.
The compatibility-sensitive case is
runsc. It registers ICMP as a normal transport protocol, but does not install an ICMP default handler for this path:runsc/boot/loader.goSo the no-default-handler path remains the ordinary built-in
Echo Replypath.Why This Matters
The main beneficiary is not ICMP itself, but embedders that use netstack inside user-space tunneling or virtual-networking software.
Several widely used downstream or adjacent projects use netstack, or downstream forks of it, in this role, including Tailscale's userspace networking, wireguard-go's netstack TUN backend, sing-box/mihomo-style TUN stacks, and tun2socks.
For those programs, an
Echo Replyis often a policy decision:If netstack synthesizes a local
Echo Replyafter the embedder has consumed the request, unreachable or policy-blocked addresses can appear reachable.After this change, ICMP default handlers can return
trueand take responsibility for Echo behavior. That lets tunneling software return replies that reflect its own forwarding policy and the current state of the tunnel.Flow
Current behavior:
New behavior:
The asymmetry above is intentional: the new default-handler ownership rule is shared by IPv4 and IPv6, while
LocalAddressTemporaryis a pre-existing IPv4 compatibility guard from #11609.Implementation
Relevant code points:
SetTransportProtocolHandlerstores a per-stack default handler.TransportDispatcherWithDefaultHandlerResultcarries the narrower ownership signal.DeliverTransportPacketWithDefaultHandlerResultshares the NIC transport delivery path.defaultHandlerresult propagation reports true only when the default handler consumed the packet.network/ipv4/icmp.gouses that signal while preservingLocalAddressTemporary.network/ipv6/icmp.godelivers Echo Requests through the ICMP endpoint/default-handler path before deciding whether to synthesize the built-in reply.TransportPacketHandledis too broad for this decision:Only one branch should suppress the built-in Echo Reply:
This change therefore adds a narrow dispatcher extension:
The second result is
trueonly when the per-stackdefaultHandlerreturnedtrue:This keeps endpoint visibility separate from default-handler ownership.
For IPv6, the built-in reply path now saves the data needed for the reply before transport delivery, because
DeliverTransportPacketmay modify thePacketBuffer:This also makes IPv6 Echo Requests observable through the same registered-endpoint/default-handler delivery path used by IPv4. A registered ICMP endpoint does not suppress the built-in reply; only a per-stack default handler returning
truedoes.Compatibility
Default runsc/netstack behavior should remain unchanged.
This change separates two decisions:
Behavior matrix:
ICMPv6 scope:
I am not trying to make IPv6 mirror the IPv4
LocalAddressTemporarypath in this change. That would be a separate behavior change. Here, IPv6 gains the Echo Request endpoint/default-handler delivery path and the default-handler ownership rule.LocalAddressTemporaryremains useful as an IPv4 signal for embedders that already need to distinguish promiscuous-mode temporary local delivery from ordinary local delivery.Follow-ups
I plan to keep the remaining work split into smaller follow-ups.
If this direction looks acceptable, I can follow up promptly with the mechanical helper extraction first. The
ICMP ForwarderAPI can then be discussed on top of that smaller refactor, so the public API shape does not have to be decided in this change.First, factor the built-in IPv4/IPv6
Echo Replyconstruction into an internal helper while preserving route lookup, rate limiting, stats, checksums, IPv4 options, IPv6 traffic class handling, and output hooks.Then add an
ICMP ForwarderAPI, analogous to the TCP/UDP forwarders:The goal of
ForwarderRequest.Reply()is to let embedders explicitly delegateEcho Replygeneration back to the stack, without copying netstack's reply construction logic downstream. That should also provide a cleaner way to restore the old "let netstack reply" behavior when a custom handler decides not to own a particular Echo Request.Tests
The new tests cover the behavior matrix directly.
IPv4:
IPv6:
Commands:
All of the commands above pass in my fork branch,
netstack-icmp-echo-default-handler.Review Context
While preparing the change, I discussed the direction with several downstream/heavy netstack users to validate the embedding use case and compatibility expectations.
I also used AI-assisted review as an additional pass to look for missing edge cases. The technical argument here is still based on the source links, behavior matrix, and tests above.
Fixes #8657.
FUTURE_COPYBARA_INTEGRATE_REVIEW=#13189 from Amaindex:netstack-icmp-echo-default-handler 8b0e162