Skip to content

net: implement TCP accept syscall - #454

Open
xuchang-vivo wants to merge 8 commits into
vivoblueos:mainfrom
xuchang-vivo:feat/tcp-accept
Open

net: implement TCP accept syscall#454
xuchang-vivo wants to merge 8 commits into
vivoblueos:mainfrom
xuchang-vivo:feat/tcp-accept

Conversation

@xuchang-vivo

@xuchang-vivo xuchang-vivo commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Implement TCP accept() end to end across the syscall, IPC, network manager, and smoltcp TCP layers.
  • Return a distinct accepted fd and preserve the listener for subsequent sequential accepts.
  • Support blocking and nonblocking accept behavior, peer address output, and transactional fd cleanup on failure.
  • Add shared local-port leases so listener and accepted sockets release the port exactly once.
  • Support SO_REUSEADDR, which std::net::TcpListener::bind() sets before binding a listener.
  • Add IPv4/IPv6 TCP accept coverage and update socket/VFS tests to use the accepted fd.

Design

  • A connected smoltcp TCP handle is transferred to the accepted socket.
  • The listener receives a fresh smoltcp handle and resumes listening.
  • The implementation intentionally provides sequential accept with an effective backlog of one; a queued multi-connection backlog requires a pending connection queue and multiple listener handles.
  • Closed protocol sockets are removed from NetworkManager after successful shutdown, and RefCell borrow scopes are explicit around that cleanup.
  • Socket option names are matched by exact value rather than as bit flags. POSIX option_name selects one option, and constants can share bits; for example, SO_REUSEADDR (0x0004) & SO_RCVTIMEO (0x1006) is non-zero.
  • The SO_REUSEADDR state is stored on Connection, exposed through setsockopt()/getsockopt(), and inherited by accepted connections. The current port allocator remains strict because BlueOS does not retain port leases for TCP TIME_WAIT sockets.

Testing

  • rustfmt --check on all changed Rust files
  • git diff --check
  • ninja -C out/qemu_mps2_an385.debug obj/kernel/kernel/blueos/libblueos.rlib
  • ninja -C out/seeed_xiao_esp32c3.debug tcp_server_example

kernel_unittest reaches final linking but is currently blocked by the repository/toolchain log -> std versus kernel panic_impl duplicate lang-item error.

Related

@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

✅ All jobs completed successfully, see https://github.com/vivoblueos/kernel/actions/runs/32344128512.

@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

✅ All jobs completed successfully, see https://github.com/vivoblueos/kernel/actions/runs/32356837034.

@han-jiang277

Copy link
Copy Markdown
Contributor

Summary

The PR implements accept() for TCP listeners, refactors PosixSocket::accept to carry the accepted connection / fd / ipc_reply / nonblocking flag, replaces Drop for Connection with a PortLease RAII type, adds SO_REUSEADDR getsockopt/setsockopt plumbing, and rewrites write_to_sockaddr to return Result<(), i32>.

The implementation is structurally sound and the test additions are appropriate. However, there are several real bugs, the most severe of which is a deadlock between blocking accept() and shutdown() on the same listener: a SocketWaker::wake() early-return on is_shutdown suppresses the only wakeup that could unblock the parked accept thread, and queue_and_wait then busy-yields forever on the shared ipc_reply futex. There are also two port-leak regressions introduced by removing Drop for Connection without covering every port-acquisition site, a listener-state corruption on accept() error rollback, and a handful of smaller correctness and cleanup issues.

Findings are ranked most-severe first. Correctness findings outrank cleanup/altitude.


P0 — Critical

1. accept() + shutdown() deadlock on the same listener (lost wakeup + shared futex)

Files: kernel/kernel/src/net/smoltcp/socket/tcp.rs:438-450, kernel/kernel/src/net/socket/socket_waker.rs:43-47, kernel/kernel/src/net/connection.rs:924-944

TcpSocket::shutdown sets is_shutdown.set(true) before calling socket.close(). socket.close() transitions the smoltcp socket out of Listen, which fires rx_waker.wake(). But SocketWaker::wake() guards on socket_is_shutdown.get() and returns early without re-enqueuing the captured Operation::Accept:

// socket_waker.rs
fn wake(&self) {
    if self.socket_is_shutdown.get() {
        log::debug!("[SocketWaker] {} socket is shutdown! ", self.name);
        return;  // <-- the only wakeup that could unblock accept() is dropped here
    }
    ...
}

So a thread parked in accept() (futex = STATE_WAITING_FOR_CONSUME) is never woken. Worse, Connection::shutdown reuses self.ipc_reply — the same OperationIPCReply the blocked accept() holds — so shutdown()'s queue_and_wait spins forever in:

while self.reply_futex.load(Ordering::Acquire) != STATE_IDLE {
    yield_me();   // accept never releases WAITING_FOR_CONSUME → infinite spin
}

The code's own comment at connection.rs:925 acknowledges this design limit ("If multiple threads share this socket, a stalled owner can block all I/O indefinitely"), but the PR's new accept path is the first operation that can park the owner indefinitely (no timeout), turning the design limit into a hard deadlock.

Failure scenario: Thread A calls accept(lfd) on a blocking listener, registers a recv waker, parks in queue_and_wait (futex = WAITING_FOR_CONSUME). Thread B calls shutdown(lfd) to stop the server. shutdown() acquires TcpSocket::shutdown, sets is_shutdown=true, calls socket.close()rx_waker.wake() early-returns. Thread B's queue_and_wait(shutdown_task) then spins in yield_me() because Thread A still holds the futex. Neither thread makes progress. The accepted_fd allocated in syscalls::accept is also leaked.

Suggested fix: Either (a) fire the waker before setting is_shutdown, (b) have shutdown() forcibly release the blocked ipc_reply (e.g. wake with ECONNABORTED and reset futex to STATE_IDLE), or (c) give queue_and_wait a real timeout (currently IPC_REPLY_TIMEOUT is ignored — see queue_and_wait_timeout at connection.rs:946-990, which loops indefinitely). At minimum, SocketWaker::wake() should enqueue the operation then check is_shutdown, or wake the client with an error before suppressing.


2. sendto() leaks the ephemeral port (regression from removing Drop for Connection)

File: kernel/kernel/src/net/connection.rs:335-356

The PR deletes impl Drop for Connection (which released the port from local_endpoint) and replaces it with PortLease. But sendto() acquires a port without creating a PortLease:

// connection.rs sendto(), line 346
let local_port = PORT_GENERATOR.acquire_port(self.socket_type, 0)?;
endpoint.replace((local_port).into());
Some(local_port)
// ^^^ no PortLease::new(...) anywhere in sendto

bind() (line 141) and connect() (line 248) both create a PortLease. sendto() does not. When a UDP socket calls sendto() without prior bind(), an ephemeral port is allocated and stored in local_endpoint, but nothing releases it on drop.

Failure scenario: A UDP socket calls sendto() without bind(). Port P is allocated as (P, SockDgram) in PORT_GENERATOR.allocated_ports. The socket is closed → Connection drops → no PortLeaserelease_port never called. Port P stays allocated forever. Repeated create/sendto/close cycles on UDP sockets exhaust the ephemeral range (~16K ports) and all new socket operations fail with EADDRNOTAVAIL.

Suggested fix: Add self.local_port_lease.lock().replace(PortLease::new(self.socket_type, local_port)); in sendto() alongside the acquire_port call, mirroring bind()/connect().


3. connect() hardcodes SockStream, PortLease stores self.socket_type — UDP port leak

File: kernel/kernel/src/net/connection.rs:248-251

let port = PORT_GENERATOR.acquire_port(SocketType::SockStream, 0)?;  // hardcoded SockStream
self.local_port_lease
    .lock()
    .replace(PortLease::new(self.socket_type, port));  // self.socket_type may be SockDgram

For a UDP socket calling connect(), the port is acquired as (port, SockStream) but the PortLease stores SockDgram. On drop, PortLease::drop calls release_port(SockDgram, port), which looks for (port, SockDgram) in the BTreeSet — not found, returns false (ignored by let _ =). The port is leaked as (port, SockStream) and cannot be reacquired by either TCP or UDP.

Failure scenario: A UDP socket calls connect() without bind(). Port P is allocated as (P, SockStream). On close, release_port(SockDgram, P) fails silently. P is leaked. After enough UDP connect cycles, the ephemeral range is exhausted.

Suggested fix: Replace SocketType::SockStream with self.socket_type at line 248. (This is technically a pre-existing bug, but the PR's PortLease change makes the mismatch newly consequential — before, Drop for Connection used self.socket_type for release, which also mismatched but the old code at least used the same type on both sides for TCP. With PortLease, the mismatch is now silent and permanent.)


4. accept() error rollback restores an Established handle as the listener

File: kernel/kernel/src/net/smoltcp/socket/tcp.rs:193-210

After the state check at line 157-174 confirms Established | CloseWait, the code takes old_handle (line 186-189) — which is now in Established/CloseWait state — and tries to create a fresh listener handle. On both error paths (create_smoltcp_socket() returns None at line 195, or listen() fails at line 206), the recovery does:

self.smoltcp_socket_handle = Some(old_handle);

But old_handle is the established connection's handle, not a Listen-state socket. The listener's TcpSocket now points at an Established smoltcp socket. The next accept() call sees State::Established, takes the same old_handle again, and hands it out as a new accepted socket — two accepted fds share one smoltcp SocketHandle.

Failure scenario: First accept() reaches the handle-swap path, but create_smoltcp_socket() fails (e.g. interface socket set is full). old_handle (Established) is restored to the listener. The established connection is never returned to the user (accept returns an error). The remote peer has an established connection but the server never reads data. The next accept() sees Established again, takes the same handle, returns a second fd for the same remote endpoint — double-remove on shutdown, interleaved data corruption.

Suggested fix: On error, the listener must be returned to a proper Listen state. Either re-listen on old_handle before restoring it, or accept that the listener is wedged and surface a fatal error. The cleanest fix is to perform create_smoltcp_socket() and listen() before taking old_handle, so the swap is atomic.


P1 — High

5. new_accepted() does not inherit is_nonblocking / recv_timeout / send_timeout

File: kernel/kernel/src/net/connection.rs:192-206

new_accepted() copies local_endpoint, local_port_lease, and reuse_address from the listener, but does not copy is_nonblocking, recv_timeout, or send_timeout. The accepted Connection is constructed via Self::new(...) which defaults is_nonblocking=false and timeouts to None.

POSIX (Linux glibc, BSD) inherits O_NONBLOCK and SO_RCVTIMEO/SO_SNDTIMEO from the listener to the accepted socket. This PR diverges from that behavior.

Failure scenario: Server creates a nonblocking listening socket (SOCK_STREAM | SOCK_NONBLOCK), accepts a connection, then calls recv() on the accepted fd expecting EAGAIN when no data is ready. The accepted socket's is_nonblocking is false, so recv() blocks indefinitely, deadlocking the server. Same issue for SO_RCVTIMEO/SO_SNDTIMEO — timeouts set on the listener do not apply to accepted sockets.

Suggested fix: In new_accepted(), also copy is_nonblocking, recv_timeout, and send_timeout from the listener.


6. SO_REUSEADDR is accepted and reported but never enforced

File: kernel/kernel/src/net/connection.rs:434-440, kernel/kernel/src/net/syscalls.rs:459-468, kernel/kernel/src/net/port_generator.rs:54-93

The PR adds set_reuse_address()/reuse_address() on Connection, and setsockopt(SO_REUSEADDR)/getsockopt(SO_REUSEADDR) correctly store and retrieve the flag. But bind(), listen(), and PortGenerator::acquire_port() never consult reuse_address. The flag is pure cargo.

Failure scenario: Application calls setsockopt(SO_REUSEADDR, 1), verifies via getsockopt (returns 1), then calls bind() to a port in TIME_WAIT or still bound by another socket. acquire_port() still returns PortInUsebind() returns EADDRINUSE. The kernel accepts the option (returns 0 from setsockopt) but does not honor it, silently violating POSIX SO_REUSEADDR semantics. Server restart patterns break.

Suggested fix: Either implement SO_REUSEADDR in acquire_port() (e.g. allow acquiring a port in TIME_WAIT state when the flag is set), or remove the set_reuse_address plumbing until the feature is actually implemented — accepting an option that does nothing is worse than rejecting it with ENOPROTOOPT.


7. write_to_sockaddr result silently discarded by fill_ip_endpoint and recvfrom

Files: kernel/kernel/src/net/types.rs:420-426, kernel/kernel/src/net/syscalls.rs:367-372

The PR changes write_to_sockaddr from returning () to Result<(), i32> (with a new null-pointer check). Two of three callers were not updated:

// types.rs:421  (SocketMsghdr::fill_ip_endpoint)
pub fn fill_ip_endpoint(&mut self, endpoint: IpEndpoint) {
    write_to_sockaddr(   // <-- Result<(), i32> silently dropped (unused_must_use warning)
        endpoint,
        self.msg_name.cast::<libc::sockaddr>(),
        &mut self.msg_namelen as *mut libc::socklen_t,
    );
}

// syscalls.rs:367  (recvfrom)
let _ = net::write_to_sockaddr(   // <-- explicitly discarded
    endpoint,
    address_ref as *mut libc::sockaddr,
    address_len_ref as *mut libc::socklen_t,
);

Only the new accept() caller at syscalls.rs:648 actually checks the Result.

Failure scenario: In the recvmsg() path, if msg_name or msg_namelen is null in the user's msghdr, write_to_sockaddr returns Err(-EINVAL) but fill_ip_endpoint swallows it. The caller proceeds as if the address was filled, but msg_name is left unwritten and msg_namelen is not updated. recvmsg returns success with uninitialized/garbage address data in the user's buffer. Compiles with an unused_must_use warning.

Suggested fix: Propagate the Result from fill_ip_endpoint and have recvfrom/recvmsg return -EINVAL on Err. Or, if the contract is "best-effort write, ignore failures," document that and use let _ = consistently (including in fill_ip_endpoint).


8. shutdown() clears is_listening before the operation completes — state diverges

File: kernel/kernel/src/net/connection.rs:273-286

pub fn shutdown(&self) -> ConnectionResult {
    self.is_listening.store(false, Ordering::Release);   // <-- cleared before enqueue
    let shutdown_task = Operation::Shutdown { ... };
    self.ipc_reply.queue_and_wait(shutdown_task)         // <-- may fail
}

If the Shutdown operation fails (e.g. NetStackQueueFull, or the smoltcp socket is in a bad state), is_listening is already false but the underlying smoltcp socket may still be in Listen state. The kernel-level flag and the smoltcp-level state diverge.

Failure scenario: Listener's shutdown() fails due to a full NETSTACK_QUEUE. is_listening is now false. The user retries accept() expecting it to work (the smoltcp socket is still listening), but syscalls::accept returns EINVAL ("socket is not listening") at syscalls.rs:615-618. Recovery requires calling listen() again, but TcpSocket::listen rejects is_active()/is_listening() sockets — the listener is wedged.

Suggested fix: Move self.is_listening.store(false, ...) to after queue_and_wait returns Ok, or clear it inside the Operation::Shutdown handler after the smoltcp socket is actually closed.


@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. We agree that the shared ipc_reply design has a real structural limitation, but we do not think the reported P0 scenario is reachable through the current application-level call path.

The actual ordering is:

  1. Thread A calls blocking accept().
  2. Connection::accept() sets reply_futex to STATE_WAITING_FOR_CONSUME.
  3. The network thread handles Operation::Accept, observes WouldBlock, registers the recv waker, and intentionally returns without waking the reply.
  4. Thread B calls shutdown() on the same fd.
  5. Connection::shutdown() calls queue_and_wait() before Operation::Shutdown is enqueued. Since the shared ipc_reply is still in STATE_WAITING_FOR_CONSUME, Thread B waits there indefinitely.

Therefore, in this scenario, Thread B never reaches TcpSocket::shutdown(). is_shutdown is never set, socket.close() is never called, and the SocketWaker::wake() early-return is not the cause of the deadlock described in the comment.

The underlying problem is broader than accept(). Blocking recv(), send(), recvfrom(), sendto(), etc. use the same WouldBlock + one-shot waker + shared OperationIPCReply pattern. The accept implementation exposes this existing architectural limitation because it adds another operation that can wait indefinitely, but it does not introduce the shared-reply design itself.

A correct fix requires a network-layer refactor, such as per-operation reply channels together with cancellation/generation handling for pending operations. Reordering the shutdown waker or forcibly resetting the shared futex in this PR would be incomplete and could introduce stale operations or deliver the wrong reply to a caller.

For this PR, the current supported usage should remain one in-flight operation per fd. In particular, applications should not concurrently operate on the same fd from multiple threads, especially accept() concurrently with shutdown(). We should document this limitation here and track the architectural fix in a separate PR, with coverage for all blocking socket operations.

The accepted-fd leak is still a consequence of the blocked unsupported-concurrency scenario, but it should be addressed as part of that broader cancellation/ownership redesign rather than as an accept-specific P0 fix in this PR.

@xuchang-vivo

xuchang-vivo commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I rechecked all eight findings against the current implementation and the actual operation/FD ownership transitions:

  1. Shared OperationIPCReply / accept + shutdown — The shared reply channel is a real architectural limitation for concurrent blocking operations on one fd. However, the exact P0 lost-wakeup sequence described is not reachable in the current code: Connection::shutdown() must first pass queue_and_wait(), which waits for STATE_IDLE before it enqueues Operation::Shutdown; a blocked accept() therefore prevents shutdown from reaching TcpSocket::shutdown() and changing the waker state. The caller can still wait indefinitely when multiple threads share an fd. This is tracked as the network-layer redesign issue net: shared OperationIPCReply can deadlock concurrent blocking I/O and shutdown #456; this PR keeps the existing one-in-flight-operation-per-fd contract.

  2. UDP sendto() ephemeral-port lease — Confirmed. The lease was acquired but not retained by the connection. Fixed in c7ed444 by retaining the lease for the socket lifetime.

  3. connect() lease socket type — Confirmed pre-existing bug. connect() used SockStream instead of the connection's actual socket type when acquiring an ephemeral port, which could mismatch release accounting. Fixed in 553bfcd.

  4. Accept rollback / duplicate handle — The reported P0 scenario is not reproducible with the current transaction. On an accept error, the allocated fd is freed; the accepted socket is inserted into NetworkManager only after smoltcp accept succeeds; and failed listener-handle replacement restores the original handle. Pending-accept state could still be modeled more explicitly, but it is a follow-up rather than a duplicate-fd leak in this PR.

  5. Accepted-socket flags/timeouts — Linux does not inherit O_NONBLOCK from the listening socket, and the accepted Connection here starts with blocking mode, matching that behavior. Timeout fields are not copied, but timeout enforcement is not implemented for the network operations globally, so this is a follow-up consistency issue rather than an accept-specific P1 regression.

  6. SO_REUSEADDR — The option was advertised without implementing its semantics. This PR now rejects both setsockopt() and getsockopt() for SO_REUSEADDR with ENOPROTOOPT until a real implementation is available (commit 1cc0b47).

  7. Ignored write_to_sockaddr() result — The ignored result in the receive callbacks is worth cleaning up, but it does not produce garbage output on valid calls: msg_name == NULL is legal when the caller does not request a source address, and the helper validates null pointers and bounds its copies. This is minor error-propagation cleanup, not a P1 correctness or memory-safety issue.

  8. Shutdown state/FD ordering — Confirmed and fixed in 1cc0b47: Connection::shutdown() clears is_listening only after a successful network-stack shutdown, and the explicit net::syscalls::shutdown() path releases the fd only after that success. The generic VFS close() lifecycle still frees the descriptor before invoking SocketFile::close() and needs a separate synchronized redesign; that is covered by the broader follow-up rather than changed here.

The branch now contains the fixes in c7ed444, 553bfcd, and 1cc0b47; @han-jiang277

@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

❌ Job failed. Failed jobs: build_and_check_boards (failure), see https://github.com/vivoblueos/kernel/actions/runs/32557384017.

@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

❌ Job failed. Failed jobs: build_and_check_boards (failure), see https://github.com/vivoblueos/kernel/actions/runs/32557723574.

@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

❌ Job failed. Failed jobs: build_and_check_boards (failure), see https://github.com/vivoblueos/kernel/actions/runs/32559206486.

@xuchang-vivo

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

✅ All jobs completed successfully, see https://github.com/vivoblueos/kernel/actions/runs/32561174321.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants