Skip to content

Commit

Permalink
tcp: don't ever return ECONNRESET on close(2)
Browse files Browse the repository at this point in the history
The SUS doesn't mention this error code as a possible one [1]. The FreeBSD
manual page specifies a possible ECONNRESET for close(2):

[ECONNRESET]	The underlying object was a stream socket that was
		shut down by the peer before all pending data was
		delivered.

In the past it had been EINVAL (see 21367f6), and this EINVAL was
added as a safety measure in 623dce1.  After conversion to
ECONNRESET it had been documented in the manual page in 78e3a7f, but
I bet wasn't ever tested to actually be ever returned, cause the
tcp-testsuite[2] didn't exist back then.  So documentation is incorrect
since 2006, if my bet wins.  Anyway, in the modern FreeBSD the condition
described above doesn't end up with ECONNRESET error code from close(2).
The error condition is reported via SO_ERROR socket option, though.  This
can be checked using the tcp-testsuite, temporarily disabling the
getsockopt(SO_ERROR) lines using sed command [3].  Most of these
getsockopt(2)s are followed by '+0.00 close(3) = 0', which will confirm
that close(2) doesn't return ECONNRESET even on a socket that has the
error stored, neither it is returned in the case described in the manual
page.  The latter case is covered by multiple tests residing in tcp-
testsuite/state-event-engine/rcv-rst-*.

However, the deleted block of code could be entered in a race condition
between close(2) and processing of incoming packet, when connection had
already been half-closed with shutdown(SHUT_WR) and sits in TCPS_LAST_ACK.
This was reported in the bug 146845.  With the block deleted, we will
continue into tcp_disconnect() which has proper handling of INP_DROPPED.

The race explanation follows.  The connection is in TCPS_LAST_ACK.  The
network input thread acquires the tcpcb lock first, sets INP_DROPPED,
acquires the socket lock in soisdisconnected() and clears SS_ISCONNECTED.
Meanwhile, the syscall thread goes through sodisconnect() which checks for
SS_ISCONNECTED locklessly(!).  The check passes and the thread blocks on
the tcpcb lock in tcp_usr_disconnect().  Once input thread releases the
lock, the syscall thread observes INP_DROPPED and returns ECONNRESET.

- Thread 1: tcp_do_segment()->tcp_close()->in_pcbdrop(),soisdisconnected()
- Thread 2: sys_close()...->soclose()->sodisconnect()->tcp_usr_disconnect()

Note that the lockless operation in sodisconnect() isn't correct, but
enforcing the socket lock there will not fix the problem.

[1] https://pubs.opengroup.org/onlinepubs/9799919799/
[2] https://github.com/freebsd-net/tcp-testsuite
[3] sed -i "" -Ee '/\+0\.00 getsockopt\(3, SOL_SOCKET, SO_ERROR, \[ECONNRESET\]/d' $(grep -lr ECONNRESET tcp-testsuite)

PR:			146845
Reviewed by:		tuexen, rrs, imp
Differential Revision:	https://reviews.freebsd.org/D48148
  • Loading branch information
glebius committed Dec 23, 2024
1 parent 893839b commit 053a988
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 9 deletions.
5 changes: 1 addition & 4 deletions lib/libsys/close.2
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
.\" SUCH DAMAGE.
.\"
.Dd December 1, 2017
.Dd December 18, 2024
.Dt CLOSE 2
.Os
.Sh NAME
Expand Down Expand Up @@ -111,9 +111,6 @@ is not an active descriptor.
An interrupt was received.
.It Bq Er ENOSPC
The underlying object did not fit, cached data was lost.
.It Bq Er ECONNRESET
The underlying object was a stream socket that was shut down by the peer
before all pending data was delivered.
.El
.Pp
In case of any error except
Expand Down
5 changes: 0 additions & 5 deletions sys/netinet/tcp_usrreq.c
Original file line number Diff line number Diff line change
Expand Up @@ -690,11 +690,6 @@ tcp_usr_disconnect(struct socket *so)
inp = sotoinpcb(so);
KASSERT(inp != NULL, ("tcp_usr_disconnect: inp == NULL"));
INP_WLOCK(inp);
if (inp->inp_flags & INP_DROPPED) {
INP_WUNLOCK(inp);
NET_EPOCH_EXIT(et);
return (ECONNRESET);
}
tp = intotcpcb(inp);

if (tp->t_state == TCPS_TIME_WAIT)
Expand Down

0 comments on commit 053a988

Please sign in to comment.