From a4c084a176af03672962a8b587992bdfa180b8c9 Mon Sep 17 00:00:00 2001 From: Anton Date: Thu, 20 Aug 2026 22:45:45 +0200 Subject: [PATCH 1/3] Fix #17: use tcpip_input() instead of ip_input() in wireguardif_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. --- .../wireguard_lwip/src/wireguardif.c | 37 +++++++++++++++++-- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/components/microlink/components/wireguard_lwip/src/wireguardif.c b/components/microlink/components/wireguard_lwip/src/wireguardif.c index 6627fd0..abcd595 100644 --- a/components/microlink/components/wireguard_lwip/src/wireguardif.c +++ b/components/microlink/components/wireguard_lwip/src/wireguardif.c @@ -537,11 +537,40 @@ static void wireguardif_process_data_message(struct wireguard_device *device, st // 5. If the plaintext packet has not been dropped, it is inserted into the receive queue of the wg0 interface. if (dest_ok) { - // Send packet to be processed by LWIP + // Send packet to be processed by LWIP. Deliberately + // tcpip_input(), NOT ip_input(): this whole function + // runs on the caller's own task (whichever task feeds + // received WireGuard packets into wireguardif_network_rx, + // e.g. ml_wg_mgr in this project - not lwIP's own + // tcpip_thread), and ip_input()/ip4_input() is not + // thread-safe - it must only ever run on tcpip_thread. + // tcpip_input() is lwIP's own thread-safe dispatcher for + // exactly this situation (any netif driver feeding a + // packet in from its own RX task/ISR instead of from + // tcpip_thread itself): it posts the pbuf to + // tcpip_thread's mailbox, which then calls ip_input() on + // its own thread - the same pattern every other lwIP + // netif driver uses. Fixes #17: reproduced on real + // hardware as a stack overflow (unrelated call site, + // wg_udp_output_cb recursing via the WG netif's own + // overly-broad route) that only appeared under real TCP + // load through the tunnel, root-caused back to this + // exact thread-safety violation letting this function's + // inline IP-stack processing race against other tasks' + // correct use of the socket API. Calling ip_input() + // directly here (rather than through tcpip_input()) is + // exactly the anti-pattern lwIP's own docs warn against + // for any code that isn't itself running on tcpip_thread. WG_DEBUG("[WG_RX_IP] Passing %u bytes to IP layer\n", (unsigned)pbuf->tot_len); - ip_input(pbuf, device->netif); - // pbuf is owned by IP layer now - pbuf = NULL; + err_t tcpip_err = tcpip_input(pbuf, device->netif); + if (tcpip_err == ERR_OK) { + // pbuf ownership transferred to tcpip_thread + pbuf = NULL; + } else { + WG_DEBUG("[WG_RX_IP] tcpip_input() failed: %d (mailbox full?), dropping\n", tcpip_err); + // pbuf stays non-NULL - freed by this function's + // normal cleanup path below + } } else { WG_DEBUG("[WG_RX_IP] DROPPED: dest_ok=false\n"); } From 0de347ef0eebdbf7be9155d6e740401b1af338d6 Mon Sep 17 00:00:00 2001 From: Anton Date: Thu, 20 Aug 2026 23:33:58 +0200 Subject: [PATCH 2/3] Fix five more thread-safety violations found via CONFIG_LWIP_CHECK_THREAD_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 #17 fix and found five more real violations - none of them the #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. --- .../wireguard_lwip/src/wireguardif.c | 52 +++++- components/microlink/src/ml_wg_mgr.c | 151 +++++++++++++++--- 2 files changed, 179 insertions(+), 24 deletions(-) diff --git a/components/microlink/components/wireguard_lwip/src/wireguardif.c b/components/microlink/components/wireguard_lwip/src/wireguardif.c index abcd595..f10a81d 100644 --- a/components/microlink/components/wireguard_lwip/src/wireguardif.c +++ b/components/microlink/components/wireguard_lwip/src/wireguardif.c @@ -72,6 +72,50 @@ void wireguardif_disable_socket_bind(void) { g_disable_socket_bind = true; } +// netif_set_link_up() is core lwIP netif state and must only run on +// tcpip_thread (confirmed on real hardware via CONFIG_LWIP_CHECK_THREAD_SAFETY, +// which asserts immediately otherwise). Both call sites below run on whatever +// task calls wireguardif_network_rx() (this project's ml_wg_mgr task, never +// tcpip_thread - handshake messages are processed here directly, never via +// the tcpip_input()-based inner-IP-packet path), so this always needs +// dispatching, unlike wg_udp_output_cb's context-dependent case. Fire-and-forget +// (tcpip_callback(), no wait): nothing here depends on netif_set_link_up() +// having completed before this function returns, so there's no reason to pay +// for a blocking round-trip on every handshake completion. +static void netif_set_link_up_in_tcpip(void *arg) { + netif_set_link_up((struct netif *)arg); +} +static void wireguardif_mark_link_up(struct netif *netif) { + // One call site is per-data-packet (wireguard_process_data_message()'s + // defensive "make sure link is reported as up"), not just per-handshake - + // skip the tcpip_callback() dispatch entirely once the flag is already + // set, so the common case (link already up) costs one flag read, not a + // callback queued on every packet. Reading netif->flags without the core + // lock is fine here: it's a single bit that only ever transitions + // unset->set for the life of a session, so a stale read costs at most one + // redundant (still correct) dispatch, never a wrong result. + if (netif->flags & NETIF_FLAG_LINK_UP) { + return; + } + tcpip_callback(netif_set_link_up_in_tcpip, netif); +} + +// Same thread-safety requirement and same reasoning as wireguardif_mark_link_up() +// above - both of this function's call sites (wireguardif_tmr() and +// wireguardif_periodic(), the periodic per-peer keepalive/rekey/timeout sweep) +// run once per tick, not once per packet, but still worth skipping the +// dispatch once the flag is already clear (the common steady-state case, +// either "link up" or "link already reported down"). +static void netif_set_link_down_in_tcpip(void *arg) { + netif_set_link_down((struct netif *)arg); +} +static void wireguardif_mark_link_down(struct netif *netif) { + if (!(netif->flags & NETIF_FLAG_LINK_UP)) { + return; + } + tcpip_callback(netif_set_link_down_in_tcpip, netif); +} + bool wireguardif_is_wireguard_packet(const uint8_t *data, size_t len) { if (len < 4) return false; // WireGuard message types are 1-4 in the first 32-bit LE word @@ -391,7 +435,7 @@ static void wireguardif_process_response_message(struct wireguard_device *device wireguardif_send_keepalive(device, peer); // Set the IF-UP flag on netif - netif_set_link_up(device->netif); + wireguardif_mark_link_up(device->netif); printf("[WG] *** WIREGUARD SESSION ESTABLISHED wg_idx=%u ***\n", wg_idx); } else { // Packet bad @@ -482,7 +526,7 @@ static void wireguardif_process_data_message(struct wireguard_device *device, st } // Make sure that link is reported as up - netif_set_link_up(device->netif); + wireguardif_mark_link_up(device->netif); if (pbuf->tot_len > 0) { //4a. Once the packet payload is decrypted, the interface has a plaintext packet. If this is not an IP packet, it is dropped. @@ -1184,7 +1228,7 @@ static void wireguardif_tmr(void *arg) { if (!link_up) { // Clear the IF-UP flag on netif - netif_set_link_down(device->netif); + wireguardif_mark_link_down(device->netif); } } @@ -1234,7 +1278,7 @@ void wireguardif_periodic(struct netif *netif) { } } if (!link_up) { - netif_set_link_down(device->netif); + wireguardif_mark_link_down(device->netif); } } diff --git a/components/microlink/src/ml_wg_mgr.c b/components/microlink/src/ml_wg_mgr.c index ed40942..fc2f874 100644 --- a/components/microlink/src/ml_wg_mgr.c +++ b/components/microlink/src/ml_wg_mgr.c @@ -24,6 +24,7 @@ #include "lwip/ip_addr.h" #include "lwip/ip.h" #include "lwip/tcpip.h" +#include "arch/sys_arch.h" /* sys_thread_tcpip() - detects whether we're already on tcpip_thread */ #include "nacl_box.h" #include "wireguardif.h" #include "wireguard.h" @@ -181,6 +182,23 @@ static err_t wg_derp_output_cb(const uint8_t *peer_public_key, * on that thread. */ static struct udp_pcb *s_wg_output_pcb = NULL; +/* Context for dispatching udp_sendto() onto tcpip_thread when wg_udp_output_cb + * isn't already running there. */ +typedef struct { + struct udp_pcb *pcb; + struct pbuf *p; + ip_addr_t dst; + u16_t port; + err_t result; + SemaphoreHandle_t done; +} wg_udp_send_ctx_t; + +static void wg_udp_sendto_in_tcpip(void *arg) { + wg_udp_send_ctx_t *sctx = (wg_udp_send_ctx_t *)arg; + sctx->result = udp_sendto(sctx->pcb, sctx->p, &sctx->dst, sctx->port); + xSemaphoreGive(sctx->done); +} + static err_t wg_udp_output_cb(uint32_t dest_ip, uint16_t dest_port, const uint8_t *data, size_t len, void *ctx) { microlink_t *ml = (microlink_t *)ctx; @@ -195,7 +213,6 @@ static err_t wg_udp_output_cb(uint32_t dest_ip, uint16_t dest_port, (int)dest_port, len >= 1 ? data[0] : -1); - /* Use raw PCB to send — safe from any thread context */ if (!s_wg_output_pcb) return ERR_CONN; struct pbuf *p = pbuf_alloc(PBUF_TRANSPORT, len, PBUF_RAM); @@ -206,7 +223,40 @@ static err_t wg_udp_output_cb(uint32_t dest_ip, uint16_t dest_port, IP_SET_TYPE_VAL(dst, IPADDR_TYPE_V4); ip4_addr_set_u32(ip_2_ip4(&dst), dest_ip); /* already network byte order */ - err_t err = udp_sendto(s_wg_output_pcb, p, &dst, dest_port); + /* udp_sendto() is raw lwIP core API - not thread-safe unless called from + * tcpip_thread. This callback runs from two different contexts: from + * tcpip_thread itself (when an app socket's send routes through the WG + * netif) and directly from ml_wg_mgr's own task (handshakes, keepalives, + * periodic housekeeping - confirmed on real hardware via + * CONFIG_LWIP_CHECK_THREAD_SAFETY, which caught this exact call during a + * one-shot handshake triggered from process_disco_pong()). Calling + * tcpip_callback() and blocking for the result when *already* on + * tcpip_thread would deadlock (tcpip_thread can't service its own queued + * callback while blocked waiting on it) - hence the explicit check + * rather than always dispatching. pbuf_alloc()/pbuf_free() aren't core + * state and don't need this - only the actual PCB send does. */ + err_t err; + if (sys_thread_tcpip(LWIP_CORE_LOCK_QUERY_HOLDER)) { + err = udp_sendto(s_wg_output_pcb, p, &dst, dest_port); + } else { + wg_udp_send_ctx_t sctx = { + .pcb = s_wg_output_pcb, .p = p, .dst = dst, .port = dest_port, + .done = xSemaphoreCreateBinary(), + }; + if (!sctx.done) { + pbuf_free(p); + return ERR_MEM; + } + err_t cb_err = tcpip_callback(wg_udp_sendto_in_tcpip, &sctx); + if (cb_err != ERR_OK) { + vSemaphoreDelete(sctx.done); + pbuf_free(p); + return cb_err; + } + xSemaphoreTake(sctx.done, portMAX_DELAY); + vSemaphoreDelete(sctx.done); + err = sctx.result; + } pbuf_free(p); return err; } @@ -215,6 +265,49 @@ static err_t wg_udp_output_cb(uint32_t dest_ip, uint16_t dest_port, * WireGuard Interface Initialization * ========================================================================== */ +/* Splices netif into lwIP's global netif_list and brings it up. Like + * netif_set_up()/netif_set_link_up() themselves, direct netif_list + * manipulation is core lwIP state and must only happen on tcpip_thread - + * confirmed on real hardware via CONFIG_LWIP_CHECK_THREAD_SAFETY, which + * asserts immediately if this runs anywhere else (this used to run inline + * on ml_wg_mgr's own task). Dispatched via tcpip_callback() + a semaphore, + * matching the same pattern ml_zerocopy.c already uses for its own PCB + * setup - this runs once at startup, not per-packet, so the round-trip + * cost is irrelevant. */ +typedef struct { + struct netif *netif; + SemaphoreHandle_t done; +} wg_netif_up_ctx_t; + +static void wg_netif_bring_up_in_tcpip(void *arg) { + wg_netif_up_ctx_t *ctx = (wg_netif_up_ctx_t *)arg; + ctx->netif->next = netif_list; + netif_list = ctx->netif; + netif_set_up(ctx->netif); + netif_set_link_up(ctx->netif); + xSemaphoreGive(ctx->done); +} + +/* udp_new() (raw lwIP PCB allocation) has the same tcpip_thread-only + * requirement - confirmed via the same CONFIG_LWIP_CHECK_THREAD_SAFETY + * assertion once wg_netif_bring_up_in_tcpip() above stopped masking it. + * Same dispatch pattern; only touches the module-static s_wg_output_pcb, + * so no per-call context struct needed beyond the semaphore. */ +static SemaphoreHandle_t s_wg_output_pcb_done; +static void wg_output_pcb_create_in_tcpip(void *arg) { + (void)arg; + s_wg_output_pcb = udp_new(); + if (s_wg_output_pcb) { + /* Set source port to 51820 (matching DISCO socket) WITHOUT calling + * udp_bind — avoids registering for input which would steal WG + * responses from the DISCO BSD socket. udp_sendto uses local_port. */ + s_wg_output_pcb->local_port = 51820; + /* DSCP 46 (EF) → WMM AC_VO for low-latency WiFi scheduling */ + s_wg_output_pcb->tos = 0xB8; + } + xSemaphoreGive(s_wg_output_pcb_done); +} + static esp_err_t wg_init_interface(microlink_t *ml) { /* Convert our WG private key to base64 */ char privkey_b64[64]; @@ -264,28 +357,46 @@ static esp_err_t wg_init_interface(microlink_t *ml) { * callback uses raw udp_sendto (not BSD sendto) to avoid deadlock. */ netif->input = tcpip_input; - /* Add to lwIP netif list (bypass netif_add which wants init callback) */ - netif->next = netif_list; - netif_list = netif; - - /* Bring interface up */ - netif_set_up(netif); - netif_set_link_up(netif); + /* Add to lwIP netif list and bring up on tcpip_thread (bypass netif_add + * which wants init callback) - see wg_netif_bring_up_in_tcpip() above. */ + { + wg_netif_up_ctx_t ctx = { .netif = netif, .done = xSemaphoreCreateBinary() }; + if (!ctx.done) { + ESP_LOGE(TAG, "Failed to allocate netif-up semaphore"); + free(netif); + return ESP_FAIL; + } + err_t cb_err = tcpip_callback(wg_netif_bring_up_in_tcpip, &ctx); + if (cb_err != ERR_OK) { + ESP_LOGE(TAG, "tcpip_callback failed to queue netif bring-up: %d", cb_err); + vSemaphoreDelete(ctx.done); + free(netif); + return ESP_FAIL; + } + xSemaphoreTake(ctx.done, portMAX_DELAY); + vSemaphoreDelete(ctx.done); + } /* Create raw UDP PCB for WG output (avoids BSD sendto deadlock on TCPIP - * thread). Bind to port 51820 to match the DISCO socket source port. - * The existing BSD disco_sock4 is only used from the wg_mgr task for - * DISCO/STUN; this raw PCB is used from the TCPIP thread for WG output. */ + * thread). Bind to port 51820 to match the DISCO socket source port. */ if (!s_wg_output_pcb) { - s_wg_output_pcb = udp_new(); - if (s_wg_output_pcb) { - /* Set source port to 51820 (matching DISCO socket) WITHOUT calling - * udp_bind — avoids registering for input which would steal WG - * responses from the DISCO BSD socket. udp_sendto uses local_port. */ - s_wg_output_pcb->local_port = 51820; - /* DSCP 46 (EF) → WMM AC_VO for low-latency WiFi scheduling */ - s_wg_output_pcb->tos = 0xB8; + s_wg_output_pcb_done = xSemaphoreCreateBinary(); + if (!s_wg_output_pcb_done) { + ESP_LOGE(TAG, "Failed to allocate WG output PCB semaphore"); + free(netif); + return ESP_FAIL; + } + err_t cb_err = tcpip_callback(wg_output_pcb_create_in_tcpip, NULL); + if (cb_err != ERR_OK) { + ESP_LOGE(TAG, "tcpip_callback failed to queue WG output PCB creation: %d", cb_err); + vSemaphoreDelete(s_wg_output_pcb_done); + s_wg_output_pcb_done = NULL; + free(netif); + return ESP_FAIL; } + xSemaphoreTake(s_wg_output_pcb_done, portMAX_DELAY); + vSemaphoreDelete(s_wg_output_pcb_done); + s_wg_output_pcb_done = NULL; } /* Register output callbacks for magicsock mode */ From f7fa5a06abb3d48285bb3b1347c1cbbd3db8b940 Mon Sep 17 00:00:00 2001 From: Anton Date: Fri, 21 Aug 2026 00:14:40 +0200 Subject: [PATCH 3/3] Use netif->input() instead of hardcoded tcpip_input() for the RX fix 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 #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. --- .../wireguard_lwip/src/wireguardif.c | 48 ++++++++----------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/components/microlink/components/wireguard_lwip/src/wireguardif.c b/components/microlink/components/wireguard_lwip/src/wireguardif.c index f10a81d..736c815 100644 --- a/components/microlink/components/wireguard_lwip/src/wireguardif.c +++ b/components/microlink/components/wireguard_lwip/src/wireguardif.c @@ -581,37 +581,29 @@ static void wireguardif_process_data_message(struct wireguard_device *device, st // 5. If the plaintext packet has not been dropped, it is inserted into the receive queue of the wg0 interface. if (dest_ok) { - // Send packet to be processed by LWIP. Deliberately - // tcpip_input(), NOT ip_input(): this whole function - // runs on the caller's own task (whichever task feeds - // received WireGuard packets into wireguardif_network_rx, - // e.g. ml_wg_mgr in this project - not lwIP's own - // tcpip_thread), and ip_input()/ip4_input() is not - // thread-safe - it must only ever run on tcpip_thread. - // tcpip_input() is lwIP's own thread-safe dispatcher for - // exactly this situation (any netif driver feeding a - // packet in from its own RX task/ISR instead of from - // tcpip_thread itself): it posts the pbuf to - // tcpip_thread's mailbox, which then calls ip_input() on - // its own thread - the same pattern every other lwIP - // netif driver uses. Fixes #17: reproduced on real - // hardware as a stack overflow (unrelated call site, - // wg_udp_output_cb recursing via the WG netif's own - // overly-broad route) that only appeared under real TCP - // load through the tunnel, root-caused back to this - // exact thread-safety violation letting this function's - // inline IP-stack processing race against other tasks' - // correct use of the socket API. Calling ip_input() - // directly here (rather than through tcpip_input()) is - // exactly the anti-pattern lwIP's own docs warn against - // for any code that isn't itself running on tcpip_thread. + // Send packet to be processed by LWIP via netif->input(), + // NOT a hardcoded ip_input(): this whole function runs on + // the caller's own task (ml_wg_mgr, not lwIP's tcpip_thread), + // and ip_input()/ip4_input() is not thread-safe - it must + // only run on tcpip_thread. wg_init_interface() already sets + // netif->input = tcpip_input specifically so RX code would + // dispatch there instead of entering the IP stack directly - + // calling the netif's own configured input function (rather + // than hardcoding tcpip_input() here too) actually honors + // that existing setup, instead of just swapping one hardcoded + // function for another that happens to currently match it. + // Root-caused on real hardware (2026-08-20) - matches + // CamM2325/microlink#17 exactly; a stale comment here used to + // (incorrectly) claim ip_input() + // already dispatches to tcpip_thread internally, which it + // does not. WG_DEBUG("[WG_RX_IP] Passing %u bytes to IP layer\n", (unsigned)pbuf->tot_len); - err_t tcpip_err = tcpip_input(pbuf, device->netif); - if (tcpip_err == ERR_OK) { - // pbuf ownership transferred to tcpip_thread + err_t input_err = device->netif->input(pbuf, device->netif); + if (input_err == ERR_OK) { + // pbuf ownership transferred to netif->input() pbuf = NULL; } else { - WG_DEBUG("[WG_RX_IP] tcpip_input() failed: %d (mailbox full?), dropping\n", tcpip_err); + WG_DEBUG("[WG_RX_IP] netif->input() failed: %d, dropping\n", input_err); // pbuf stays non-NULL - freed by this function's // normal cleanup path below }