From owner-dev-commits-src-branches@freebsd.org Tue Sep 14 12:52:14 2021 Return-Path: Delivered-To: dev-commits-src-branches@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id A5726668B50; Tue, 14 Sep 2021 12:52:14 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4H83CL2Fzwz4hR5; Tue, 14 Sep 2021 12:52:14 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id EF9261B335; Tue, 14 Sep 2021 12:52:13 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 18ECqDwm016219; Tue, 14 Sep 2021 12:52:13 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 18ECqDlf016218; Tue, 14 Sep 2021 12:52:13 GMT (envelope-from git) Date: Tue, 14 Sep 2021 12:52:13 GMT Message-Id: <202109141252.18ECqDlf016218@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: 072901b7bc0b - stable/13 - sctp: Fix races around sctp_inpcb_free() MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: 072901b7bc0be6375940d06bf9e2fdce54a77a8e Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-branches@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commits to the stable branches of the FreeBSD src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Sep 2021 12:52:14 -0000 The branch stable/13 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=072901b7bc0be6375940d06bf9e2fdce54a77a8e commit 072901b7bc0be6375940d06bf9e2fdce54a77a8e Author: Mark Johnston AuthorDate: 2021-09-07 13:44:12 +0000 Commit: Mark Johnston CommitDate: 2021-09-14 12:51:35 +0000 sctp: Fix races around sctp_inpcb_free() sctp_close() and sctp_abort() disassociate the PCB from its socket. As a part of this, they attempt to free the PCB, which may end up lingering. Fix some bugs in this area: - For some reason, sctp_close() and sctp_abort() set SCTP_PCB_FLAGS_SOCKET_GONE using an atomic compare-and-set without the PCB lock held. This is racy since sctp_flags is normally updated without atomics, using the PCB lock to synchronize. So, the update can be lost, which can cause all sort of races with other SCTP components which look for the _GONE flag. Fix the problem simply by acquiring the PCB lock in order to set the flag. Note that we have to drop and re-acquire the lock again in sctp_inpcb_free(), but I don't see a good way around that for now. If it's a real problem, the _GONE flag could be split out of sctp_flags and into a dedicated sctp_inpcb field. - In sctp_inpcb_free(), load sctp_socket after acquiring the PCB lock, to avoid possible races with parallel sctp_inpcb_free() calls. - Add an assertion sctp_inpcb_free() to verify that _ALLGONE is not set. Reviewed by: tuexen Sponsored by: The FreeBSD Foundation (cherry picked from commit c17b531bedd10c7ebea08919fd73ee708ff37336) --- sys/netinet/sctp_pcb.c | 16 ++++++---------- sys/netinet/sctp_usrreq.c | 31 +++++++++++-------------------- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/sys/netinet/sctp_pcb.c b/sys/netinet/sctp_pcb.c index 3e517889d171..85ea5a3f8a53 100644 --- a/sys/netinet/sctp_pcb.c +++ b/sys/netinet/sctp_pcb.c @@ -3321,19 +3321,15 @@ sctp_inpcb_free(struct sctp_inpcb *inp, int immediate, int from) /* mark any iterators on the list or being processed */ sctp_iterator_inp_being_freed(inp); SCTP_ITERATOR_UNLOCK(); - so = inp->sctp_socket; - if (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) { - /* been here before.. eeks.. get out of here */ - SCTP_PRINTF("This conflict in free SHOULD not be happening! from %d, imm %d\n", from, immediate); -#ifdef SCTP_LOG_CLOSING - sctp_log_closing(inp, NULL, 1); -#endif - return; - } + SCTP_ASOC_CREATE_LOCK(inp); SCTP_INP_INFO_WLOCK(); - SCTP_INP_WLOCK(inp); + so = inp->sctp_socket; + KASSERT((inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) != 0, + ("%s: inp %p still has socket", __func__, inp)); + KASSERT((inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) == 0, + ("%s: double free of inp %p", __func__, inp)); if (from == SCTP_CALLED_AFTER_CMPSET_OFCLOSE) { inp->sctp_flags &= ~SCTP_PCB_FLAGS_CLOSE_IP; /* socket is gone, so no more wakeups allowed */ diff --git a/sys/netinet/sctp_usrreq.c b/sys/netinet/sctp_usrreq.c index 822a8ffb534f..62d6996ab60d 100644 --- a/sys/netinet/sctp_usrreq.c +++ b/sys/netinet/sctp_usrreq.c @@ -416,24 +416,23 @@ sctp_abort(struct socket *so) { struct epoch_tracker et; struct sctp_inpcb *inp; - uint32_t flags; inp = (struct sctp_inpcb *)so->so_pcb; if (inp == NULL) { return; } + SCTP_INP_WLOCK(inp); NET_EPOCH_ENTER(et); -sctp_must_try_again: - flags = inp->sctp_flags; #ifdef SCTP_LOG_CLOSING sctp_log_closing(inp, NULL, 17); #endif - if (((flags & SCTP_PCB_FLAGS_SOCKET_GONE) == 0) && - (atomic_cmpset_int(&inp->sctp_flags, flags, (flags | SCTP_PCB_FLAGS_SOCKET_GONE | SCTP_PCB_FLAGS_CLOSE_IP)))) { + if (((inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) == 0)) { + inp->sctp_flags |= SCTP_PCB_FLAGS_SOCKET_GONE | SCTP_PCB_FLAGS_CLOSE_IP; #ifdef SCTP_LOG_CLOSING sctp_log_closing(inp, NULL, 16); #endif + SCTP_INP_WUNLOCK(inp); sctp_inpcb_free(inp, SCTP_FREE_SHOULD_USE_ABORT, SCTP_CALLED_AFTER_CMPSET_OFCLOSE); SOCK_LOCK(so); @@ -448,13 +447,9 @@ sctp_must_try_again: so->so_pcb = NULL; SOCK_UNLOCK(so); } else { - flags = inp->sctp_flags; - if ((flags & SCTP_PCB_FLAGS_SOCKET_GONE) == 0) { - goto sctp_must_try_again; - } + SCTP_INP_WUNLOCK(inp); } NET_EPOCH_EXIT(et); - return; } static int @@ -516,7 +511,6 @@ sctp_close(struct socket *so) { struct epoch_tracker et; struct sctp_inpcb *inp; - uint32_t flags; inp = (struct sctp_inpcb *)so->so_pcb; if (inp == NULL) @@ -525,25 +519,26 @@ sctp_close(struct socket *so) /* * Inform all the lower layer assoc that we are done. */ + SCTP_INP_WLOCK(inp); NET_EPOCH_ENTER(et); -sctp_must_try_again: - flags = inp->sctp_flags; #ifdef SCTP_LOG_CLOSING sctp_log_closing(inp, NULL, 17); #endif - if (((flags & SCTP_PCB_FLAGS_SOCKET_GONE) == 0) && - (atomic_cmpset_int(&inp->sctp_flags, flags, (flags | SCTP_PCB_FLAGS_SOCKET_GONE | SCTP_PCB_FLAGS_CLOSE_IP)))) { + if ((inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) == 0) { + inp->sctp_flags |= SCTP_PCB_FLAGS_SOCKET_GONE | SCTP_PCB_FLAGS_CLOSE_IP; if (((so->so_options & SO_LINGER) && (so->so_linger == 0)) || (so->so_rcv.sb_cc > 0)) { #ifdef SCTP_LOG_CLOSING sctp_log_closing(inp, NULL, 13); #endif + SCTP_INP_WUNLOCK(inp); sctp_inpcb_free(inp, SCTP_FREE_SHOULD_USE_ABORT, SCTP_CALLED_AFTER_CMPSET_OFCLOSE); } else { #ifdef SCTP_LOG_CLOSING sctp_log_closing(inp, NULL, 14); #endif + SCTP_INP_WUNLOCK(inp); sctp_inpcb_free(inp, SCTP_FREE_SHOULD_USE_GRACEFUL_CLOSE, SCTP_CALLED_AFTER_CMPSET_OFCLOSE); } @@ -563,13 +558,9 @@ sctp_must_try_again: so->so_pcb = NULL; SOCK_UNLOCK(so); } else { - flags = inp->sctp_flags; - if ((flags & SCTP_PCB_FLAGS_SOCKET_GONE) == 0) { - goto sctp_must_try_again; - } + SCTP_INP_WUNLOCK(inp); } NET_EPOCH_EXIT(et); - return; } int