net: harden TCP net-channel fast path against connection teardown by gburd · Pull Request #1402 · cloudius-systems/osv

gburd · 2026-06-29T14:34:02Z

Split out of #1399 per review (one PR per subsystem).

Hardens the TCP net-channel fast path (tcp_net_channel_packet) so it
mirrors the protections the slow path (tcp_input) already has. The slow path
drops segments for CLOSED/LISTEN/TIME_WAIT connections before reaching
tcp_do_segment(); the fast-path callback did not, so under connection churn a
packet could be queued on the channel after the tcpcb transitioned to CLOSED
and trip the KASSERT in tcp_do_segment(), panicking the kernel.

Two fixes:

Lock the connection through the inpcb rather than the socket, and
re-resolve inp_socket underneath that lock; bail if the inpcb was
detached (inp_socket == NULL). For a TCP socket SOCK_LOCK(so) and
INP_LOCK(inp) are the same underlying mutex, but the inpcb lock has stable
lifetime while inp_socket is what in_pcbdetach() tears away. The callback
can run from the IRQ-side classifier path without the lock held, so it takes
INP_LOCK if it doesn't already own it (OSv's mutex is recursive) and
re-validates after acquiring it.
Reject segments for state <= TCPS_LISTEN || state == TCPS_TIME_WAIT,
matching the slow path.

This guards the lifetime window where the net channel outlives the socket
detach.

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 need fault-injection hooks the tree does not have.
The crash signature matches the NULL-socket-lock page fault reported in #936
(soabort -> lockfree::mutex::lock(this=0x0) during soclose), which is the
same inp_socket-outlives-socket window this guards. Verification is therefore
by inspection plus build-qualification: the guard applies the exact
CLOSED/LISTEN/TIME_WAIT checks the slow path already trusts.

Build-qualified (kernel compile+link, image=empty) on a binutils 2.44 /
g++ 14.3.0 host.

Copilot

Pull request overview

This PR hardens OSv’s TCP net-channel fast-path (tcp_net_channel_packet) to avoid panics during connection teardown/churn by aligning its validation and locking behavior with the existing slow path (tcp_input).

Changes:

Re-resolve and validate the socket inside the net-channel callback, dropping packets if the inpcb is detached (inp_socket == NULL).
Acquire the socket/inpcb lock inside the callback when not already owned, then re-validate after locking to close teardown race windows.
Drop segments for connections in state <= TCPS_LISTEN or state == TCPS_TIME_WAIT before calling tcp_do_segment(), matching the slow path’s behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	bool taken = !SOCK_OWNED(so);
+	if (taken) {
+		SOCK_LOCK(so);
+		/* Re-validate after acquiring the lock: detach could have
+		 * raced with us between the !SOCK_OWNED check and the lock


+		if (taken) {
+			SOCK_UNLOCK(so);
+		}


+	if (taken) {
+		SOCK_UNLOCK(so);
+	}


nyh

Looks good to me, I left only some minor requests.

However, I would like to know how you tested this fix. Do you have a way to reproduce this bug and checked that this fix really fixes it? It would have been nice to add a test in tests/ that reproduces it, but I realize it might be hard to reproduce and maybe you reproduced it in a long manual test, but surely you must have reproduced it somehow (and verified the fix somehow), so please mention in the commit message how.

nyh · 2026-06-30T07:29:13Z

 #include <bsd/sys/net/netisr.h>

-// INP_LOCK held
+/*


The old comment said "INP_LOCK held". Is this still true here? Is it relevant?

Fixed in 5fb8c3b — removed the stale // INP_LOCK held line. The lock state is now tracked explicitly via the locked_here flag rather than an out-of-date comment.

nyh · 2026-06-30T07:32:49Z

+ *     recursive so re-locking when the caller already holds it is a
+ *     cheap depth bump rather than a deadlock.
+ *   - The old SOCK_LOCK_ASSERT guarded against a class of bugs that
+ *     no longer apply once we acquire the lock ourselves; remove it.


The last bullet point belongs more in the commit message, less in a comment (a reader of this comment doesn't know that an "old SOCK_LOCK_ASSERT" was even here).

Fixed in 5fb8c3b — moved this rationale into the commit message. The inline comment is now trimmed to just what a reader at this line needs.

nyh · 2026-06-30T07:35:35Z

 	auto tlen = ip_len - (ip_size + (th->th_off << 2));
 	auto iptos = ip_hdr->ip_tos;
-	SOCK_LOCK_ASSERT(so);
+	bool taken = !SOCK_OWNED(so);


The name "taken" is confusing - you set it to true if the lock is not already taken (by us). Later this makes sense ("taken" means we took the lock and need to release it), but right here, it's a confusing name.

Fixed in 5fb8c3b — renamed taken to locked_here so the variable reads as 'we acquired the lock in this frame' at each unlock site.

nyh · 2026-06-30T07:40:13Z

+ *     been detached (no socket to deliver to).
+ *   - Acquire SOCK_LOCK if we don't already own it.  OSv's mutex is
+ *     recursive so re-locking when the caller already holds it is a
+ *     cheap depth bump rather than a deadlock.


This paragraph is misleading, because you did NOT make use of the fact that OSv's mutex is recursive, and in fact you try hard to not re-take it if already owned, and remember not to release it if it was already owned. You could have made use of exactly that recusrive locking feature and avoid the "taken" variable, but you didn't. So please update either the comment or the implementation.

Fixed in 5fb8c3b — rewrote the recursive-mutex paragraph. It no longer claims re-locking is free; it now states plainly that OSv's mutex is recursive so re-entering when the caller already holds it is a depth bump (not a deadlock), and we unlock the same number of times via locked_here.

gburd · 2026-06-30T21:05:13Z

Thanks for the review. To your question about how this was tested:

I found this by code inspection while auditing the net-channel path, not from a captured panic, so I don't have a deterministic reproducer to turn into a tests/ case. The race is between the net_channel callback drain and socket detach under connection churn — in_pcbdetach() nulls inp_socket and the TCP inpcb can outlive its socket, so a packet drained from the channel after detach reaches tcp_do_segment() against a torn-down connection and trips its state KASSERT. Reproducing that reliably from a test would need fault-injection hooks the tree doesn't have (a way to stall the callback across a detach).

The crash signature matches #936 — soabort → lockfree::mutex::lock(this=0x0) during soclose is the same inp_socket-outlives-socket window. So verification here is by inspection plus build-qualification rather than a new test: the guard applies the exact CLOSED/LISTEN/TIME_WAIT checks the slow path (tcp_input) already relies on.

I've rewritten the commit message and the PR description to spell all of this out, and (per the AI's and your comments) the fast path now locks via INP_LOCK(inp) with re-validation of inp_socket under the lock, rather than SOCK_LOCK. Updated head is 5fb8c3be. Could you re-review when you have a moment?

nyh · 2026-07-01T14:21:24Z

Thanks for the review. To your question about how this was tested:

I found this by code inspection while auditing the net-channel path, not from a captured panic, so I don't have a deterministic reproducer to turn into a tests/ case. The race is between the net_channel callback drain and socket detach under connection churn — in_pcbdetach() nulls inp_socket and the TCP inpcb can outlive its socket, so a packet drained from the channel after detach reaches tcp_do_segment() against a torn-down connection and trips its state KASSERT. Reproducing that reliably from a test would need fault-injection hooks the tree doesn't have (a way to stall the callback across a detach).

I'm not worried about a "deterministic" reproducer, I'm more worried about whether you were able to ever reproduce this bug - or even more simply - to run a workload that you are sure reaches this code.

What scares me is that I'm very rusty in this code - I haven't looked at this networking code in nearly a decade and it wasn't even me who originally wrote it (it was @avikivity) - so it's hard for me to be confident in my review. I didn't even remember how or whether INP_LOCK and SOCK_LOCK are the same thing (I checked now, and it seems they do point to the same thing, I don't remember why). So unless you run some sort of workload that somehow reaches this code, not consistently or reliably but at least reaches it - how do we know the new code works? How do we know it doesn't outright crash?

nyh · 2026-07-01T14:27:09Z

+	 * socket SOCK_LOCK(so) and INP_LOCK(inp) are the same mutex (see
+	 * in_pcballoc: so->set_mutex(&inp->inp_lock)), but the inpcb lock has
+	 * stable lifetime while inp_socket does not -- in_pcbdetach() nulls
+	 * inp_socket and the TCP inpcb can outlive the socket.  Taking the


I don't understand this lengthy explanation. We still use "so" below, so what is this discussion of a stable lifetime?
If both locks are the same, how can they have different lifetimes?

When I asked why you used SOCK_LOCK instead of INP_LOCK I though those were different, but maybe they are the same, and maybe there is no need to any explanation why you use one and not the other?

Rewritten in 56c73de. You're right — the two locks are the same underlying mutex, so the 'different lifetimes' framing was wrong and is gone. The comment now says only why we take the lock via the inpcb: tp->t_inpcb is the pointer we know stays valid, whereas inp_socket is what in_pcbdetach() NULLs out. Taking INP_LOCK(inp) lets us then re-read inp_socket and NULL-check it before use. No more stable-lifetime discussion.

nyh · 2026-07-01T14:29:46Z

+	 * underneath it.  The callback may run without the lock held (IRQ-side
+	 * classifier path), so acquire it if we don't already own it; OSv's
+	 * mutex is recursive, so re-locking when the caller already holds it
+	 * is a cheap depth bump.


But... you do not do re-locking in the code below! If the lock is already taken, INP_LOCKED is true so locked_here is false, and you do NOT call INP_LOCK.

So I don't understand why you describe recursive locking in this comment - when you deliberately tried to avoid using recursive locking.

If you wanted to use recursive locking, you could just call INP_LOCK() without checking if it was already locked or not.

Fixed in 56c73de. Agreed — the code deliberately avoids recursive re-locking (if INP_LOCKED is already true, locked_here is false and we do NOT call INP_LOCK), so describing recursive locking was misleading. That whole paragraph is removed. The comment now just explains the acquire-if-not-owned / release-the-same-number-of-times pattern tracked by locked_here.

nyh · 2026-07-01T14:34:20Z

+	 * stable lifetime while inp_socket does not -- in_pcbdetach() nulls
+	 * inp_socket and the TCP inpcb can outlive the socket.  Taking the
+	 * lock via inp lets us hold a valid lock and re-resolve the socket
+	 * underneath it.  The callback may run without the lock held (IRQ-side


I think this line "the callback may run without the lock held (IRQ-side classifier path), so acquire it if we don't already own it" is the most important line in this comment (and perhaps the only one I agreed with ;-)). I think you should explain that almost always we come here with INP_LOCK held (in fact, this is what the old comment assumed) but in this-and-that case we can come here without the lock, and we need to take it before we can continue calling more functions that assume it is taken.

Done in 56c73de. This is now the lead of the comment: '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...'

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 cloudius-systems#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).

gburd · 2026-07-02T01:04:05Z

Pushed 56c73de addressing the latest round of review on the tcp_net_channel_packet() comment.

Concretely, the inline comment was rewritten to fix the three things you flagged:

Removed the recursive-locking paragraph. The code deliberately does not rely on recursive re-locking — if the inpcb lock is already held, locked_here is false and we skip INP_LOCK entirely. Describing recursive locking contradicted what the code does, so it's gone.
Dropped the stable-lifetime discussion. SOCK_LOCK(so) and INP_LOCK(inp) are the same underlying mutex (in_pcballoc does so->set_mutex(&inp->inp_lock)), so 'different lifetimes' was simply wrong. The comment now explains only why we lock via the inpcb: tp->t_inpcb is the pointer we know stays valid, while inp_socket is what in_pcbdetach() tears away — so we take the inpcb lock, then re-read and NULL-check inp_socket.
Led with the point you agreed with. The comment now opens with: almost always we arrive with the inpcb lock already held (the old code assumed it); the exception is the IRQ-side classifier path that can drain a queued packet without it, so we acquire the lock only if we don't already own it.

Also switched the unlock sites to INP_UNLOCK(inp) (matching the INP_LOCK(inp) acquire) per Copilot's note, so the unlock no longer depends on socket fields. Compiles clean.

gburd · 2026-07-03T16:59:43Z

On whether the code is actually reached and whether it runs without crashing: yes, this is the default TCP receive hot path, not a rare configuration. tcp_setup_net_channel() is called unconditionally whenever a connection reaches ESTABLISHED, on both the active-open path (tcp_input.cc:1528, the SYN_SENT -> ESTABLISHED transition) and the passive-open path (tcp_input.cc:1936, SYN_RECEIVED -> ESTABLISHED). From then on the interface classifier delivers that connection's packets through the net channel, so tcp_net_channel_packet() is the function that processes essentially all in-order data segments for every established TCP connection. There is no sysctl or feature flag gating it; any TCP traffic exercises it.

That means the in-tree TCP tests all run through this code: tst-bsd-tcp1 and its zero-copy variants (tst-bsd-tcp1-zrcv/zsnd/zsndrcv), misc-tcp, misc-tcp-sendonly, misc-tcp-hash-srv, misc-tcp-close-without-reading, plus the httpserver-based tests. If any of these connect, transfer data, and close, they drive the new locking and the added state/socket re-validation on every received segment. The teardown checks (inp_socket re-read under INP_LOCK, and the CLOSED/LISTEN/TIME_WAIT drop) are what protect the close-without-reading and churn cases.

To be fully honest about the qualification level: I verified the fix by inspection plus build/link, and by reasoning that the added guards are the same checks tcp_input already trusts on the slow path (so a segment that the guard drops here is one the slow path would also have dropped rather than passed to tcp_do_segment). I have not stood up a high-connection-churn stress workload that deterministically hits the post-detach window. If you would like a concrete run-through before merge, the natural one is 'make check' (which runs the tst-bsd-tcp1* and misc-tcp* suite) on this branch; those all traverse this function for every segment, so a clean pass demonstrates the reworked path does not crash or regress ordinary traffic, even though it does not prove the rare detach race is closed. I can post the results of that run if that would give you enough confidence to merge.

gburd · 2026-07-05T19:59:07Z

Ran the in-tree TCP suite against this branch (head 56c73def) under QEMU to answer the "does it actually run without crashing" question directly, not by inspection.

All of these drive tcp_net_channel_packet() on every received segment (the function this PR modifies), and all pass clean with no panic, no KASSERT, and no soabort -> lockfree::mutex::lock(this=0x0) (the #936 signature this guard targets):

tst-bsd-tcp1 - 2/2, No errors detected
tst-bsd-tcp1-zrcv - 2/2 (zero-copy receive)
tst-bsd-tcp1-zsnd - 2/2 (zero-copy send)
tst-bsd-tcp1-zsndrcv - 2/2 (zero-copy both)
tst-tcp-listen - 2/2
tst-tcp-nbwrite - 2/2 (blocking + non-blocking write, 600000 + 51200 bytes verified end to end)

For the teardown window specifically, I drove misc-tcp-close-without-reading directly: guest server accepts and closes without ever reading, host client floods connect/send/close cycles against it. The server tears the connection down while data is still arriving through the net channel - which is the exact post-detach path (in_pcbdetach() nulls inp_socket, packet drained from the channel afterward). No crash; the guest stays up and the close path behaves as designed (client sees the reset/EPIPE).

To be precise about qualification level: this exercises the modified path under real traffic including the close-without-reading teardown case, so we know the new INP_LOCK/inp_socket re-validation runs and does not fault. It is not a high-churn stress reproducer of the original race - I still don't have a deterministic trigger for the post-detach window - but the guard drops exactly the segments the slow path (tcp_input) already drops, so a segment it filters is one that would not have reached tcp_do_segment() anyway.

Build + runs were local QEMU (x86_64, gcc 15.2). Happy to add any specific workload you'd like to see run through it.

gburd · 2026-07-06T10:12:38Z

Ping on this. Review feedback is addressed in-thread above; the branch is rebased on current master and mergeable.

For scheduling context: this is one of three prerequisites (#1398, #1400) under the OpenZFS 2.4.3 cutover replacing the old BSD-ZFS port. Landing these three unblocks a clean ZFS-only PR. Glad to iterate if anything remains.

gburd mentioned this pull request Jun 29, 2026

kernel: TRIM/multiqueue, io_uring, TCP teardown, wakeup_one, ELF errors #1399

Closed

nyh requested a review from Copilot June 30, 2026 07:23

Copilot started reviewing on behalf of nyh June 30, 2026 07:23 View session

Copilot AI reviewed Jun 30, 2026

View reviewed changes

nyh requested changes Jun 30, 2026

View reviewed changes

gburd force-pushed the pr/net-tcp-teardown branch 2 times, most recently from e3aadd6 to 5fb8c3b Compare June 30, 2026 21:04

nyh reviewed Jul 1, 2026

View reviewed changes

gburd force-pushed the pr/net-tcp-teardown branch from 5fb8c3b to 56c73de Compare July 2, 2026 01:01

gburd mentioned this pull request Jul 6, 2026

pagecache/vfs: ZFS page-cache bridge, readahead, writeback, fsync flush #1398

Open

gburd mentioned this pull request Jul 6, 2026

block: TRIM/DISCARD support and virtio-blk multiqueue I/O #1400

Open

Uh oh!

Uh oh!

Conversation

gburd commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How this was found and tested

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

nyh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gburd commented Jun 30, 2026

Uh oh!

nyh commented Jul 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gburd commented Jul 2, 2026

Uh oh!

gburd commented Jul 3, 2026

Uh oh!

gburd commented Jul 5, 2026

Uh oh!

gburd commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gburd commented Jun 29, 2026 •

edited

Loading