From 56c73def5c97a85aedffa12ffb21b2029feb2d87 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Tue, 2 Jun 2026 10:48:15 -0400 Subject: [PATCH 1/2] net: harden TCP net-channel fast path against connection teardown tcp_net_channel_packet() is the net-channel fast path: a per-connection callback run from net_channel::process_queue(). It assumed it ran with the socket lock held and the inpcb still attached to its socket. Neither holds in general: - The callback can be drained from the IRQ-side classifier path, which does not hold the lock. - in_pcbdetach() nulls inp_socket, and a TCP inpcb can outlive its socket (in_pcbfree() is deferred), so inp_socket may be NULL by the time a queued packet is drained. - A packet can arrive after the tcpcb has transitioned to CLOSED/LISTEN/TIME_WAIT. The slow path (tcp_input) drops segments for those states before reaching tcp_do_segment(); the fast path did not, so under connection churn a late packet reached tcp_do_segment() against a torn-down connection and tripped its state KASSERT, panicking the kernel. Lock the connection through the inpcb rather than the socket. For a TCP socket SOCK_LOCK(so) and INP_LOCK(inp) are the same underlying mutex (in_pcballoc does so->set_mutex(&inp->inp_lock)); we take it via the inpcb because tp->t_inpcb is the pointer we know is valid, whereas inp_socket is what detach tears away. Holding INP_LOCK(inp) lets us re-resolve inp_socket underneath it: - Almost always we arrive with the inpcb lock already held (the old code assumed this unconditionally). The one exception is the IRQ-side classifier drain, which runs without it, so we acquire INP_LOCK(inp) only when INP_LOCKED(inp) is false and release it the same number of times (tracked by locked_here). This deliberately does not rely on recursive re-locking. - Re-resolve inp_socket under the lock; drop the packet if the inpcb has detached. - Skip segments for connections at or below LISTEN or in TIME_WAIT, matching the slow path. How this was found and tested: the lifetime hazard was found by code inspection while auditing the net-channel path, not from a captured panic, so there is no deterministic in-tree reproducer to turn into a tests/ case -- the race is between net_channel callback drain and socket detach under connection churn, and reproducing it reliably would require fault-injection hooks the tree does not have. The crash signature matches the page_fault on a NULL socket lock reported in #936 (soabort -> lockfree::mutex::lock(this=0x0) during close), which is the same inp_socket-outlives-socket window this guards. Verification is therefore by inspection plus build-qualification rather than a new test: the guard reproduces the exact CLOSED/LISTEN/TIME_WAIT checks the slow path already trusts. Build-qualified on a g++ 14.3.0 host: tcp_input.cc compiles and the kernel links clean (loader.elf, image=empty). --- bsd/sys/netinet/tcp_input.cc | 47 +++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/bsd/sys/netinet/tcp_input.cc b/bsd/sys/netinet/tcp_input.cc index a0aa4f1f01..fe7bb1f1d7 100644 --- a/bsd/sys/netinet/tcp_input.cc +++ b/bsd/sys/netinet/tcp_input.cc @@ -3186,7 +3186,6 @@ tcp_newreno_partial_ack(struct tcpcb *tp, struct tcphdr *th) #include #include -// INP_LOCK held static void tcp_net_channel_packet(tcpcb* tp, mbuf* m) { @@ -3202,14 +3201,56 @@ tcp_net_channel_packet(tcpcb* tp, mbuf* m) auto drop_hdrlen = h - start; tcp_fields_to_host(th); trace_tcp_input_ack(tp, th->th_ack.raw()); - auto so = tp->t_inpcb->inp_socket; + + /* + * Almost always we arrive here with the inpcb lock already held (the + * old code simply assumed it). The exception is the IRQ-side + * classifier path, which can drain a queued packet without the lock. + * So acquire INP_LOCK(inp) only if we don't already own it, and + * release it below the same number of times (tracked by locked_here). + * + * We lock via the inpcb because tp->t_inpcb is the pointer we know is + * valid; inp_socket is what in_pcbdetach() tears away. Holding the + * inpcb lock lets us re-read inp_socket and NULL-check it before use. + */ + auto inp = tp->t_inpcb; + if (!inp) { + m_freem(m); + return; + } + bool locked_here = !INP_LOCKED(inp); + if (locked_here) { + INP_LOCK(inp); + } + auto so = inp->inp_socket; + if (!so) { + if (locked_here) { + INP_UNLOCK(inp); + } + m_freem(m); + return; + } auto ip_len = ntohs(ip_hdr->ip_len); auto tlen = ip_len - (ip_size + (th->th_off << 2)); auto iptos = ip_hdr->ip_tos; - SOCK_LOCK_ASSERT(so); + /* + * tcp_do_segment() only handles a live data-bearing connection. The + * slow path (tcp_input) drops segments for CLOSED/LISTEN/TIME_WAIT + * before reaching it; mirror that here. + */ + if (tp->get_state() <= TCPS_LISTEN || tp->get_state() == TCPS_TIME_WAIT) { + if (locked_here) { + INP_UNLOCK(inp); + } + m_freem(m); + return; + } bool want_close; m_trim(m, ETHER_HDR_LEN + ip_len); tcp_do_segment(m, th, so, tp, drop_hdrlen, tlen, iptos, TI_UNLOCKED, want_close); + if (locked_here) { + INP_UNLOCK(inp); + } // since a socket is still attached, we should not be closing assert(!want_close); } From 53e26ca8ff52be9453f0bd2e58ad21363f6fcae0 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Wed, 8 Jul 2026 06:08:16 -0400 Subject: [PATCH 2/2] net: drop the conditional inpcb lock in tcp_net_channel_packet tcp_net_channel_packet() is only reached through net_channel::process_queue(), and every caller of process_queue() (the flush_net_channel sites in uipc_socket.cc under SOCK_LOCK, and the tcp_flush_net_channel sites in tcp_input/tcp_timer under INP_LOCK) already holds the inpcb lock. SOCK_LOCK and INP_LOCK are the same mutex: in_pcb.cc does so->set_mutex(&inp->inp_lock). The IRQ classifier only enqueues mbufs via post_packet(); it never calls the process function directly. So the "acquire the lock if we don't already own it" branch was dead code. Replace it with INP_LOCK_ASSERT(inp), matching the rest of this file. Keep the inp_socket NULL-check (a batched segment can detach the socket mid-drain) and the connection-state check, both of which run under the asserted lock. --- bsd/sys/netinet/tcp_input.cc | 40 +++++++++++++----------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/bsd/sys/netinet/tcp_input.cc b/bsd/sys/netinet/tcp_input.cc index fe7bb1f1d7..96fbf3ccea 100644 --- a/bsd/sys/netinet/tcp_input.cc +++ b/bsd/sys/netinet/tcp_input.cc @@ -3203,30 +3203,24 @@ tcp_net_channel_packet(tcpcb* tp, mbuf* m) trace_tcp_input_ack(tp, th->th_ack.raw()); /* - * Almost always we arrive here with the inpcb lock already held (the - * old code simply assumed it). The exception is the IRQ-side - * classifier path, which can drain a queued packet without the lock. - * So acquire INP_LOCK(inp) only if we don't already own it, and - * release it below the same number of times (tracked by locked_here). - * - * We lock via the inpcb because tp->t_inpcb is the pointer we know is - * valid; inp_socket is what in_pcbdetach() tears away. Holding the - * inpcb lock lets us re-read inp_socket and NULL-check it before use. + * Every caller reaches this through net_channel::process_queue(), + * which always runs with the inpcb lock held (that lock aliases the + * socket mutex - see in_pcb.cc: so->set_mutex(&inp->inp_lock)). The + * IRQ classifier only enqueues via post_packet(), it never invokes + * the process function directly, so we can assert the lock rather + * than re-acquire it. */ auto inp = tp->t_inpcb; - if (!inp) { - m_freem(m); - return; - } - bool locked_here = !INP_LOCKED(inp); - if (locked_here) { - INP_LOCK(inp); - } + INP_LOCK_ASSERT(inp); + + /* + * process_queue() drains a batch of mbufs in a loop; an earlier + * segment can close the connection and detach the socket + * (in_pcbdetach() clears inp_socket) before a later iteration runs. + * Drop the packet instead of dereferencing a torn-down socket. + */ auto so = inp->inp_socket; if (!so) { - if (locked_here) { - INP_UNLOCK(inp); - } m_freem(m); return; } @@ -3239,18 +3233,12 @@ tcp_net_channel_packet(tcpcb* tp, mbuf* m) * before reaching it; mirror that here. */ if (tp->get_state() <= TCPS_LISTEN || tp->get_state() == TCPS_TIME_WAIT) { - if (locked_here) { - INP_UNLOCK(inp); - } m_freem(m); return; } bool want_close; m_trim(m, ETHER_HDR_LEN + ip_len); tcp_do_segment(m, th, so, tp, drop_hdrlen, tlen, iptos, TI_UNLOCKED, want_close); - if (locked_here) { - INP_UNLOCK(inp); - } // since a socket is still attached, we should not be closing assert(!want_close); }