Document the fragmented-traffic drop on the direct path (#779) - #780
Merged
Conversation
The IPv6 extension-header walk stops at Fragment (44) and ESP (50), so those packets match no L4 dispatch branch in handle_ip(): an allowed packet is silently blackholed on the direct path. IPv4 fragments (IP_MF, first fragments included) are dropped even earlier -- before the WireGuard hijack, unlike their IPv6 counterparts. Decide the issue's option B: keep dropping (reassembly rejected as a battery/memory cost against traffic PMTUD keeps rare; dispatching first fragments alone would strand mid-flow continuations), document it at both drop sites and in the codebase map, state that ESP stays a permanent limitation, and log the previously silent drop instead of vanishing. See #779.
frag_off is network byte order but IP_MF and IP_OFFMASK are host-order constants, and the test had no ntohs. On little-endian Android that never looked at More Fragments at all: it tested bit 13 of the byte-swapped value, i.e. bit 0x20 of the real field's low byte, so the drop fired on an arbitrary slice of fragment offsets (256-511 bytes, 768-1023, ...). True first fragments (MF set, offset 0) fell through and were parsed as whole packets; last fragments (MF clear, offset non-zero) fell through too and had their payload read as a TCP/UDP header, yielding garbage ports. The ERROR log printed the same swapped value, which is why the offsets in it never looked wrong enough to chase. Convert once and test both halves, so first, middle and last fragments alike drop, and the comment added alongside it is true of the code. Also rate-limit the new undispatchable-protocol WARN the way the two WireGuard drop paths in this function already do -- a single ESP flow hits it every packet -- and keep HOPOPTS/IGMP/ESP out of that log, mirroring the exemption at the "Unknown protocol" site above. The throttle counts on its own counter: sharing one with the exempt protocols would let steady ESP traffic eat every slot and silence the protocols worth seeing.
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
1 participant
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.
Closes #779.
Implements the issue's option B: fragmented traffic keeps being dropped on the direct path, and the limitation becomes stated, deliberate, and observable rather than silent.
Decision rationale
What changed
ip.c:IP_MFguard documenting that first fragments are dropped deliberately, that this fires before the WireGuard hijack (unlike IPv6 fragments), and why reassembly was rejected.elseon the L4 dispatch chain: a previously silent blackhole now logsProtocol %d allowed but not forwardable, droppingat WARN — one logcat line per affected packet, no DB writes.ip6_ext.h: the Fragment bullet of the walk's contract now cross-references IPv6 fragmented traffic stays blackholed on the direct path (Fragment header stops the ext-header walk) #779.agents/docs/codebase-map.md: new "Known limitations (deliberate)" section covering both families' behavior, the IPv4-before-WG-hijack asymmetry, and the ESP decision.Verification
ndk;27.2.12479018, aarch64-linux-android24)-fsyntax-only -Wall -Wextraoverip.c: clean; all warnings pre-existing, none in changed regions.-Wall -Wextra -Werror:ip6_ext_testanddns_frame_testboth pass.