Compare commits

..
Author SHA1 Message Date
LeencyandClaude Opus 5 1159103075 kernel/net: release the socket mutex before tcp_close in tcp_input
Check kernel codestyle / Check kernel codestyle (pull_request) Successful in 19s
Test PR / Build (es_ES) (pull_request) Successful in 2m50s
Test PR / Build (en_US) (pull_request) Successful in 2m54s
Test PR / Build (ru_RU) (pull_request) Successful in 2m55s
Summary: three paths in tcp_process_input called tcp_close/tcp_drop while
still holding the socket's mutex, which socket_free then tries to take
itself -- a self-deadlock of the TCP input thread, and, now that socket_free
also holds socket_mutex, a freeze of every socket syscall behind it.

Подробно:
tcp_process_input locks the socket's own mutex right after .found_socket and
keeps it for the whole state processing. Three of its paths reached
socket_free without releasing it:

- data arriving for a terminated process (tcp_close, then a reset reply);
- a new SYN arriving in TIME_WAIT (tcp_close, then restart at .findpcb);
- a SYN inside the window (tcp_drop, then .drop_with_reset).

socket_free has always locked the socket's mutex at its top, and the mutex is
not recursive, so these paths hung the TCP input thread the moment they were
taken. Since socket_free now also takes socket_mutex first, the hung thread
would additionally hold the list lock, stalling every socket syscall in every
process -- which is what turned this from a latent bug into one worth fixing
on the same branch.

Each site now unlocks the socket mutex first, the shape the file already uses
in .unlock_and_close and in the refused-connection path of .state_syn_sent.
The terminated-process path preserves edx across tcp_close because the reset
reply is built from the segment header; the TIME_WAIT path preserves ecx and
edx because .findpcb and everything after it still need the data count and the
header. The SYN-in-window path now leaves through .drop_no_socket instead of
.drop_with_reset: the old exit would have unlocked the freed socket's mutex
and built the reply out of freed memory, and the reply itself is redundant --
the connection is synchronized here, so tcp_drop already sends the RST via
tcp_output (tcp_outflags for TCPS_CLOSED is RST+ACK).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-07-30 16:46:41 +03:00
LeencyandClaude Opus 5 3051fae0ec kernel/net: serialize socket_free against walks of net_sockets
Summary: socket_free() unlinked a socket from net_sockets without holding
socket_mutex, so a socket freed by the TCP timers could rewrite the list while
tcp_process_input was walking it, leaving a garbage pointer that faulted in
ring 0 on the next incoming segment.

Подробно:
The header of socket_free already stated the rule -- "Caller should lock and
unlock socket_mutex" -- but no live caller obeyed it. socket_close, the
socket_pair out-of-memory paths, the DROPSOCKET path in tcp_input, and
tcp_close (reached from tcp_timer_640ms and from tcp_usreq) all called it with
the mutex free. The only caller that did hold it, socket_process_end, is
disabled by a `ret` at its first instruction.

Meanwhile tcp_process_input.findpcb walks net_sockets under socket_mutex and
dereferences whatever NextPtr it reads, and tcp_timer_160ms/640ms walk the same
list with no lock at all -- the second of them calling tcp_close, hence
socket_free, from inside the walk. The mutex therefore protected nothing: one
side honoured it, the other rewrote the list underneath.

Observed as a kernel page fault inside tcp_process_input with EBX = 0x0000047A,
where ebx is by contract the socket pointer taken from the list walk. It was
preceded by a flood of "SOCKET_notify: error finding thread", i.e. an
application had died leaving sockets behind -- socket_process_end being
disabled, those stay on net_sockets, go to FIN_WAIT/TIME_WAIT and are then
closed by the timers, which is exactly the unlocked socket_free path, at the
same time as traffic keeps arriving.

The mutex is acquired at the top of socket_free rather than around the unlink
alone. socket_alloc takes socket_mutex first and the neighbour's SOCKET.mutex
second; wrapping only the unlink would have acquired them in the opposite
order, since SOCKET.mutex is taken earlier in this function, and deadlocked
against socket_alloc. Taking it first also covers the socket_check walk at the
top, which reads net_sockets as well. No live caller holds socket_mutex, and
the mutex is not recursive, so no nesting is introduced; socket_process_end
gets a note explaining what its dead body would have to change before it can be
revived.

Not addressed here: the timer walks themselves still run unlocked, so two
concurrent walks are still unserialized. Holding socket_mutex across
tcp_timer_640ms would span tcp_output, tcp_disconnect and tcp_close, which is
the deadlock class that got socket_process_end disabled in the first place, and
belongs in its own change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-07-30 16:39:49 +03:00
2 changed files with 66 additions and 2 deletions

No files matched your search

+29 -1
View File
@@ -2098,7 +2098,7 @@ socket_alloc:
;-----------------------------------------------------------------;
; ;
; socket_free: Free socket data memory and remove socket from ;
; the list. Caller should lock and unlock socket_mutex. ;
; the list. Takes socket_mutex itself; callers must not hold it. ;
; ;
; IN: eax = socket ptr ;
; ;
@@ -2110,6 +2110,22 @@ socket_free:
DEBUGF DEBUG_NETWORK_VERBOSE, "SOCKET_free: %x\n", eax
; Unlinking below writes NextPtr/PrevPtr of the neighbours, so it has to be
; serialized against every walk of net_sockets -- tcp_process_input.findpcb
; above all, which walks the list under socket_mutex and dereferences whatever
; NextPtr it reads. Nothing used to hold the mutex here, so a socket freed by
; the TCP timers could rewrite the list from under that walk.
;
; The mutex is taken here, at the very top, and not merely around the unlink:
; socket_alloc locks socket_mutex and then the neighbour's SOCKET.mutex, so
; taking them in the opposite order -- SOCKET.mutex first, as the code below
; does -- would be an ABBA deadlock against it. Locking here also covers the
; socket_check walk, which reads net_sockets too.
pusha
mov ecx, socket_mutex
call mutex_lock
popa
call socket_check
jz .error
@@ -2157,7 +2173,13 @@ socket_free:
DEBUGF DEBUG_NETWORK_VERBOSE, "SOCKET_free: success!\n"
; Both exits converge here: the success path falls through, and a failed
; socket_check jumps in.
.error:
pusha
mov ecx, socket_mutex
call mutex_unlock
popa
ret
.error1:
@@ -2381,6 +2403,12 @@ socket_process_end:
ret ; FIXME
; NOTE: the body below is disabled by the ret above, and cannot be re-enabled
; as it stands: it holds socket_mutex across tcp_disconnect and socket_free,
; and socket_free now takes that mutex itself. Whoever revives this has to drop
; the lock around those calls (or hand the work to another thread, as the TODO
; below suggests), not put the lock back into socket_free -- the ordering there
; is what keeps it from deadlocking against socket_alloc.
cmp [net_sockets + SOCKET.NextPtr], 0 ; Are there any active sockets at all?
je .quickret ; nope, exit immediately
+37 -1
View File
@@ -726,8 +726,20 @@ endl
test ecx, ecx
jz .not_terminated
; tcp_close ends in socket_free, which locks the socket's own mutex (held here
; since .found_socket) and socket_mutex: release ours first or deadlock. Same
; shape as .unlock_and_close. The reset reply below only needs the segment
; header, so edx is preserved across the call and the socket is never touched
; again.
pusha
lea ecx, [ebx + SOCKET.mutex]
call mutex_unlock
popa
mov eax, ebx
push edx
call tcp_close
pop edx
inc [TCPS_rcvafterclose]
jmp .respond_seg_reset
.not_terminated:
@@ -761,8 +773,20 @@ endl
; mov edx, [ebx + TCP_SOCKET.RCV_NXT]
; cmp edx, [edx + TCP_header.SequenceNumber]
; add edx, 64000 ; TCP_ISSINCR FIXME
; tcp_close ends in socket_free, which locks the socket's own mutex (held here
; since .found_socket) and socket_mutex: release ours first or deadlock. The
; segment header (edx) and data count (ecx) survive the call because .findpcb
; and everything after it still need them; the freed socket is off the list by
; the time the walk restarts.
pusha
lea ecx, [ebx + SOCKET.mutex]
call mutex_unlock
popa
mov eax, ebx
push ecx edx
call tcp_close
pop edx ecx
jmp .findpcb ; FIXME: skip code for unscaling window, ...
.no_new_request:
@@ -881,10 +905,22 @@ endl
test [edx + TCP_header.Flags], TH_SYN
jz .not_syn_full
; tcp_drop ends in tcp_close -> socket_free, which locks the socket's own mutex
; (held here since .found_socket) and socket_mutex: release ours first or
; deadlock. Same shape as the refused-connection path in .state_syn_sent.
; Exiting through .drop_with_reset afterwards would unlock the freed mutex and
; build the reply from the freed socket; it is also redundant -- the connection
; is synchronized here, so tcp_drop itself sends the RST via tcp_output
; (tcp_outflags for TCPS_CLOSED is RST+ACK). Leave through .drop_no_socket.
pusha
lea ecx, [ebx + SOCKET.mutex]
call mutex_unlock
popa
mov eax, ebx
mov ebx, ECONNRESET
call tcp_drop
jmp .drop_with_reset
jmp .drop_no_socket
.not_syn_full:
; If ACK bit is off, we drop the segment and return