Fix #17 plus five more lwIP thread-safety violations (found via CONFIG_LWIP_CHECK_THREAD_SAFETY) - #28
Conversation
…if_process_data_message ip_input()/ip4_input() is not thread-safe and must only run on lwIP's own tcpip_thread. This call site runs on whatever task feeds received WireGuard packets into wireguardif_network_rx() (e.g. a project's own WG-management task, not tcpip_thread), so calling ip_input() directly here violates lwIP's core thread-safety contract. Reproduced as a concrete crash on real ESP32 hardware: sustained real TCP load through the tunnel (a firmware upload over a Tailscale-based integration) triggered a stack overflow at an unrelated call site (wg_udp_output_cb recursing via the WG netif's own broad route table entry), traced back through several ruled-out causes (queue corruption, core- affinity races, MTU/fragmentation, PSRAM cache coherency, each checked with real evidence) to this exact thread-safety violation letting this function's inline IP-stack processing race against other tasks' correct use of the socket API. tcpip_input() is lwIP's own thread-safe dispatcher for exactly this situation - same signature, posts the pbuf to tcpip_thread's mailbox instead of processing it in place. Verified on real hardware: the same load pattern that reliably crashed the board before now completes cleanly across multiple runs.
…READ_SAFETY Espressif's CONFIG_LWIP_CHECK_THREAD_SAFETY makes lwIP core functions assert immediately if called off tcpip_thread. Used it to audit the rest of this codebase after the CamM2325#17 fix and found five more real violations - none of them the CamM2325#17 bug itself, but the same underlying pattern: code assuming a function always runs on tcpip_thread, when in a magicsock-style integration it's often called directly from a project's own WireGuard-management task instead. - wg_init_interface()'s one-time interface bring-up (netif_list splice, netif_set_up(), netif_set_link_up()) ran inline on the caller's task; a comment even explained why netif_add() (which handles this safely) was deliberately avoided. Fixed by dispatching the whole sequence through tcpip_callback() + a semaphore - this project already uses exactly that pattern correctly elsewhere (ml_zerocopy.c's own PCB setup), just missed here. - Same fix for udp_new() creating the raw WG output PCB in the same function. - wg_udp_output_cb()'s udp_sendto() - its own comment claimed the raw PCB was "safe from any thread context," true only when already on tcpip_thread (this callback runs both from tcpip_thread, when an app socket's send routes through the WG netif, and directly from the WG-management task for handshakes/keepalives). Always dispatching would deadlock when already on tcpip_thread, so the fix checks sys_thread_tcpip(LWIP_CORE_LOCK_QUERY_HOLDER) first and only dispatches when actually off tcpip_thread. - netif_set_link_up(), two call sites (post-handshake, and a defensive per-data-packet check) - both always run off tcpip_thread in this integration. Fixed with a shared wrapper that also skips the tcpip_callback() dispatch once NETIF_FLAG_LINK_UP is already set, so the per-packet call site costs one flag read in the steady state, not a queued callback on every packet. - netif_set_link_down(), two call sites (wireguardif_tmr() and wireguardif_periodic()'s per-tick timeout sweep) - same pattern, mirrored fix. Verified on real hardware with the checker still enabled: clean boot through full connection, 30+ seconds of normal operation, and a complete OTA-style push over a real tunnel with sustained TCP load - zero further assertions.
wg_init_interface() already sets netif->input = tcpip_input specifically so RX code would dispatch through it instead of entering the IP stack directly - calling the netif's own configured input function (rather than hardcoding tcpip_input() again here) actually honors that existing setup instead of just swapping one hardcoded function for another that happens to currently match it. Same thread-safety fix as before, functionally identical in this integration, but follows the same pattern PR CamM2325#20 independently arrived at for this exact line, and matches how every other lwIP netif driver hands a received packet up the stack. Verified on real hardware again after the change: a full OTA push over the Tailscale tunnel (the same sustained-real-TCP-load scenario that originally crashed the board) completes cleanly, zero assertions.
|
Independent reproduction of the root cause in this PR, on separate hardware — details and a Short version: ESP32-S3 running a SOCKS5 proxy over the tailnet, every TCP transfer past a few tens of KB killed the device. Different assertion from the which is the same corruption landing in TCP's send queue instead. Arrived at it the same way you did — ruled out MTU, queue depth and buffer exhaustion with packet captures first, then found Confirming the fix works: routing RX through I have not exercised the other five violations this PR fixes as hard, but the reasoning for each looks right to me, and the One caveat worth flagging for whoever tests this next: with |
Original fix (commit 1):
ip_input()/ip4_input()is not thread-safe and must only run on lwIP's owntcpip_thread. This call site (inwireguardif_process_data_message()) runs on whatever task feeds received WireGuard packets intowireguardif_network_rx()— in a typical integration that's a dedicated WireGuard-management task, nottcpip_thread— so callingip_input()directly here violates lwIP's core thread-safety contract.Reproduced as a concrete crash on real ESP32-S3 hardware: sustained real TCP load through the tunnel (a firmware upload over a Tailscale-based integration built on this library) triggered a stack overflow at an unrelated call site (
wg_udp_output_cbrecursing via the WG netif's own broad route table entry). Traced back through several ruled-out causes — queue corruption, core-affinity races, MTU/fragmentation, PSRAM cache coherency, each checked with real evidence (tcpdump capture, targeted ENQ/DEQ tracing, a hardware stack watchpoint) — to this exact thread-safety violation letting this function's inline IP-stack processing race against other tasks' correct use of the socket API.tcpip_input()is lwIP's own thread-safe dispatcher for exactly this situation (same signature, posts the pbuf totcpip_thread's mailbox instead of processing it in place — the pattern every other lwIP netif driver uses).Follow-up fix (commit 2): after the fix above, used Espressif's
CONFIG_LWIP_CHECK_THREAD_SAFETY(makes every lwIP core function that requirestcpip_threadassert immediately, by name, with a backtrace, if called from the wrong task) to audit the rest of the codebase rather than assume this was the only violation. It found five more, all insidewg_init_interface()'s one-time interface setup andwireguardif.c's handshake/session-state handling:netif_set_up()andudp_new()inwg_init_interface()'s one-time bring-up — dispatched throughtcpip_callback()+ a semaphore (this project's ownml_zerocopy.calready uses exactly this pattern correctly elsewhere for its own PCB setup).wg_udp_output_cb()'sudp_sendto()— this callback runs both fromtcpip_thread(when an app socket's send routes through the WG netif) and directly from the WireGuard-management task (handshakes/keepalives). Always dispatching would deadlock when already ontcpip_thread, so the fix checkssys_thread_tcpip(LWIP_CORE_LOCK_QUERY_HOLDER)first and only dispatches when actually needed.netif_set_link_up()(2 call sites) andnetif_set_link_down()(2 call sites) — fixed with shared wrapper functions that also skip thetcpip_callback()dispatch once the netif's link-state flag already matches, so the per-data-packet call site costs one flag read in the steady state, not a queued callback on every packet.Verified on real hardware, both fixes together, with the checker still enabled: clean boot through a full connection, 30+ seconds of normal operation, and a complete OTA-style push over a real tunnel with sustained TCP load — zero further assertions, zero anomalies.
Fixes #17.