Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 23 Dec 2024 18:36:26 GMT
From:      Gleb Smirnoff <glebius@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: 053a98849734 - main - tcp: don't ever return ECONNRESET on close(2)
Message-ID:  <202412231836.4BNIaQHO028758@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch main has been updated by glebius:

URL: https://cgit.FreeBSD.org/src/commit/?id=053a988497342a6fd0a717cc097d09c23f83e103

commit 053a988497342a6fd0a717cc097d09c23f83e103
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2024-12-23 18:35:49 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2024-12-23 18:35:49 +0000

    tcp: don't ever return ECONNRESET on close(2)
    
    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 21367f630d72), and this EINVAL was
    added as a safety measure in 623dce13c64ef.  After conversion to
    ECONNRESET it had been documented in the manual page in 78e3a7fdd51e6, 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
---
 lib/libsys/close.2       | 5 +----
 sys/netinet/tcp_usrreq.c | 5 -----
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/lib/libsys/close.2 b/lib/libsys/close.2
index 83f8604a4a22..91a7a902d70d 100644
--- a/lib/libsys/close.2
+++ b/lib/libsys/close.2
@@ -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
@@ -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
diff --git a/sys/netinet/tcp_usrreq.c b/sys/netinet/tcp_usrreq.c
index 35578b348c9f..acc3e2ea2942 100644
--- a/sys/netinet/tcp_usrreq.c
+++ b/sys/netinet/tcp_usrreq.c
@@ -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)



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202412231836.4BNIaQHO028758>