Skip to content

Fix #17 plus five more lwIP thread-safety violations (found via CONFIG_LWIP_CHECK_THREAD_SAFETY) #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
antonmeyer wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix #17 plus five more lwIP thread-safety violations (found via CONFIG_LWIP_CHECK_THREAD_SAFETY) #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4c084a
Fix #17: use tcpip_input() instead of ip_input() in wireguardif_proce…
Aug 20, 2026
0de347e
Fix five more thread-safety violations found via CONFIG_LWIP_CHECK_TH…
Aug 20, 2026
f7fa5a0
Use netif->input() instead of hardcoded tcpip_input() for the RX fix
Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 73 additions & 8 deletions components/microlink/components/wireguard_lwip/src/wireguardif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -537,11 +581,32 @@ 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 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);
ip_input(pbuf, device->netif);
// pbuf is owned by IP layer now
pbuf = NULL;
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] netif->input() failed: %d, dropping\n", input_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");
}
Expand Down Expand Up @@ -1155,7 +1220,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);
}
}

Expand Down Expand Up @@ -1205,7 +1270,7 @@ void wireguardif_periodic(struct netif *netif) {
}
}
if (!link_up) {
netif_set_link_down(device->netif);
wireguardif_mark_link_down(device->netif);
}
}

Expand Down
151 changes: 131 additions & 20 deletions components/microlink/src/ml_wg_mgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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;
}
Expand All @@ -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];
Expand Down Expand Up @@ -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 */
Expand Down