implement the external code review, plus three defects it missed

[w453y]

review-fixes: external review, test infrastructure, CI, and IPv6 echo origination

158 commits, 105 files, +11057/-389, off main at 8b8282e.

This branch started as the implementation of a four-document external review
(code review, architecture, testing, symmetric deployment) and grew to cover
everything that review made reachable. Three quarters of it is test and CI
infrastructure, because most of the defects it fixes were on paths nothing
could execute.

Every behaviour change here has a before-arm on record: a failing test or a
capture taken against the unfixed build. Where a claim is reasoned rather
than measured, it says so.

---

1. Fast-path correctness (XDP)

• Deferred GTSM ran after the promiscuous pass (087c9af). With any
  multihop session configured, prog_flags bit 1 makes the parser defer the
  TTL verdict; a low-TTL packet for an unconfigured address pair then reached
  the cfg == NULL branch and was passed to the stack. The comment there
  claimed an unconfigured pair must still drop, which was false outside
  promiscuous mode. This was not in the review — it was found while verifying
  one of its findings, and it is what turns a parity gap into a reachable
  off-link spoof.
• Fragmented UDP aimed at a BFD port is dropped (3fa4ffc), first
  fragments only, with the port test short-circuited so udp->dest is never
  read at a nonzero offset. Non-first fragments pass, deliberately: they have
  no counter to witness, and counting fragments generally is not an error
  signal.
• Control packets carrying the A or M bit are dropped (eb4add9) with
  their own stat slot rather than passed. A malformed header stays counted and
  passed — a broken header is not evidence of an attack — but a well-formed
  header we cannot honour must not reach a userspace path that accepts it as
  plain unauthenticated BFD. bfd_hdr_valid became bfd_hdr_verdict,
  matching the parser's sentinel idiom.
• IP-optioned UDP counts in its own slot (74e9e0d) instead of being
  filed as a BFD reject.
• A failed sweeper arm has a witness (2e5edca). bpf_timer_init can
  fail, its return was discarded, and inited was already set — so on a
  kernel without timer support the sweep silently never armed and nothing said
  so. Now captured in init_err with a stat slot. Deliberately not retried:
  on an unsupported kernel that runs a failing helper on every packet forever.
• The multihop flag is named by value, not bit index (8696ea2).

2. Validation parity between the two planes

The engine validated control packets in XDP and, separately, in userspace, and
the two disagreed. Closed in three parts:

• One shared acceptance rule (a78a879). bfd_ctrl_check() in
  bfd_shared.h, called by both planes, taking host-order fields explicitly
  because the two byte-swap with different helpers. The divergence it exposed:
  userspace never looked at bfd->len at all, so a packet claiming length 200
  inside a 24-byte datagram was accepted there and rejected in XDP. Fixing
  that also required MSG_TRUNC on all four recvmsg calls, or the two
  planes would still have compared different lengths.
• GTSM on the userspace receive path (808ab96, then 7815e87). The
  first attempt used IP_MINTTL / IPV6_MINHOPCOUNT. The namespace rig then
  showed both are inert on UDP — Linux enforces IP_MINTTL only for TCP — so
  a TTL-64 packet reached the session in both families. The v6 half had been
  broken since long before this branch. Replaced with IP_RECVTTL /
  IPV6_RECVHOPLIMIT and a per-packet check, placed before the acceptance
  predicate. A missing cmsg reads -1 and drops.
• your_disc must name a session (4ff544d), or be zero with the peer in
  Down/AdminDown, across all four receive blocks. The old code fell back to
  the address pair on any miss. This is XDP's own rule and has been running on
  the live mesh for months, so it closes a divergence rather than inventing
  policy.

3. Dataplane socket robustness

• The XDP program outlived the engine (13819dd). Attach is now through
  bpf_link_create, so the kernel detaches on process death including
  SIGKILL. Before-arm on record: with the flags-based build, pkill -KILL
  left the program attached and answering control packets from a frozen
  tx_config. Consequence for operators: ip link set dev X xdp off no
  longer removes it — killing the owner does.
• Receive drains had no budget (88a1eef). Three MSG_DONTWAIT drains
  ran to EAGAIN, so a sustained flood of frames XDP passes to the stack kept
  the loop inside a drain and starved transmit, detect, dplane read and echo.
  Reaching the stack needs only a well-formed header at TTL 255 naming an
  unconfigured pair — trivially generatable, no session required. Not in the
  review; found while deciding whether a dead-man switch was warranted, and it
  is why one was not needed.
• dp_send treated EAGAIN as fatal (ea67ef3). dp_conn is
  O_NONBLOCK, so a full send buffer surfaced as an error whose only path
  closed the connection and orphaned every session. Replaced with a 64KB
  outbox that every message goes through, flushed from the main loop.
  Ordering is correct by construction because nothing bypasses the queue, and
  partial writes fall out for free. All four disconnect paths now route
  through one dp_drop_conn(why) that clears the queue — a stale outbox
  otherwise carried into the next connection.
• Out-of-bounds read in the bffdp parser (0441f6c), found by the fuzz
  target on its first run with a corpus. dp_process can reply, a reply on a
  full queue calls dp_drop_conn, which zeroes dp_have mid-loop; both
  counters are size_t, so the subtraction underflowed and the loop walked
  past dp_buf. Observed 42 bytes past the end. Fixed by making the loop
  condition underflow-proof rather than by knowing which callee shrinks the
  buffer. 45-byte reproducer committed.

4. Lifetime and configuration-change bugs

• echo_peers and tx_config on shared peers and address moves
  (343bb8f). echo_peers was keyed on peer address alone, so tearing down
  one session removed echo for another sharing that peer; and sm_addrs ran
  unconditionally on the update path, so an address change leaked the old
  kernel map entries. Now echo_peer_refresh() re-derives membership by
  scanning the session table rather than refcounting — a refcount that drifts
  by one silently enables or disables echo with no witness, while a 64-slot
  rescan on a config event is free.
• AdminDown is announced before a session is freed (78547cd). An orderly
  local teardown previously made the peer burn a full detect timeout. Three
  packets, because there is no retransmission once the slot is gone.
  Deliberately not sent on the --dp-hold orphan path, where the peer must
  not notice. Proven on the wire: the peer now reports neighbor signaled
  session down rather than control detection time expired.
• The slow transmit rate is jittered (8845deb). Only the Up branch had
  RFC 5880 §6.8.3 jitter, so a mesh coming up together synchronised into a
  burst every second. Jittering 1s to 750ms is not an undershoot — §6.8.7
  constrains DesiredMinTxInterval, not the resulting gap.
• Received control packets are counted in both planes (650ab2b).
  rx_pkts did not exist for the userspace half, so non-ktx receive was
  structurally zero and show bfd peers counters reported 0 input.
• Echo counters are reported (b650227). The engine had the numbers all
  along and the counters reply filled only the control fields.

5. Observability

• Stat slots defined once (9e4b5a6). BFD_STAT_LIST(X) in
  bfd_shared.h; the BPF side takes only the enum so no name strings land in
  the object, and the harness parses the header rather than keeping a fourth
  copy of the table. All twenty bare count(N) calls became named constants.
• JSON snapshot on SIGUSR1 (ee9e0de, b18ea0a), plus flap accounting
  and last_reason. Written to a temp path and renamed so a reader never sees
  a half-written file; the handler only sets a flag, so nothing in it needs to
  be async-signal-safe. This snapshot is what most of the test suites assert
  against, and it is why the counter-fidelity fix above had to come first.
• Log levels (05013be, 335c1db, 4a6032a). State transitions stay at
  info deliberately: it is the most operationally valuable line the process
  emits, and hiding it means an operator debugging a flap has to restart at a
  higher verbosity to see what already happened.
• Loop counters and inter-pass gaps (cd7a7e9), which is what settled the
  tick-ladder question below.
• v6 session keys render correctly in the loader and all slots are dumped
  by name (184e5b8); the loader's age arithmetic no longer wraps when a
  packet races the now-snapshot (aa693cd).

6. Main loop restructure (architecture item 2.2)

• The loop is clocked on a timerfd and polls every socket (2fac4ad),
  replacing the SO_RCVTIMEO blocking receive that was the de facto clock.
  That timeout slept on the jiffy wheel, which put a hard floor under
  detection resolution that no tuning could cross — measured, not assumed; see
  the tick ladder below.
• Only sockets poll reported readable are drained (c1311a1), one batch
  map fetch per pass instead of a lookup per session (f6bd178), and the
  dplane fds are polled rather than called blind (6c654b8).
• The dead SO_RCVTIMEO and the sub-jiffy warning it justified were removed
  (ad13120).
• The ringbuf still has no consumer in engine mode, and f9ef2c2 says so
  explicitly rather than leaving it implied.

7. Multi-interface fast path (architecture item 2.3)

• Coverage is per session, not per process (77e062e, e1fdd94). The ADD
  has always carried ifindex and ifname and the engine ignored both, so a
  single-hop session bfdd placed on any interface other than --kernel-tx
  silently ran userspace-only. ktx_attach() is split into ktx_load() and
  an idempotent ktx_attach_if(); the same loaded program is attached on
  demand to whatever interface a session arrives on, with per-interface mode
  fallback because a veth or a driver without native XDP refuses drv mode.
• No detach path in v1, on purpose: bfdd churns sessions during
  reconfiguration and a flapping link teardown is worse than the cliff it
  fixes.
• 0591b2f records what this was worth — the off-interface session could not
  stay up at all.

8. IPv6 echo origination

The echo originator was IPv4-only. The stated reason was an assumption nobody
had tested: that a neighbour's forwarding plane does not loop a self-addressed
IPv6 packet the way it loops a v4 one, inferred from FRR sourcing its v6
echoes at the peer.

It loops. Measured before writing any code, with a v4 control arm in the same
rig so a v6 zero could not be a broken injector: 10/10 returned at hop limit
254 with the neighbour forwarding, 0/10 with it off.

• 30470f4 — seven v6 echo cases in the Layer 1 harness, landed first so the
  before-arm is in the history: the return with a known discriminator, the
  same frame with one we never sent, 254-not-self, off-link, not-self at 255,
  declined, and reflect.
• 6c2587d — the XDP half. parse.h's v6 branch dropped every return before
  the echo path was reached and read the UDP header only after the hop-limit
  check, which is the v4 trap verbatim; the pointer moved and the GTSM check
  gained the same narrow exception (echo port, hop limit 254, self-addressed).
  echo_reflect_v6 gained the return branch, ordered before GTSM with every
  miss returning rather than falling through, so BFD_STAT_ECHO_TTL stays
  unreachable in both families.
• 9452f47 — the engine half. Frame build split into echo_build_v4 /
  echo_build_v6 over a shared payload writer, 86-byte v6 frame, UDP checksum
  folded over the 40-byte pseudo-header, no IP checksum. A second stale family
  gate was found in stats.c, which reported echo as inactive on a
  transmitting v6 session; active now means the negotiated interval is
  non-zero rather than the family is v4.
• fd145c1 — the wire test and its evidence. Two arms of one config with only
  the neighbour's sysctl changed: 295 frames sent in each, all with a valid
  UDP checksum as judged by tcpdump rather than by our own fold; 0 returned
  with forwarding off; 295 returned with it on, every returned payload
  byte-identical to one that was sent.

9. Test infrastructure

Four layers, none of which existed at the branch point.

Layer 1 — BPF_PROG_TEST_RUN (783563a … 67a6ef2, and the v6 echo
cases). Loads the real object, builds synthetic frames, populates maps through
the syscall API, asserts verdict, returned frame and map deltas. Covers the
bounce field by field, the adjust_tail trim, the demux matrix including the
no-fallback-on-miss rule, fragments, IP options, the malformed/unsupported
matrix, the sweep via a test-only BPF object, and the echo matrices. Runs in
seconds and needs no testbed.

Layer 2 — userspace unit suites (65f9961, f16c9f4, 9d25e25,
6cdf973, ba79e48, c31a92f). The FSM transition table with no seams
required, detect timing and the RFC 5880 jitter bounds, bffdp framing over a
real socket torn at every boundary, session lifecycle in both families
including the address-move path, and shared detect-basis vectors driven
against both the engine and the XDP copy of the same rule.

Layer 3 — namespace rigs (9b7f30e, b8da5c6, 4db21d7, 9852361,
12e1714, cdd8943, f61e7b6). A veth/netns rig for the userspace receive
path — the path four review documents and two reviewers never reached, because
nothing could reach it. On its first working run it caught inert IP_MINTTL,
inert IPV6_MINHOPCOUNT, and the unimplemented demux. Extended to end-to-end
scenarios under pytest, multihop over a router namespace, and three scenarios
against stock FRR in containers.

Layer 4 — ABI, fuzz, and the harness itself (a825576, 96979d6,
054120f, 3f550b3). _Static_assert pins on every shared struct size,
offset and enum value, generated by a probe rather than hand-written; a
libFuzzer target for the bffdp parser driven through a read seam rather than a
socket, because an earlier socket-based harness found only connection-lifecycle
bugs in itself.

Harness bugs fixed along the way, each of which had been producing vacuous
passes: --only with a name matching nothing printed all cases passed with
no cases (88cd653); phantom sessions were picked by property and could
select a live peer (bb170fc); ip-options asserted a counter it no longer
used and had been failing unnoticed for three weeks (837e3b2); a map read by
name matched every loaded engine (41a8a01, 1965bef); --json was not
machine-readable (dbcc745).

10. CI

Five jobs on every pull request (7de3346 and the surrounding commits):

• full (self-hosted): build, four unit suites, fuzz regressions, netns rig
  and Layer 3 scenarios, FRR container scenarios.
• toolchain (matrix 18/19/20/21): the LLVM floor as a test rather than a
  comment. 18 and 19 must fail and the log must contain the expected
  diagnostic, so a Makefile breakage cannot satisfy the expected-fail arms.
• kernel (self-hosted): unit suites inside a virtme-ng microVM under a
  chosen kernel; adding one is a single matrix line.
• cross: the BPF object built for aarch64, with a negative arm proving a
  bogus triple fails.
• portability: build and make check on a stock machine with nothing
  preinstalled, plus scan-build and the perf harness. Its value is the stock
  machine — that is what caught the clang floor.

Plus a nightly fuzz workflow (60f61e1) which cannot run until this merges,
since scheduled workflows only fire from the default branch.

tools/pre-push (a669c5a) runs the injection matrix and refuses the push on
a failing case, skipping cleanly when the testbed is absent — a hook that
blocks pushes on an unrelated machine gets disabled, and a disabled hook
catches nothing.

11. Benchmark reproduction

perf/bfdwire.py and perf/check.py (0309498 … 7335d80) recompute the
published benchmark numbers from the committed pcaps, reproducing results 1
and 3 to the last digit. Result 2 is checked with tolerances, because its
percentiles could not be reproduced exactly and gating on an unrecoverable
figure would gate on the reconstruction rather than on the result.

Two portability traps are handled and documented: parse the hex rather than
tcpdump's decode, because one tcpdump build renders UDP/3784 as a Broadcom
lawful-intercept dissector; and pass -n, because otherwise addresses render
differently depending on the host's /etc/hosts.

12. Measurements, including three that corrected published claims

• Sweep ladder (762de40, a81cfa6, ba1d338). The sweep interval is
  now a load-time tunable, and the ladder that enabled shows a shorter sweep
  giving a larger mean overshoot. A falsification arm at 100ms produced a
  2.58ms maximum, which a 100ms quantizer cannot do. Confirmed in the code
  afterwards: the only path to Down with diag 1 is userspace fsm_detect; the
  kernel sweep clears a flag and emits a ringbuf event nobody consumes. The
  README's attribution of detection overshoot to 5ms sweep quantization was
  wrong. What this does not undermine: RX-clocked TX is about transmit, and
  it stands.
• Tick ladder (ab46d1e, cd7a7e9, dbdea40, 6418b18). The main-loop
  tick does quantize detection, down to about one jiffy, and no further —
  SO_RCVTIMEO slept on the jiffy wheel. Four explanations were falsified
  before this one. This is the measurement behind the timerfd restructure.
• Starved detection (ca61f16, 3a754fd). Under RT starvation the engine
  detects a dead peer ~950ms late against a 30ms budget, bounded by the RT
  throttle rather than by anything in the design. Engine self-report and an
  independent hypervisor capture agree on all ten samples, differing by the
  detect budget. The project is scheduler-immune in one direction only:
  transmit is answered from softirq, but noticing a quiet peer runs entirely
  in the loop being starved. The honest harm statement is a delayed local
  notification, not a lie on the wire.
• Symmetric kernel-TX (abf3e84, 802aa99). Two RX-clocked ends produce
  690223 packets in 5 seconds against a configured 10ms interval, against 1043
  with kernel-TX on one side. The README predicted the opposite — that two
  such ends would fall silent — and flagged it untested. "Silence" reads as a
  safe failure mode; a flood is not. Sessions stay Up while the link does not.
• Wedged-but-alive (c9e8cfd, 48ff9bb). With kernel-TX, 57 sessions
  stayed Up for 60 seconds with the engine SIGSTOPped, ~2000x the detect
  budget. A demonstrated consequence with no demonstrated cause: SIGSTOP is
  synthetic and the one known wedge mechanism was removed by the drain bound.
  Not fixed, and arguably should not be — a dead-man switch reintroduces the
  failure mode RX-clocked TX exists to avoid.

13. Upstream FRR

Bugs found by running a real dataplane at scale, all reported and merged
separately: the unixc sockaddr length (#22621), the output buffer truncating
registration bursts (#22645), the buffer never drained on shutdown (#22692),
show bfd peers counters tearing down the dplane connection (#22694), echo
interval never negotiated for dataplane sessions (#22805), and the IPv6 echo
source-address regression (#22920).

14. Documentation

docs/ gained evidence directories for each measurement — sweep ladder, tick
ladder, starved detection, symmetric ktx, wedged ktx, dp-fuzz, netns rig, v6
echo — each with the captures behind its numbers.

Seven documents were corrected for the two wrong claims above (06aa950,
2eed3b8, 802aa99, a7e2583, 2568503). Two counts in prose were found
stale in the same pass, which is why tests/README.md now points at the
X-macro rather than repeating a slot count.

---

Deliberately not done

• Symmetric deployment (04) is parked. Its ping-pong premise is now
  measured and holds, but the proposed pacing gate is an architecture change.
  Worth separating: §3.5's argument that the gate is a TX-rate governor
  applies to the current asymmetric deployment too, since an on-link
  attacker who reads my_disc off our traffic can make the engine bounce at
  line rate. That is not addressed here.
• A ringbuf consumer in engine mode. The kernel detector's verdict reaches
  nobody. Wiring it is a behaviour change — a new path to Down — and belongs
  in its own commit with its own evidence.
• _Static_assert as the answer to plane divergence. It shipped, but the
  earlier pushback stands and is recorded: both planes include the same header
  from the same tree, so the failure that actually bites is a stale loaded
  object against rebuilt userspace, which no compile-time assert can see. A
  version field checked at attach time is still the fix.
• The root README refresh, which is being done separately in one pass.

Known gaps

• The multi-interface attach is structurally exercised but not traffic-proven:
  validation used a veth carrying no live session.
• echo_lost is a running total that never decrements, so it should not be
  read as a current loss figure.
• The stats snapshot reports a single xdp_ifindex, which names the
  --kernel-tx interface rather than everything the program now covers.

Testing

All four unit suites, the injection matrix against the live mesh, dp_hold
with a peer-side witness, poll_final, the namespace rigs and the container
scenarios. CI green on all five jobs.