fix: remote connectivity — USB tether TX, bus-state failover, WireGuard re-establishment, coord deadline - #156
Conversation
…transmits tinyusb_net_send_sync() defers can_xmit+xmit onto the TinyUSB task and waits on an event group. With timeout 0 the caller reclaims its own packet before the equal-priority TinyUSB task can run it, so the frame is silently not sent unless that task wins a microsecond race from the other core. Bench (fresh ESP32-S3, v1.3): 0/20 pings over the tether, 17 frames sent vs 411 expired vs 979 busy retries in 150 s. With a 20 ms wait: 591/600 pings at 4 ms, +1003 frames / 0 expired in 35 s, HTTP 17 ms. Regression from #135.
A 16-slot ring lost ~1 safety frame per minute on a remote behind a dock hub chain (bench 2026-09-19: 252 ring-full drops + 55 expiries in 4 h, each one a pstop frame that then only arrived via the DERP mirror >1 s late and was rejected as out-of-order). The disco probe engine emits bursts of >16 frames toward the tailnet peers; a pstop frame caught in the burst was dropped while the endpoint drained. 64 slots (96 KB PSRAM) absorb the burst; 400 ms lifetime rides through host scheduling hiccups and is still well inside the 1.6 s stop-on-silence budget.
… MAC - esp_tinyusb device events: attached/resumed -> netif link up, detached/suspended -> link down. Until now the synthetic 'connected' at start was never undone, so a host that de-enumerated us or suspended the bus left a dead tether ranked as the best uplink and the supervisor never failed over (bench 2026-09-18: port de-authorised, device kept its lease, no WiFi for minutes). Enables CONFIG_TINYUSB_SUSPEND/RESUME_CALLBACK. - The MAC given to tinyusb_net_init() is the HOST's (CDC-NCM iMACAddress); the lwIP netif now uses a distinct locally-administered address instead of the same one (spec violation that worked on Linux by accident).
dcs_wifi_set_enabled() reaches flash (wifi_list NVS blob, PHY calibration NVS) and the supervisor's stack lives in PSRAM, so the first time the failover path ever ran — tether link-down from the previous commit — it died in spi_flash_disable_interrupts_caches_and_other_cpu's esp_task_stack_is_sane_cache_disabled assert (coredump: net_sup -> dcs_wifi_set_enabled -> ml_config_get_wifi_list -> nvs_get_blob). The toggle now runs on a transient 4 KB internal-stack worker; the supervisor waits on a dedicated semaphore so its link-down task notification is not consumed. Any Ethernet unit losing Eth+USB would have hit the same abort.
… source while DERP-only Bench 2026-09-19: a USB-tethered remote sat DERP-only for 7+ minutes with no valid keypair for the machine host — its handshake initiations went via the relay only and never completed — while the host pinged it DIRECTLY every few seconds (disco both ways on the tether). One host-initiated handshake healed it instantly; an idle wireguard-go never initiates on its own, so the remote must be able to re-establish the session by itself. Now the source of the most recent direct disco ping/pong from a safety peer is kept as a handshake candidate (60 s freshness). While the peer has no direct WG endpoint, each initiation is sent both via DERP and to that candidate: one handshake, two legs, either response completes it, and a direct response re-adopts the direct endpoint (update_peer_addr). Data frames are unchanged. Diag: wg_hs_cand_sends in the monitor.
…ery diagnostics A bare WireGuard initiation never re-adds a peer that the far side's wireguard-go has trimmed for inactivity — only an inbound disco PING does (Tailscale lazy peer configuration; already noted in ml_wg_mgr.c). So the second handshake leg alone cannot recover a remote that stopped pinging its machine host: disco_send_ping_to_peer() now also fans the ping out to the handshake candidate (the peer's last direct disco source) for safety peers, which keeps the host's peer configured and yields the pong the normal direct regain needs even when the candidate table lacks it. /admin/api/peers gains wg_up, eps, best, hs_cand, ping_age_ms, backoff_ms per peer so the next stuck-DERP-only event is attributable from a snapshot; monitor gains disco_hs_cand_pings.
…nects with retries The pong-promotion path stores a regained/first direct endpoint with wireguardif_update_endpoint() (connect_ip only) and calls wireguardif_connect() — which re-points the LIVE endpoint peer->ip and retries the handshake — only when some keypair is still valid. Without one it fell to a deliberate one-shot (then active=false, 'wait for them to initiate'), or to nothing at all for an already-known peer. Right for bulk peers that may have trimmed us; a deadlock for the machine host, whose wireguard-go initiates only when it has data — and it has nothing to send while we are silent. The demote's connect_derp() had left peer->ip at 0.0.0.0, so the remote kept initiating relay-only while disco reported direct: indefinitely with the relay dead or slow. A stale-but- valid keypair took the other branch and recovered in seconds — a lottery (bench 2026-09-19: 10 s, 21 s, 169 s, never). Safety peers now connect() with retries in that branch. Diag: wg_safety_reconnects.
Pongs arrive several times a second and wireguardif_connect() resets the pending handshake state; without pacing a slow path could keep answering an already-superseded initiation. One connect per 5 s per peer.
…rite Review: microlink_get_peer_info() left wg_active/wg_send_hs/wg_hs_pending/ wg_init_age_ms unset when there was no WG peer to read; the candidate tuple is written from the disco task and read on the TCPIP thread — write port last (0 first) so a reader seeing a non-zero port sees the matching ip. Lock-free scalar reads for telemetry documented as deliberate.
coord_recv() bounded a partial length-prefixed read by a retry COUNT (300), but every retry first blocks for the socket's SO_RCVTIMEO (2 s in long-poll, 60 s during the MapResponse fetch), so a stalled-but-open peer could hold the coord task for minutes and starve the 120 s control watchdog that only runs between reads. One frame now has 10 s of wall clock (1/12 of the watchdog, >= 4 socket timeouts; progress does not extend it — a trickle is a stall too). On expiry the read fails with ETIMEDOUT, not EAGAIN, so noise_recv/poll_map_update reconnect instead of resuming a misaligned stream; an orderly EOF mid-frame is ECONNRESET. noise_recv's own 300-count payload loop gets the same budget. Host test (88 checks, ASAN/UBSAN) extracts the real loop bodies and drives them with a fake clock: header stall -> ETIMEDOUT at 10 s; slow-but-progressing payload -> success; EOF; EAGAIN interleave; attribution matrix.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a WireGuard handshake "second leg": each peer now records the source of its most recent direct disco packet (hs_cand_ip/hs_cand_port/hs_cand_ms, set via wireguardif_set_hs_candidate), and while a peer is DERP-only wireguardif_peer_output mirrors handshake initiations to that direct candidate as well as the relay, counting them in wireguardif_hs_cand_sends and treating a successful direct send as success even if the relay leg fails — so an idle DERP-only peer that never initiates can still be healed and re-adopt its direct endpoint. Exposes per-peer path-recovery diagnostics through microlink_get_peer_info (endpoint count, chosen endpoint, handshake candidate, ping/init ages, backoff, and WG session/handshake flags) and surfaces them in the /peers HTTP response. The WG-owned scalars are read without the core lock on purpose, accepting a stale value in a single telemetry sample to avoid stalling the safety loop.
Worth a look
- Handshake initiation mirrored to unvalidated disco-source address (spoofable off-path leg) —
components/microlink/components/wireguard_lwip/src/wireguardif.c:181· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Direct disco ping can steer WireGuard handshake candidate endpoint —
components/microlink/src/ml_wg_mgr.c:2934· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Handshake candidate check/use is not atomic —
components/microlink/components/wireguard_lwip/src/wireguardif.c:184· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Payload deadline is not enforced while coord_recv blocks —
components/microlink/src/ml_coord.c:258· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Partial-frame deadline does not bound blocking recv —
components/microlink/src/ml_coord.c:184· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 464 functions depend on the 411 functions this change touches.
Health — this change adds coupling hotspots:
- new:
disco_send_ping_to_peer()— 8 callers, 8 callees - new:
dcs_support_init()— 2 callers, 30 callees - new:
add_peer()— 2 callers, 22 callees - new:
wireguardif_network_rx()— 3 callers, 11 callees - new:
disco_send_call_me_maybe()— 6 callers, 4 callees - new:
noise_send()— 7 callers, 3 callees - new:
neg_apply_target()— 2 callers, 10 callees - new:
microlink_init()— 1 callers, 17 callees - …and 43 more — each is listed as a finding
Verification — 464 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 463 function(s) in the blast radius were not formally verified this run
· 51 more finding(s) on lines outside this diff (see the check run).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large, safety-critical change touching WireGuard handshake re-establishment, USB tether link-state failover, and coordination-socket timeout handling, a human look would still be worthwhile.
- Checked the coord_recv wall-clock deadline replacing the retry-count guard, including errno normalization (EAGAIN vs ECONNRESET/ETIMEDOUT) for misaligned-stream detection.
- Checked the new handshake-candidate mirroring in wireguardif.c/ml_wg_mgr.c and the rate-limited safety-peer reconnect path.
- Checked the USB tether TinyUSB attach/detach/suspend/resume handler driving netif link state, and the TX ring size/TTL increase in ml_usb_tx.c.
- Reviewed the 3 ruled-out candidates (safety-peer reconnect pong race, coord deadline sizing vs. frame size, USB suspend debounce) and found their reasoning consistent with the code.
Extended reasoning...
Overview
This PR spans 14 files across the MicroLink WireGuard-over-lwIP stack, the USB device-tether TinyUSB integration, and firmware/machn sdkconfig defaults. It changes: (1) ml_coord.c's partial-frame receive logic from a retry-count bound to a wall-clock deadline with errno normalization; (2) wireguardif.c/.h and wireguard.h to add a per-peer handshake-candidate address so INITIATIONS can be mirrored off DERP; (3) ml_wg_mgr.c to wire up candidate tracking, extra disco pings, and a rate-limited safety-peer WireGuard reconnect; (4) microlink.c/.h and ml_config_httpd.c to expose new peer/link diagnostics via a lock-free read of wireguard_device fields; (5) ml_dev_tether.c to mirror TinyUSB bus events onto tether netif link state and change the device MAC; (6) ml_usb_tx.c to enlarge the TX ring and TTL.
Security risks
No injection, auth-bypass, or credential-exposure concerns were evident. The lock-free diagnostic read of wireguard peer state in microlink.c is explicitly reasoned about in the code/PR as safe-enough for diagnostics-only use (stale/torn reads only affect an HTTP monitor endpoint, not the data path), which is a reasonable tradeoff for a read-only admin API.
Level of scrutiny
This warrants a high level of scrutiny: it touches the safety-relevant tether/mesh link used for protective-stop signaling (failover behavior, WireGuard re-establishment, USB TX reliability for "pstop" frames), and several of the changes are timing-sensitive (deadlines, debounce-free bus-event handling, rate-limited reconnect vs. pong-driven state resets). The PR description cites a 3.7h soak and a deterministic repro, which is good evidence, but the changes are complex enough — and safety-adjacent enough — that independent human review of the timing interactions (e.g., the ruled-out pong-vs-reconnect race, the stacked coord/payload deadlines, and the no-debounce USB suspend/resume) is warranted before merge.
Other factors
The automated bug-hunting run completed all budgeted rounds and reported no bugs; three related candidate issues were investigated and explicitly ruled out (each is a plausible-sounding but not-actually-broken interaction). No CLAUDE.md conventions apply to the changed paths. Given the size, safety-criticality, and the fact that this consolidates five prior PRs' worth of changes into 12 commits, deferring to a human is the appropriate outcome even with zero automated findings.
rajasimman-madhivanan
left a comment
rajasimman-madhivanan
left a comment
There was a problem hiding this comment.
And no test updates following these changes?
| errno = ETIMEDOUT; | ||
| return -1; | ||
| } | ||
| int n = ml_recv(ml->coord_sock, buf + recvd, len - recvd, 0); |
There was a problem hiding this comment.
This can block for 60s and so coord_recv can block for more than 60s even though the timeout expected is 10s. Is this okay?
There was a problem hiding this comment.
Correct as written — and the same held one call deeper: after the 3-byte Noise header the payload read's first byte still waited a full SO_RCVTIMEO. Fixed in 0625bc7: once a frame is partial, every wait is bounded by the remaining 10 s budget via select() (the file's existing ml_select_fds idiom), never by SO_RCVTIMEO; noise_recv enters the payload read mid-frame so it is bounded from the first byte. Worst-case hold of the coord task drops from budget + 60 s (≈70 s) to the 10 s budget. The policy is extracted into ml_coord_frame_budget.h with a host test (host/test_coord_frame_budget.c, 18 checks) that includes the #127 regression: 300 stalled reads cost 10 s total, not 300 × SO_RCVTIMEO.
| // Written from the disco task, read on the TCPIP thread (peer_output). | ||
| // Order the writes so a reader that sees a non-zero port also sees the | ||
| // matching ip/ms; a reader racing the update at worst skips one leg. | ||
| peer->hs_cand_port = 0; |
There was a problem hiding this comment.
ordinary field assignments do not guarantee cross-core memory ordering in C. The compiler or CPU may expose those writes to the other core in a different order. May be a todo here just in case this var is accessed from another thread concurrently?
There was a problem hiding this comment.
Agreed — plain stores give no cross-core ordering guarantee, and the comment was promising one. Rather than a TODO, 0625bc7 uses the file's existing idiom for exactly this case (wireguardif_core_lock_needed() + LOCK_TCPIP_CORE(), as at netif_set_link_up/down): the setter and getter update/read the whole hs_cand_{ip,port,ms} record under the core lock, and the reader in peer_output runs on the TCPIP thread which already holds it. The lock query is holder-aware, so a caller already holding the lock does not re-take it.
|
Re "no test updates?": the repo's unit-test surface is the host harness (
Both roles build; |
…oord waits, regain policy + host tests
- wireguardif: hs_cand_{ip,port,ms} is updated and read under LOCK_TCPIP_CORE
(the file's existing idiom) instead of relying on store order, which plain
C assignments do not guarantee across cores.
- coord_recv: once a frame is partial, each wait is bounded by the REMAINING
10 s budget via select(), never by SO_RCVTIMEO (2 s / 60 s); the Noise
payload read enters mid-frame so its first byte is bounded too. Worst-case
hold of the coord task drops from budget + 60 s to the budget.
- ml_wg_mgr: the regain decision goes through ml_wg_regain_action(); the
safety branch now marks tried_initial_handshake, so a pong inside the 5 s
pace window no longer falls into the bulk one-shot (second connect() 200 ms
later + active=false cancelling the retries just armed).
- host tests: test_wg_regain_policy (11 checks), test_coord_frame_budget
(18 checks, incl. the #127 300-retry regression); wired into make -C host test.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a WireGuard handshake "second leg": wireguardif_peer_output mirrors handshake initiations to the most recent direct disco source of a DERP-only peer (wireguardif_set_hs_candidate/wireguardif_get_hs_candidate, stored per-peer and read under the TCPIP core lock), so an idle peer that never initiates can still be healed direct while the relay leg is slow or dead, counted in wireguardif_hs_cand_sends. Bounds one partial coordination frame to a 10 s wall-clock budget via the new ml_coord_frame_budget helper so a trickling-but-open peer can no longer pin the coord task for up to 10 minutes past the control watchdog, arming on the first byte and capping each blocking wait to the remaining budget. Expands microlink_peer_info_t with path-recovery diagnostics (endpoint count, chosen and handshake-candidate endpoints, ping/init ages, backoff, and WG session/handshake state) and paces keyless-regain wireguardif_connect() calls to REKEY_TIMEOUT via last_safety_connect_ms.
Worth a look
- HTTP telemetry reads WireGuard peer state without TCPIP/core synchronization —
components/microlink/src/microlink.c:880· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- WireGuard peer index is bounds-checked only after being used —
components/microlink/src/microlink.c:866· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- TCPIP core lock does not cover peer lookup in cross-thread candidate setter —
components/microlink/components/wireguard_lwip/src/wireguardif.c:1050· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Public peer info struct size/layout changed —
components/microlink/include/microlink.h:62· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- USB detach/suspend events are dropped when netif is not yet available —
components/ml_dev_tether/src/ml_dev_tether.c:145· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 497 functions depend on the 444 functions this change touches.
Health — this change adds coupling hotspots:
- new:
disco_send_ping_to_peer()— 8 callers, 8 callees - new:
dcs_support_init()— 2 callers, 30 callees - new:
add_peer()— 2 callers, 22 callees - new:
wireguardif_network_rx()— 3 callers, 11 callees - new:
disco_send_call_me_maybe()— 6 callers, 4 callees - new:
noise_send()— 7 callers, 3 callees - new:
neg_apply_target()— 2 callers, 10 callees - new:
microlink_init()— 1 callers, 17 callees - …and 43 more — each is listed as a finding
Verification — 497 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 496 function(s) in the blast radius were not formally verified this run
· 51 more finding(s) on lines outside this diff (see the check run).
…ookup, bus-seeded tether link - coord frame budget: a slow-but-live frame is no longer cut at 10 s. Two limits from the first byte: a 10 s no-progress window restarted by every byte (the #127 stall still costs 10 s) and a hard cap scaled by frame size, max(10 s, len / 1 KiB/s) — a 64 KB MapResponse frame gets ~64 s, so a link sustaining ~8 kbit/s completes instead of reconnecting. Host test: 27 checks incl. 64 KB at 1 KiB/s completes, 512 B/s trickle cut at the cap, mid-frame stall abandoned 10 s after the last byte. - wireguardif hs-candidate set/get: the core lock now also covers the peer slot lookup; HS_CAND_FRESH_MS exported. - microlink_get_peer_info: reads the candidate lock-free like its neighbours (documented telemetry snapshot; the locked getter is for the disco task). - tether: the netif's initial link state is seeded from the bus (tud_mounted && !tud_suspended) under s_rx_lock, so a mount/suspend event that fired before the netif existed is not lost.
|
Graphify advisories on
Both roles build; |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a handshake "second leg" so DERP-only safety peers can heal: wireguardif_set_hs_candidate/wireguardif_get_hs_candidate record the source of the most recent direct disco packet (under the TCPIP core lock, stale after HS_CAND_FRESH_MS), and wireguardif_peer_output mirrors handshake initiations to that direct candidate alongside the relay, treating the send as OK if either leg succeeds while counting mirrored sends in wireguardif_hs_cand_sends. Replaces the retry-count bound on partial coordination-frame reads with a wall-clock budget (ml_coord_frame_budget): a 10 s no-progress window restarted on every byte plus a frame-size-scaled hard cap, so a stalled-but-open peer is abandoned in ~10 s instead of holding the coord task for up to 10 minutes and starving the control watchdog. Expands microlink_peer_info_t with path-recovery diagnostics (chosen endpoint, handshake candidate, ping/init ages, WG session flags) and paces keyless-regain wireguardif_connect calls via last_safety_connect_ms.
Worth a look
- s_netif read/write across on_usb_event and ml_dev_tether_stop only partly lock-protected —
components/ml_dev_tether/src/ml_dev_tether.c· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Direct disco packet fields are stored as a handshake candidate without endpoint validation —
components/microlink/src/ml_wg_mgr.c:2936· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- WiFi toggle helper uses shared request/semaphore while supervisor can be started multiple times —
firmware/components/dcs_support/src/dcs_net_supervisor.c:290· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Lock-free reads of WireGuard peer state race with TCPIP thread updates —
components/microlink/src/microlink.c:879· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Public peer info struct layout was expanded —
components/microlink/include/microlink.h:65· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 499 functions depend on the 446 functions this change touches.
Health — this change adds coupling hotspots:
- new:
disco_send_ping_to_peer()— 8 callers, 8 callees - new:
dcs_support_init()— 2 callers, 30 callees - new:
add_peer()— 2 callers, 22 callees - new:
wireguardif_network_rx()— 3 callers, 11 callees - new:
disco_send_call_me_maybe()— 6 callers, 4 callees - new:
noise_send()— 7 callers, 3 callees - new:
neg_apply_target()— 2 callers, 10 callees - new:
microlink_init()— 1 callers, 17 callees - …and 45 more — each is listed as a finding
Verification — 499 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 498 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below; 51 more finding(s) on lines outside this diff (see the check run).
| * MapResponse). A stalled peer holds the coord task — and delays the | ||
| * ML_CTRL_WATCHDOG_MS check that runs between reads — for at most 10 s; a slow | ||
| * but live MapResponse frame keeps going as long as bytes arrive. */ | ||
| static int coord_recv_ex(microlink_t * ml, uint8_t * buf, size_t len, bool mid_frame) |
There was a problem hiding this comment.
coord_recv_ex()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
|
||
| #define S(x) ((int64_t)(x) * 1000000LL) /* seconds -> us */ | ||
|
|
||
| int main(void) |
There was a problem hiding this comment.
main()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
components/microlink/src/ml_wg_mgr.c— A safety peer that lands in a reused peer-table slot can have its first direct-regain wireguardif_connect() delayed up to 5s by a stale timestamp left over from that slot's previous occupant. The!existinginit block resets sibling stale-stamp fields (direct_backoff_until, relay_refetch_next_ms, last_cmm_sent_ms) precisely to avoid an evicted slot's leftover stamp suppressing the new occupant, but the new last_safety_connect_ms field (ml_wg_mgr.c:617, set at ml_wg_mgr.c:3183) was never added to that list. Fix: also zero p->last_safety_connect_ms in the!existingblock (ml_wg_mgr.c:2320-2345) so a fresh peer's regain pacing in ml_wg_regain_action() never reads a predecessor's connect time.Extended reasoning...
Peer table slots are reused without a memset: eviction only sets ml->peers[evict_idx].active=false (ml_wg_mgr.c ~2176), and the re-key retire path does the same (~2095). The
!existingblock at 2320 resets many per-peer timers explicitly BECAUSE stale values from the evicted occupant would misbehave (see its own comments for direct_backoff_until and relay_refetch_next_ms), but last_safety_connect_ms is absent from that list. Sequence: peer A occupies slot X, becomes health-tracked (is_safety_peer true), gets a direct pong, and process_disco_pong sets p->last_safety_connect_ms=T0 and calls wireguardif_connect (ml_wg_mgr.c:3181-3187). A then unbonds (is_health_tracked false) and, before T0+5000ms, is evicted from slot X to make room for a new pinned peer B. B is newly added via the!existingpath, inheriting slot X's last_safety_connect_ms=T0 untouched. If B is itself a safety peer and gets its own first direct pong before T0+5000ms, ml_wg_regain_action(true, ..., now, T0) computes now-T0 < ML_WG_SAFETY_CONNECT_PACE_MS and returns ML_WG_REGAIN_NONE instead of…Verification: normal (bounded). The new field p->last_safety_connect_ms (microlink_internal.h:617, set only at ml_wg_mgr.c:3183 for safety peers) is a stale-stamp field just like its siblings, but it is NOT reset in the
!existingslot-reuse init block. That block at ml_wg_mgr.c:2320-2346 deliberately zeroes direct_backoff_until (2335), relay_refetch_next_ms (2342) and last_cmm_sent_ms (2345), each with a…
…ce started, idempotent supervisor start - ml_wg_mgr: the !existing slot-reuse block also resets last_safety_connect_ms and tried_initial_handshake — a predecessor's stamp paced a fresh safety peer's first connect() by up to 5 s, and its one-shot flag would deny a fresh bulk peer its single initiation. - tether: the netif is created, started and published under s_rx_lock, then its link state is seeded from the bus; on_usb_event()/on_usb_rx() can no longer observe an un-started netif, and the seed is ordered with events. - dcs_net_supervisor_start() is idempotent (single instance by construction).
|
Round on claude[bot] 🟡 (outside diff) — Graphify advisories:
|
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 4 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a WireGuard handshake "second leg" for DERP-only safety peers: wireguardif_peer_output mirrors handshake initiations to the most recent direct disco source (hs_cand_ip/port/ms, fresh within HS_CAND_FRESH_MS), so an idle remote that never initiates can be healed by a host-driven handshake, and a direct response re-adopts the direct endpoint; the candidate is set/read under the TCPIP core lock via wireguardif_set_hs_candidate/wireguardif_get_hs_candidate and mirrored sends counted in wireguardif_hs_cand_sends. Replaces the retry-count frame timeout in coord_recv_ex with a wall-clock budget (ml_coord_frame_budget_init): a 10 s no-progress window that resets on each byte plus a frame-size-scaled hard cap, so a stalled-but-open peer is abandoned in ~10 s instead of holding the coord task for up to 10 min and starving the control watchdog. Expands microlink_peer_info_t and microlink_get_peer_info with path-recovery diagnostics (endpoint count, chosen/candidate endpoints, ping/init ages, backoff, WG session/handshake flags) and paces keyless-regain wireguardif_connect calls via last_safety_connect_ms.
Worth a look
- Timeout retry can duplicate USB frames —
components/ml_dev_tether/src/ml_usb_tx.c:103· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- tinyusb_net_send_sync timeout is converted from ms to ticks —
components/ml_dev_tether/src/ml_usb_tx.c:102· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- coord_sock is re-read across select/recv without synchronization —
components/microlink/src/ml_coord.c:190· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Lock-free reads of TCPIP-owned WireGuard peer state introduce data races —
components/microlink/src/microlink.c:876· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 499 functions depend on the 446 functions this change touches.
Health — this change adds coupling hotspots:
- new:
disco_send_ping_to_peer()— 8 callers, 8 callees - new:
dcs_support_init()— 2 callers, 30 callees - new:
add_peer()— 2 callers, 22 callees - new:
wireguardif_network_rx()— 3 callers, 11 callees - new:
disco_send_call_me_maybe()— 6 callers, 4 callees - new:
noise_send()— 7 callers, 3 callees - new:
neg_apply_target()— 2 callers, 10 callees - new:
microlink_init()— 1 callers, 17 callees - …and 45 more — each is listed as a finding
Verification — 499 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 498 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below; 51 more finding(s) on lines outside this diff (see the check run).
| * MapResponse). A stalled peer holds the coord task — and delays the | ||
| * ML_CTRL_WATCHDOG_MS check that runs between reads — for at most 10 s; a slow | ||
| * but live MapResponse frame keeps going as long as bytes arrive. */ | ||
| static int coord_recv_ex(microlink_t * ml, uint8_t * buf, size_t len, bool mid_frame) |
There was a problem hiding this comment.
coord_recv_ex()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
|
||
| #define S(x) ((int64_t)(x) * 1000000LL) /* seconds -> us */ | ||
|
|
||
| int main(void) |
There was a problem hiding this comment.
main()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because this round touches WireGuard peer-state re-establishment, USB tether netif/link-state ordering, and the coordination frame read path — all safety- and security-sensitive — a human look would still be worthwhile.
- Confirmed the coord frame hard cap (
ml_coord_frame_budget.h) now scales with frame length viaml_coord_frame_budget_total_ms(len)rather than a flat 10 s, so a large MapResponse read gets a proportional budget instead of forcing a reconnect. - Checked round 5's slot-reuse resets in
ml_wg_mgr.cadd_peer(), the tether netif create/start/publish-under-s_rx_lockreordering inml_dev_tether.c, and thedcs_net_supervisor_start()idempotency guard — each is small and consistent with the file's existing patterns.
Extended reasoning...
Overview
This run's diff (9d3b0e435..HEAD) covers a 15-commit series hardening the microlink coordination/WireGuard/USB-tether stack: a wall-clock frame-read budget replacing a fixed retry-count timeout in ml_coord.c (issue #127), a WireGuard direct-path regain policy for keyless peers in ml_wg_mgr.c, USB bus-state-to-netif-link mirroring in ml_dev_tether.c/ml_usb_tx.c, and a WiFi-failover flash-access fix plus idempotent start in dcs_net_supervisor.c. Two new host-testable headers (ml_coord_frame_budget.h, ml_wg_regain_policy.h) ship with unit tests. This is round 5 of a PR I've reviewed on 3 prior pushed versions.
Security risks
No injection, auth-bypass, or data-exposure concerns. The security-relevant surface is WireGuard handshake/keypair state management (ml_wg_mgr.c, wireguardif.c) and the Noise-frame read path (ml_coord.c) — a bug here could either weaken the peer-liveness/handshake logic or (as my own prior review on this PR flagged) cause an availability regression by tearing down connections that were merely slow rather than dead. I independently re-verified in this turn that the specific concern I raised earlier — a flat 10 s hard cap applied uniformly to up to 64 KB Noise frames — has been fixed: coord_recv_ex now calls ml_coord_frame_budget_init(&budget, len) with the caller's frame length, and the cap is max(10s, len/1KiB/s), backed by host/test_coord_frame_budget.c assertions for the 64 KB case. I did not rely on the thread's "resolved" marker alone (per the self-resolution caveat) — I read the current code to confirm the fix.
Level of scrutiny
High. This PR touches crypto-adjacent peer/session state, network failover ordering, and a control-plane read loop whose timing directly affects whether a live connection is torn down — exactly the kind of change where a subtle timing or state-reset bug could cause either indefinite hangs or spurious reconnects in production, and where a human reviewer with domain knowledge of WireGuard/lwIP internals is warranted regardless of automated review outcome.
Other factors
The round-5 commit (07e5d0586a06) is small and self-contained: three files, ~33 lines net, each fix (peer-slot-reuse state reset, netif publish-after-start ordering, supervisor idempotency) is a narrow, well-commented correction consistent with the surrounding code's existing patterns (e.g., the same slot-reuse reset idiom already used for direct_backoff_until, relay_refetch_interval_ms, etc. in add_peer()). The PR carries substantial test coverage (host-testable budget/policy headers with dedicated unit tests) and a described 3.7 h soak test. Since round 4/5 commits substantively addressed the concern from my last review, I'm posting an updated review rather than staying silent, but per the approval guidelines I should not approve a change of this scope and sensitivity — a human should still look, particularly at any open third-party review threads on the coordination/frame-budget code I could not independently inspect for content (only metadata was visible to me).
8 h steady-state soak (run 4) — evidence for this PRSetup. 2026-09-19 18:50 → 09-20 03:05 PDT, 8 h 15 min, no E-stop cycling. Image = Whole-run table
( Result. Zero resets, zero Ethernet recoveries, zero timeout-class lockstep mismatches after boot, ~148 000 heartbeats per unit all answered; internal heap flat for 8 h. Two machine STOPs, both external to the firmware under test and both recovered autonomously:
Tether TX path (this PR's #149/#152/#151 fixes): eed0 Watch item, not a regression: eed0 internal heap runs at ~11 KB free / 4 KB largest block (min 6.7 KB this run), Current head |
| * alignment is permanently lost, so it runs mid_frame: the wall-clock budget | ||
| * is armed before the first payload byte and bounds every wait. Past it, | ||
| * ETIMEDOUT makes the caller reconnect instead of resuming misaligned. */ | ||
| if (coord_recv_ex(ml, ciphertext, ct_len, true) < 0) { |
There was a problem hiding this comment.
Minor Issues:
- Payload uses coord_recv_ex while header uses coord_recv
- Header and payload now get two different timeouts
There was a problem hiding this comment.
Both points fixed in 99f57085: the budget is now caller-owned and noise_recv uses one for the whole frame — it arms on the header's first byte, and once the header says how long the payload is, ml_coord_frame_budget_set_frame_len() re-sizes the hard cap for header + payload measured from that same first byte (never shrinking); the no-progress window carries across the two reads. coord_recv() keeps a private budget for single-read frames, so there is a single code path (coord_recv_budget) and no mid_frame flag any more. Host test extended to 34 checks, including 'cap runs from the header's first byte, not from the re-size'.
noise_recv read the 3-byte header and the payload under two independent budgets (header: 10 s floor; payload: its own size-scaled cap armed separately). The budget is now caller-owned: it arms on the header's first byte, is re-sized for header + payload once the header says how long the payload is (ml_coord_frame_budget_set_frame_len, measured from that same first byte, never shrinking), and the same no-progress window carries across the two reads. coord_recv() keeps a private budget for single-read frames. Host test: 34 checks.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a WireGuard handshake "second leg" that mirrors handshake initiations to a peer's most recent direct disco source while the peer is DERP-only, letting an idle-but-directly-reachable peer complete a handshake when the relay leg is dead; the candidate endpoint is stored per-peer with a 60 s freshness window and read/written under the TCPIP core lock, and a direct response re-adopts the direct endpoint. Replaces the coordination frame's retry-count bound with a wall-clock budget (ml_coord_frame_budget): a 10 s no-progress window that every received byte restarts plus a frame-size-scaled hard cap (max(10 s, frame_len/1 KiB/s)), so a stalled-but-open peer is abandoned in ~10 s instead of holding the coord task for up to 10 min and starving the control watchdog. Surfaces path-recovery diagnostics on microlink_peer_info_t (chosen endpoint, handshake candidate, ping/init ages, backoff, WG session/handshake flags) and paces keyless-regain wireguardif_connect calls to REKEY_TIMEOUT, and wires up new host tests for the frame budget and WG regain policy.
Worth a look
- wg_peer_index can race to -1 and index before peers array —
components/microlink/src/microlink.c:875· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Disco ping can steer WireGuard candidate traffic to arbitrary IP/port —
components/microlink/src/ml_wg_mgr.c:2941· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Handshake packets can be reflected to unvalidated candidate endpoints —
components/microlink/components/wireguard_lwip/src/wireguardif.c:184· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Noise frame ciphertext length not validated before allocation/read —
components/microlink/src/ml_coord.c:284· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Safety-peer connect uses stale endpoint (candidate stored but peer->ip not updated to pong source) —
components/microlink/src/ml_wg_mgr.c:3186· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 500 functions depend on the 447 functions this change touches.
Health — this change adds coupling hotspots:
- new:
disco_send_ping_to_peer()— 8 callers, 8 callees - new:
dcs_support_init()— 2 callers, 30 callees - new:
add_peer()— 2 callers, 22 callees - new:
wireguardif_network_rx()— 3 callers, 11 callees - new:
disco_send_call_me_maybe()— 6 callers, 4 callees - new:
noise_send()— 7 callers, 3 callees - new:
neg_apply_target()— 2 callers, 10 callees - new:
noise_recv()— 4 callers, 5 callees - …and 44 more — each is listed as a finding
Verification — 500 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 499 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify wireguardif\_peer\_output.
The verifier did not have enough to check wireguardif\_peer\_output, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: err_t
Could not verify: Could not verify microlink\_get\_peer\_info.
The verifier did not have enough to check microlink\_get\_peer\_info, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: esp_err_t
Could not verify: Could not verify handler\_get\_peers.
The verifier did not have enough to check handler\_get\_peers, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: esp_err_t
Could not verify: Could not verify handler\_monitor.
The verifier did not have enough to check handler\_monitor, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: esp_err_t
Could not verify: Could not verify coord\_recv.
The verifier did not have enough to check coord\_recv, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported parameter type(s)
Could not verify: Could not verify noise\_recv.
The verifier did not have enough to check noise\_recv, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported parameter type(s)
Could not verify: Could not verify add\_peer.
The verifier did not have enough to check add\_peer, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported parameter type(s)
Could not verify: Could not verify disco\_send\_ping\_to\_peer.
The verifier did not have enough to check disco\_send\_ping\_to\_peer, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify process\_disco\_ping.
The verifier did not have enough to check process\_disco\_ping, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify process\_disco\_pong.
The verifier did not have enough to check process\_disco\_pong, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify ml\_dev\_tether\_try\_start.
The verifier did not have enough to check ml\_dev\_tether\_try\_start, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: esp_err_t
Could not verify: Could not verify usb\_drain.
The verifier did not have enough to check usb\_drain, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported parameter type(s)
Could not verify: Could not verify dcs\_net\_supervisor\_start.
The verifier did not have enough to check dcs\_net\_supervisor\_start, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify supervisor\_task.
The verifier did not have enough to check supervisor\_task, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
· 2 grounded finding(s) anchored inline below; 50 more finding(s) on lines outside this diff (see the check run).
| @@ -209,9 +259,14 @@ static int noise_send(microlink_t * ml, ml_noise_state_t * noise, const uint8_t | |||
| /* Receive and decrypt a Noise transport frame, returns plaintext length */ | |||
| static int noise_recv(microlink_t * ml, ml_noise_state_t * noise, uint8_t * plaintext, size_t max_len) | |||
There was a problem hiding this comment.
noise_recv()
high coupling complexity (Ca·Ce = 20).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
|
||
| #define S(x) ((int64_t)(x) * 1000000LL) /* seconds -> us */ | ||
|
|
||
| int main(void) |
There was a problem hiding this comment.
main()
fans out to 7 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| if (device->udp_output_fn && !ip_addr_isany(&peer->hs_cand_ip) && peer->hs_cand_port != 0 && | ||
| !wireguard_expired(peer->hs_cand_ms, HS_CAND_FRESH_MS / 1000) && | ||
| wireguard_get_message_type(data, q->tot_len) == MESSAGE_HANDSHAKE_INITIATION) { | ||
| uint32_t cand_ip = ip4_addr_get_u32(ip_2_ip4(&peer->hs_cand_ip)); | ||
| err_t leg2 = device->udp_output_fn(cand_ip, peer->hs_cand_port, data, q->tot_len, device->udp_output_ctx); | ||
| wireguardif_hs_cand_sends++; | ||
| if (result != ERR_OK && leg2 == ERR_OK) { | ||
| result = ERR_OK; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 An attacker who replays one authenticated disco ping with a spoofed source IP makes the device repeatedly send real WireGuard handshake-init packets to that address for 60s, not just the single PONG the base branch reflects. ml_wg_mgr.c:2935-2942 sets peer->hs_cand from pkt->src_ip/src_port on an unvalidated inbound ping, before the pong that would confirm it. wireguardif.c:180-189 then mirrors every paced (5s, ML_WG_SAFETY_CONNECT_PACE_MS) safety-peer handshake initiation there while HS_CAND_FRESH_MS=60s holds. Fix: gate mirroring on a validated pong / has_direct_path, not the raw inbound ping.
Extended reasoning...
Disco pings are nacl_box_open-authenticated (ml_wg_mgr.c:3430) so only the true peer's disco key can produce valid content, but the UDP source IP/port used as pkt->src_ip/src_port is not part of that authenticated payload. An attacker who observes one genuine ping (network tap/MITM) can resend the identical ciphertext with a forged source IP=victim; nacl_box_open still succeeds. Handler at ml_wg_mgr.c:2935 sees !pkt->via_derp, is_safety_peer true, calls wireguardif_set_hs_candidate(peer_index, victim_ip, victim_port) immediately - the very next comment says reachability is only proven later by a pong that flips has_direct_path, which never comes for a spoofed address. Safety-peer regain policy (ml_wg_regain_policy.h) paces handshake retries every ML_WG_SAFETY_CONNECT_PACE_MS=5000ms while DERP-only. Each retry goes through wireguardif_peer_output; since hs_cand is still fresh (HS_CAND_FRESH_MS=60000), wireguardif.c:180-189 sends a second copy via udp_output_fn straight to victim_ip:victim_port. Over the 60s window that is up to ~12 reflected packets from one injected ping, versus the…
Verification: Severity: normal (security-relevant; newly introduced by this change, modest amplification, gated by an outage window + on-path capture). The mechanism is real and reachable. Disco pings are authenticated only over the nacl box ciphertext (ml_wg_mgr.c:3430 nacl_box_open uses sender_disco_key + ml->disco_private_key); the UDP transport 5-tuple (pkt->src_ip/src_port) is NOT covered by that box,…
| if (ml->wg_netif != NULL && p->wg_peer_index >= 0) { | ||
| struct netif * netif = (struct netif *)ml->wg_netif; | ||
| ip_addr_t cur_ip; | ||
| u16_t cur_port; | ||
| info->wg_up = (wireguardif_peer_is_up(netif, (u8_t)p->wg_peer_index, &cur_ip, &cur_port) == ERR_OK); | ||
| /* Diagnostic snapshot of plain scalars owned by the TCPIP thread, read | ||
| * without the core lock on purpose: a torn read can only yield a stale | ||
| * bool or age for one JSON sample, never a fault, and taking the lock from | ||
| * the HTTP task for telemetry is what stalled the safety loop before. */ | ||
| struct wireguard_device * dev = (struct wireguard_device *)netif->state; | ||
| if (dev != NULL && p->wg_peer_index < WIREGUARD_MAX_PEERS) { | ||
| const struct wireguard_peer * wp = &dev->peers[p->wg_peer_index]; | ||
| info->wg_active = wp->active; | ||
| info->wg_send_hs = wp->send_handshake; | ||
| info->wg_hs_pending = wp->handshake.valid; | ||
| /* Handshake second-leg candidate: same lock-free telemetry snapshot (the | ||
| * locked wireguardif_get_hs_candidate() is for the disco task's decisions, | ||
| * not for the HTTP task — see the comment above). */ | ||
| if ( | ||
| !ip_addr_isany(&wp->hs_cand_ip) && wp->hs_cand_port != 0 && | ||
| !wireguard_expired(wp->hs_cand_ms, HS_CAND_FRESH_MS / 1000)) | ||
| { | ||
| info->hs_cand_ip = lwip_ntohl(ip4_addr_get_u32(ip_2_ip4(&wp->hs_cand_ip))); | ||
| info->hs_cand_port = wp->hs_cand_port; | ||
| } | ||
| uint32_t wg_now = sys_now(); /* wireguardif stamps with lwIP sys_now(), not esp_timer */ | ||
| info->wg_init_age_ms = (wp->last_initiation_tx != 0) ? (wg_now - wp->last_initiation_tx) : 0xFFFFFFFFu; |
There was a problem hiding this comment.
🟡 (optional) After merging, an admin can see /admin/api/peers show a retired or LRU-evicted peer row (active:false) carrying a completely different peer's live WireGuard diagnostics (wg_up, wg_active, hs_cand, wg_init_age_ms) - forensics data this diff adds and that is now wrong for exactly the kind of investigation it exists for. ml_wg_mgr.c's re-key retire (ml_wg_mgr.c:2105-2115) and LRU evict (ml_wg_mgr.c:2172-2176) paths call wireguardif_remove_peer() but clear only p->active, never p->wg_peer_index or p->vpn_ip. wireguardif_add_peer()'s peer_alloc() can hand that freed WG slot to an unrelated peer. microlink_get_peer_info() (microlink.c:866-892) then reads dev->peers[p->wg_peer_index] whenever the stale index is >=0, without checking p->active or matching the slot's key/vpn_ip to this peer. …
Extended reasoning...
…Fix: clear wg_peer_index (and vpn_ip) at retire/evict time, or verify the WG slot still belongs to this peer (matching public_key) before exposing its state.
ml_wg_mgr.c:2105-2115 retires a re-keyed peer: wireguardif_remove_peer(netif, p->wg_peer_index) then only ml->peers[idx].active=false; wg_peer_index and vpn_ip are left as-is. Same gap at the LRU-evict path, ml_wg_mgr.c:2172-2176. wireguardif_remove_peer (wireguardif.c:1219-1228) sets peer->valid=false on that WG slot. A later wireguardif_add_peer() for a DIFFERENT peer calls peer_alloc(), which scans for the first invalid slot and can return this same just-freed index. If ml_wg_mgr.c's own free-slot scan (2120-2125) picks an earlier already-inactive ml->peers[] slot for the new peer instead of idx, the retired entry at idx stays indefinitely with active=false, vpn_ip nonzero, wg_peer_index pointing at the now-reused slot. handler_get_peers (ml_config_httpd.c:701-704) only skips vpn_ip==0, so it emits this row. microlink_get_peer_info tests p->wg_peer_index>=0 (true) and reads dev->peers[index] (microlink.c:875-892):…
Verification: nit. Real but low-severity diagnostic-accuracy defect newly introduced by this diff (the WG diagnostics block at microlink.c:866-893 is added here; the base get_peer_info never touched wg_peer_index). Mechanism (retire path only): ml_wg_mgr.c:2105-2115 removes the WG slot (wireguardif_remove_peer, 2106; the slot is zeroed and valid=false per wireguardif.c:1223-1224) but clears only… | nit. Real…
| if (ml_coord_frame_budget_armed(budget)) { | ||
| /* Mid-frame: wait for readability at most the remaining budget. A 0 result | ||
| * loops back to the expiry check above; data/EOF/error fall through to | ||
| * recv(), which reports them. */ | ||
| int64_t wait_us = ml_coord_frame_budget_wait_us(budget, now_us); | ||
| fd_set rfds; | ||
| FD_ZERO(&rfds); | ||
| FD_SET(ml->coord_sock, &rfds); | ||
| struct timeval tv = {.tv_sec = (time_t)(wait_us / 1000000LL), .tv_usec = (suseconds_t)(wait_us % 1000000LL)}; | ||
| int sel = ml_select_fds(ml->coord_sock + 1, &rfds, NULL, NULL, &tv); | ||
| if (sel == 0) { | ||
| timeouts++; | ||
| continue; | ||
| } | ||
| if (sel < 0 && errno == EINTR) continue; | ||
| } | ||
| int n = ml_recv(ml->coord_sock, buf + recvd, len - recvd, 0); | ||
| if (n <= 0) { | ||
| if (errno == EAGAIN || errno == EWOULDBLOCK) { | ||
| if (recvd == 0) { | ||
| /* No data consumed yet — timeout is fine, caller can retry */ | ||
| return -1; | ||
| } | ||
| /* Partial data consumed — we MUST finish this read or the | ||
| * Noise frame stream will be misaligned. Retry with backoff. */ | ||
| if (++retries > 300) { /* ~3 seconds */ | ||
| ESP_LOGE(TAG, "coord_recv partial timeout: %d/%d bytes", (int)recvd, (int)len); | ||
| if (n < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { | ||
| if (recvd == 0 && !ml_coord_frame_budget_armed(budget)) { | ||
| /* Idle stream, nothing of this frame consumed — caller can retry */ | ||
| return -1; | ||
| } | ||
| /* Partial data consumed — we MUST finish this read or the Noise frame | ||
| * stream will be misaligned. select() said readable but recv() had | ||
| * nothing (spurious wakeup / socket timeout): retry until the budget. */ | ||
| timeouts++; /* outside ESP_LOGW: its args are compiled out below LOG_LOCAL_LEVEL */ | ||
| ESP_LOGW(TAG, "coord_recv socket timeout #%d mid-frame: %d/%d bytes", timeouts, (int)recvd, (int)len); | ||
| vTaskDelay(pdMS_TO_TICKS(10)); | ||
| continue; |
There was a problem hiding this comment.
🟣 coord_recv_budget's select()-based deadline can still be defeated by a blocking recv(), letting a stalled coord read cost up to 60s instead of the documented 10s bound. When select() reports the socket readable but recv() at ml_coord.c:200 finds nothing (the 'spurious wakeup / socket timeout' case the comment at ml_coord.c:207-209 itself calls out), recv() is a plain blocking call (coord_sock has no O_NONBLOCK, unlike ml_derp.c/ml_stun.c which use O_NONBLOCK+select for exactly this reason) so it can only return once data arrives or the socket's own SO_RCVTIMEO elapses -- 60s during MapResponse fetch (ml_coord.c:1636, 1753) or the 60s 'normal' timeout restored at ml_coord.c:581-582. …
Extended reasoning...
…Fix: make coord_sock non-blocking (or pass MSG_DONTWAIT) so recv() after select() never itself blocks past the remaining frame budget, for every coord_recv_budget call site.
coord_sock is created blocking at ml_coord.c:331-341 (SO_RCVTIMEO set, no O_NONBLOCK). Elsewhere the socket's SO_RCVTIMEO is set to 60s for MapResponse fetch (ml_coord.c:1636/1753) and restored to 60s as the steady-state timeout (ml_coord.c:581-582). coord_recv_budget arms a frame budget on the first byte, then each loop iteration bounds the wait with select() to the min of the 10s no-progress window and the size-scaled hard cap (ml_coord.c:184-198). If select() marks the fd readable but the byte isn't actually consumable when recv() runs -- exactly the case ml_coord.c:207-209 names 'spurious wakeup / socket timeout' -- recv() at line 200 blocks using the socket's own SO_RCVTIMEO because the socket was never switched to non-blocking. One such event during a mid-frame MapResponse read stalls the coord task up to 60s, not the <=10s ml_coord_frame_budget.h promises, undermining the ML_CTRL_WATCHDOG_MS protection…
Verification: pre-existing. The mechanism is real and reachable: coord_sock is blocking (ml_coord.c:340 sets SO_RCVTIMEO only, no O_NONBLOCK; steady-state 60s at :582, MapResponse 60s at :1636/:1753). In coord_recv_budget the armed-frame wait is bounded by select() (:188-:197), but control then falls through to the blocking ml_recv() at :200. The n < 0 && (errno==EAGAIN||EWOULDBLOCK) branch at :202 on a…
2 participants
Consolidates #149, #152, #151, #155 and #153 (closed as superseded; their review threads and bot findings were addressed there and the fixes are included here). 12 linear commits, 409+/29−. Every commit was in the build that passed the 3.7 h soak below.
What is fixed
de335e81b5fe82tinyusb_net_send_sync(…, 0)reclaimed the packet before the equal-priority TinyUSB task ran the deferred send — ~95 % of device→host frames silently dropped (regression from #135). Now a 20 ms wait; TIMEOUT-after-send duplicate accepted and documented.3a8a527f9ec4cb9823785dcs_wifi_set_enabledreaches flash from the supervisor's PSRAM stack. Toggle moved to an internal-stack worker.0440ad25b86d21e020c86cae7e9454149ebd7a335f/admin/api/peers.8cf5104coord_recvbounded by a retry count × 2 s socket timeout (≈10 min); now a 10 s wall-clock deadline,ETIMEDOUTso callers reconnect (#127). 88-check host test.Evidence
Not in this PR
#148 (release guard), #150 (name scrub), #154 (lockstep mismatch diagnostics — SIL comparator, reviewed separately). DERP-mirror latency (1.3 s median) and the W5500 RX stall are follow-ups.