Repository navigation
Conversation
`Assembler::add` returns `Err(TooManyHolesError)` once the packet holds `ASSEMBLER_MAX_SEGMENT_COUNT` (default 4) discontiguous ranges. `PacketAssembler::add` and `add_with` called it for its side effect and discarded the result, so on refusal the fragment's bytes were copied into the buffer while the range went unrecorded. Nothing then reports the loss. `is_complete` compares the total size against `peek_front`, which the unrecorded range never advances, so the datagram cannot complete: its slot stays occupied until the reassembly timeout expires it, and the caller sees a silent timeout rather than an error it could count or act on. Four ranges is not hard for an out-of-order sender, let alone a deliberate one. Both now propagate as `AssemblerError`, which is the error the callers already handle. The test spends the budget one byte at a time and asserts the next add is refused; it fails on the previous code, which returned `Ok(())`.
`proto-ipv6-fragmentation` has been a feature that enabled nothing: it
turns on `_proto-fragmentation`, and no code reads it on the IPv6
receive path. An IPv6 Fragment header therefore reached the bottom of
`process_nxt_hdr` and drew an "unrecognized next header" Parameter
Problem — the datagram was never reassembled, and no CI feature
combination compiled a line of the path, so nothing said so.
This implements RFC 8200 section 4.5 reassembly.
Identity is `{source, destination, identification}` — a new
`wire::ipv6::Key`, exported as `Ipv6FragKey` and added to `FragKey`.
Deliberately NOT the next header, which `ipv4::Key` does include: for
IPv6 the upper-layer protocol lives in the Fragment header, every
fragment carries its own copy, and keying on it would let a sender put
one datagram into two slots by lying in a later fragment.
Placement. Reassembly sits AFTER the address checks, where the IPv4
path's sits before them: RFC 8200 reassembles at the destination, so a
datagram this interface is not the destination of must not take a slot.
It sits before the raw-socket filter, so a raw socket is handed the
datagram rather than its pieces.
What is enforced:
- RFC 6946 atomic fragments (offset 0, no more fragments) bypass the
assembler entirely, so they cannot collide with a genuinely
fragmented datagram that shares their identification.
- A non-final fragment that is not a multiple of 8 octets is
discarded with Parameter Problem code 0 pointing at Payload Length.
- A fragment whose tail would carry the datagram past 65535 octets is
discarded with code 0 pointing at its Fragment Offset.
- RFC 7112: a first fragment that does not carry its upper-layer
header is discarded with code 3, a new `ParamProblem` variant. The
header chain is not walked — what is checked is that the header the
Fragment header NAMES is present, which is the case the RFC was
written for: a chain split so a stateless filter cannot see the
ports it is filtering on. An extension header or an unknown protocol
is not guessed at, it is left unenforced.
- Two finals that disagree on the total reset the slot instead of
resizing it.
- RFC 4443 section 2.4: none of those errors is sent to a multicast
destination.
- The reassembled packet's next header and hop limit come from the
FIRST fragment, kept in the assembler slot so that the fragment
which completes the datagram cannot substitute its own.
Two things the shape of the code is owed an explanation for.
`Ipv6Reassembly` hands `process_ipv6` the assembler as a field borrow
rather than the whole `FragmentsBuffer`. The 6LoWPAN path decompresses
into that struct's `decompress_buf` and passes the resulting slice to
`process_ipv6`, so the slice holds an immutable borrow of one field for
as long as the packet — which leaves no way to pass `&mut` to the
struct beside it. The assembler is a disjoint field and can still go.
The first fragment's fields live in `PacketAssembler` rather than a
table beside the set, so that the `reset` which frees a slot clears
them too; a side table would be one more thing to keep in step with
expiry and eviction.
Also widens the `PacketAssemblerSet` import from ipv4-or-sixlowpan to
the `_proto-fragmentation` umbrella its field already used. That was
unreachable while `proto-ipv6-fragmentation` enabled nothing, and is a
build error the moment it alone turns the umbrella on.
Nine tests over both media, and a CI feature row to run them: upstream
had no combination that enabled `proto-ipv6-fragmentation`. The
reassembly cases send a checksummed ICMPv6 echo request split across
fragments and require the echo REPLY, so every octet has to land at its
own offset for the case to pass.
RFC 5722: overlapping fragments make a datagram untrustworthy. Two senders claim the same octets, the receiver cannot say which a firewall in the middle saw, and the whole point of the attack is that they disagree. Everything received for the datagram is discarded. Silently. An ICMP answer here would be a reflection whose size the attacker picks, and the RFC asks for none. `Assembler::overlaps` answers the question from `iter_data`'s recorded ranges, so the receive path can ask BEFORE it copies the fragment's octets over what is already in the buffer — after the copy the evidence is gone. A fragment that repeats an earlier one EXACTLY counts as an overlap too. That is the stricter reading and it is a trade worth naming: a same-offset, same-length repeat carrying different octets is precisely the rewrite RFC 5722 exists to stop, and telling it apart from a duplicate the network produced would mean comparing payloads. The cost is that a genuinely duplicated fragment drops a datagram the upper layer then has to send again. Both behaviours have a test, so a reviewer who wants the looser rule can see exactly which case changes.
Under `alloc` a reassembly slot's buffer grows to whatever offset a fragment claims and is never given back. One datagram that reaches far into the 65535-octet space therefore pins that much memory in its slot for the life of the interface, and REASSEMBLY_BUFFER_COUNT slots do it in parallel. No sender has to complete anything to arrange it: the growth happens on the first fragment, and the ceiling is the protocol's rather than the embedder's. Two changes, and neither is a policy the library picks for you. `Interface::set_reassembly_max_len` bounds the largest datagram reassembly will produce, defaulting to the 65535 an IP header can describe. It is enforced in the assembler rather than in a protocol's receive path, so it holds for every protocol that reassembles, and it is checked BEFORE the buffer grows rather than after — a bound applied afterwards is not a bound. A fragment past it is dropped quietly: this is a local resource decision, not a protocol error the sender could have avoided, and there is no ICMP code that says "I chose not to". The bound is stamped onto a slot when the slot is handed out, so lowering it never strands a datagram already in flight above its own ceiling. A slot handed out for a new datagram now starts clean: the buffer is cleared and shrunk back to REASSEMBLY_BUFFER_SIZE. That turns the steady-state cost back into COUNT x REASSEMBLY_BUFFER_SIZE, and it also means octets of one datagram can never be read as part of the next. Tests: the assembler refuses the octet past its ceiling and refuses a total that exceeds it; a slot gives its buffer back when reused; and end to end, a datagram that fits under 65535 but not under a lowered bound is dropped in silence, then reassembles once the bound is raised — so the bound is shown to be the only thing that stopped it.
Reassembly state is currently invisible and unreleasable from outside.
The only lever is `set_reassembly_timeout`, which is the wrong
instrument for anyone keeping a budget of their own: it is
per-interface, it is coarse, and it cannot single out the datagram
whose owner has just gone away — a namespace torn down, a socket
closed, a tenant over its share.
Four accessors on `Interface`, and a `PacketAssemblerSet` API behind
them:
- `reassembly_entries()` — what is held, as `ReassemblyEntry`.
- `reassembly_entry(&FragKey)` — one of them.
- `reassembly_evict(&FragKey)` — let one go; returns whether it was
there.
- `reassembly_clear()` — let all of them go.
`ReassemblyEntry` reports `received` and `capacity` separately, and
that separation is the point rather than a detail: under `alloc` a
datagram whose first fragment claims a far offset holds a buffer far
larger than what has arrived, and a budget that watched only the octets
received would not see the octets retained.
`FragKey` becomes `pub`, and `Ipv6FragKey` gains a constructor and
accessors. Without the constructor the key is only obtainable from
`reassembly_entries`, which is enough to walk and evict but not enough
for an embedder that watches frames arriving and derives the identity
itself — which is exactly the caller that wants to refuse a datagram
before its buffer grows. The test derives a key from the addresses and
identification it would have read off the wire and evicts the
interface's own entry with it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1207 +/- ##
==========================================
+ Coverage 82.57% 82.69% +0.12%
==========================================
Files 83 83
Lines 26174 26499 +325
==========================================
+ Hits 21613 21914 +301
- Misses 4561 4585 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
|
Disclosure under this project's LLM policy (CONTRIBUTING → nlnetlabs.nl/llm-policy): the code in this pull request and its description were produced with an LLM (Anthropic's Claude) working for 254CARBON, and that should have been stated when it was opened. The policy does not accept LLM-generated code, so we are withdrawing it. Apologies for any review time it cost. This comment was also written with Claude, at the account owner's direction. |
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.
The problem
proto-ipv6-fragmentationdoes not do anything, and it does not compile.It turns on
_proto-fragmentationand nothing reads it on the IPv6 receive path, so an IPv6 Fragment header falls through to the bottom ofprocess_nxt_hdrand draws an "unrecognized next header" Parameter Problem. The datagram is never reassembled.On
maintoday, enabling it on its own is also a build error:FragmentsBuffer'sassemblerfield is gated on the_proto-fragmentationumbrella while itsuseis gated onany(proto-ipv4, proto-sixlowpan), andproto-ipv6-fragmentationis the only way to turn the umbrella on without either of those. No row inci.shenables the feature, which is why this has gone unnoticed.What this does
Five commits. 0001 stands alone and is worth taking on its own merits even if the rest is unwanted.
PacketAssembler::{add, add_with}calledAssembler::addfor its side effect and discarded itsResult.Assembler::addreturnsErr(TooManyHolesError)onceASSEMBLER_MAX_SEGMENT_COUNT(default 4) discontiguous ranges are recorded — so on refusal the fragment's octets were copied into the buffer and the range went unrecorded.is_completecompares againstpeek_front, which the unrecorded range never advances, so the datagram cannot complete: the slot is occupied until the reassembly timeout and the caller sees a silent timeout rather than an error. Affects IPv4 and 6LoWPAN too.PacketAssemblerSetimport to the umbrella, and adds aci.shrow that runs the path.Interface::set_reassembly_max_len, and a slot's buffer is given back when it is reused.reassembly_entries/reassembly_entry/reassembly_evict/reassembly_clear.Details worth a reviewer's eye
Identity is
{src, dst, identification}— not the next header.ipv4::Keyincludes the protocol field; for IPv6 the upper-layer protocol lives in the Fragment header, every fragment carries its own copy, and keying on it would let a sender split one datagram across two slots by lying in a later fragment.Reassembly runs after the address checks, where the IPv4 path's runs before them, because RFC 8200 reassembles at the destination — a datagram this interface is not the destination of should not take a slot. It runs before the raw-socket filter so a raw socket sees the datagram rather than its pieces.
Ipv6Reassemblypasses the assembler as a field borrow rather than&mut FragmentsBuffer. The 6LoWPAN path decompresses into that struct'sdecompress_bufand hands the resulting slice toprocess_ipv6, so the slice holds an immutable borrow of one field for as long as the returned packet and there is no way to pass&mutto the whole struct alongside it.Option<&mut …>withNonefrom 6LoWPAN was the cheaper option and would have been a silent capability gap. Happy to reshape this if you would rather splitFragmentsBufferinstead.The first fragment's next header and hop limit live in
PacketAssembler, so theresetthat frees a slot clears them; a side table would be one more thing to keep in step with expiry and eviction.Enforced: atomic fragments bypass the assembler (RFC 6946), so they cannot collide with a genuinely fragmented datagram of the same identification; a non-final fragment that is not a multiple of 8 draws Parameter Problem code 0 at Payload Length; a fragment whose tail passes 65535 draws code 0 at its Fragment Offset; a first fragment missing its upper-layer header draws code 3 (new
ParamProblem::IncompleteHdrChain, RFC 7112); two finals that disagree on the total reset the slot; none of these errors is sent to a multicast destination (RFC 4443 §2.4); and the reassembled header comes from the first fragment, not from whichever one completed it.Two judgement calls I would like your opinion on
Deliberately not included
ICMPv6 Time Exceeded on reassembly timeout (RFC 4443 §3.3). It needs an egress path — the expiring side has no transmit token — plus retention of the first fragment to quote, and it bears on neither correctness of delivery nor bounded memory. I would rather send it as its own PR, with its own case for "no first fragment was seen, so stay silent", than bundle it here.
Testing
Nine tests over both media in
iface::interface::tests::ipv6::fragmentation, plus assembler unit tests. The reassembly cases send a checksummed ICMPv6 echo request split across fragments and require the echo reply, so every octet has to land at its own offset for a case to pass.ci.shrow: 429 / 433 / 437 / 439. 0001 predates that row and passes under default features: 674, against 673 before it — the one new test.default, the new row, anallocrow and the 6LoWPAN row: all green (676 / 439 / 277 / 195 on the rebased branch).cargo clippy --tests --examples -- -D warnings: clean.RUSTFLAGS="--cfg ci_forbid_unused --deny unused --deny irrefutable_let_patterns" cargo check --tests --examples: clean.cargo check --examples: clean.The overlap rule and the memory bound were each confirmed to fail their tests with the rule disabled, rather than assumed to be doing something.
MSRV 1.91.0; developed and tested on 1.91.1.
Why I need it
We are building an IPv6 host stack on smoltcp and need reassembly under budgets the embedder owns — which is what 0004 and 0005 are for, and why they are last: the first three stand without them. Happy to split this into smaller PRs if that suits review better.