diff --git a/sys/net/ipfw3_basic/ip_fw3_sync.c b/sys/net/ipfw3_basic/ip_fw3_sync.c --- a/sys/net/ipfw3_basic/ip_fw3_sync.c +++ b/sys/net/ipfw3_basic/ip_fw3_sync.c @@ -192,6 +192,14 @@ struct sockaddr *sa; int error, flags, *type; + /* + * DF-0706 (a): cache the socket pointer locally. The handler + * typically outlives any individual config ioctl, and the cleanup + * at the bottom of this routine MUST release the same socket we + * were receiving on; otherwise a concurrent ip_fw3_ctl_sync_edge_conf + * that replaced fw3_sync_ctx.edge_sock would cause us to sofree() + * the *new* socket and leak the original. + */ so = fw3_sync_ctx.edge_sock; flags = MSG_FBLOCKING; @@ -206,13 +214,26 @@ if (error) break; m = sio.sb_mb; + if (m == NULL || m->m_len < sizeof(int)) { + /* DF-0704: drop short datagrams */ + kprintf("ipfw3sync: ignore short datagram\n"); + continue; + } type = (int *)m->m_data; if (*type == SYNC_TYPE_SEND_TEST) { struct cmd_send_test *cmd; + if (m->m_len < sizeof(*cmd)) { + kprintf("ipfw3sync: short SEND_TEST\n"); + continue; + } cmd = (struct cmd_send_test *)m->m_data; kprintf("test received %d\n", cmd->num); } else if (*type == SYNC_TYPE_SEND_STATE) { struct cmd_send_state *cmd; + if (m->m_len < sizeof(*cmd)) { + kprintf("ipfw3sync: short SEND_STATE\n"); + continue; + } cmd = (struct cmd_send_state *)m->m_data; if (ipfw_sync_install_state_prt != NULL) { (*ipfw_sync_install_state_prt)(cmd); @@ -224,8 +245,16 @@ kprintf("Error ignore\n"); } } - soshutdown(fw3_sync_ctx.edge_sock, SHUT_RD); - sofree(fw3_sync_ctx.edge_sock); + /* + * DF-0706 (a)+(b): use the cached socket, and only drop the single + * reference that soreference() added in ip_fw3_ctl_sync_edge_start. + * The previous code called soshutdown+sofree on fw3_sync_ctx.edge_sock + * which (i) hit the wrong socket if edge_conf replaced it while we + * were running, and (ii) when combined with ip_fw3_ctl_sync_edge_stop's + * soclose() dropped two refs for one soreference(). + */ + soshutdown(so, SHUT_RD); + sofree(so); kthread_exit(); } @@ -350,9 +379,25 @@ int ip_fw3_ctl_sync_edge_stop(struct sockopt *sopt) { + /* + * DF-0706 (b): previously called soclose(edge_sock). The edge + * socket is owned by the edge_socket_handler thread which already + * drops the soreference() added by ip_fw3_ctl_sync_edge_start via + * sofree() on exit. Calling soclose() here dropped a second + * reference, breaking the (single) refcount added at start. + * + * The correct way to stop the edge is to clear the running bit and + * shut down the receive side; the handler will return from + * so_pru_soreceive, perform soshutdown+sofree on the cached socket, + * and exit. The caller is expected to kthread_join(edge_td). + */ if (fw3_sync_ctx.running & 1) { fw3_sync_ctx.running &= 2; - soclose(fw3_sync_ctx.edge_sock, 0); + soshutdown(fw3_sync_ctx.edge_sock, SHUT_RD); + if (fw3_sync_ctx.edge_td != NULL) { + kthread_join(fw3_sync_ctx.edge_td); + fw3_sync_ctx.edge_td = NULL; + } } return 0; } @@ -474,18 +519,48 @@ ipfw_sync_send_state_prt = ip_fw3_sync_send_state; break; case MOD_UNLOAD: + /* + * DF-0706 (d): clear the exported function pointers + * FIRST so no other thread can dispatch into code we + * are about to unmap. Without these NULLs the + * pointers remain dangling after module unload. + */ + ipfw_sync_send_state_prt = NULL; + ipfw_sync_install_state_prt = NULL; + if (fw3_sync_ctx.edges != NULL) { kfree(fw3_sync_ctx.edges, M_IPFW3_SYNC); + fw3_sync_ctx.edges = NULL; } + /* + * DF-0706 (b)+(c): shut down the receive side so the + * handler thread returns from so_pru_soreceive, then + * kthread_join() it before completing the unload. + * The previous code called soclose() and immediately + * NULLed edge_td, leaving the handler running inside + * module code that was about to be unmapped. + */ if (fw3_sync_ctx.running & 1) { - fw3_sync_ctx.running = 0; - soclose(fw3_sync_ctx.edge_sock, 0); - fw3_sync_ctx.edge_td = NULL; + fw3_sync_ctx.running &= ~1; + if (fw3_sync_ctx.edge_sock != NULL) + soshutdown(fw3_sync_ctx.edge_sock, + SHUT_RD); + if (fw3_sync_ctx.edge_td != NULL) { + kthread_join(fw3_sync_ctx.edge_td); + fw3_sync_ctx.edge_td = NULL; + } + if (fw3_sync_ctx.edge_sock != NULL) { + sofree(fw3_sync_ctx.edge_sock); + fw3_sync_ctx.edge_sock = NULL; + } } if (fw3_sync_ctx.running & 2) { int i; + fw3_sync_ctx.running &= ~2; for (i = 0; i < fw3_sync_ctx.count; i++) { - soclose(fw3_sync_ctx.centre_socks[i], 0); + if (fw3_sync_ctx.centre_socks[i] != NULL) + soclose( + fw3_sync_ctx.centre_socks[i], 0); } } break;