DF-0706 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 | 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; |