From owner-dev-commits-src-all@freebsd.org Fri Sep 17 19:12:37 2021 Return-Path: Delivered-To: dev-commits-src-all@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 67F0D6681C6; Fri, 17 Sep 2021 19:12:37 +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 4HB3Vs1BLYz3Q1r; Fri, 17 Sep 2021 19:12:37 +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 033071B491; Fri, 17 Sep 2021 19:12:37 +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 18HJCa1i068322; Fri, 17 Sep 2021 19:12:36 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 18HJCaPI068321; Fri, 17 Sep 2021 19:12:36 GMT (envelope-from git) Date: Fri, 17 Sep 2021 19:12:36 GMT Message-Id: <202109171912.18HJCaPI068321@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: ade1daa5c0d6 - main - socket: Synchronize soshutdown() with listen(2) and AIO 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/main X-Git-Reftype: branch X-Git-Commit: ade1daa5c0d66b9086f9066297ed984932b5e212 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Sep 2021 19:12:37 -0000 The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=ade1daa5c0d66b9086f9066297ed984932b5e212 commit ade1daa5c0d66b9086f9066297ed984932b5e212 Author: Mark Johnston AuthorDate: 2021-09-17 16:29:28 +0000 Commit: Mark Johnston CommitDate: 2021-09-17 18:19:06 +0000 socket: Synchronize soshutdown() with listen(2) and AIO To handle shutdown(SHUT_RD) we flush the receive buffer of the socket. This may involve searching for control messages of type SCM_RIGHTS, since we need to close the file references. Closing arbitrary files with socket buffer locks held is undesirable, mainly due to lock ordering issues, so we instead make a copy of the socket buffer and operate on that without any locks. Fields in the original buffer are cleared. This behaviour clobbered the AIO job queue associated with a receive buffer. It could also cause us to leak a KTLS session reference. Reorder socket buffer fields to address this. An alternate solution would be to remove the hack in sorflush(), but this is not quite feasible (yet). In particular, though sorflush() flags the sockbuf with SBS_CANTRCVMORE, it is possible for more data to be queued - the flag just prevents userspace from reading more data. I suspect we should fix this; SBS_CANTRCVMORE represents a terminal state and protocols can likely just drop any data destined for such a buffer. Many of them already do, but in some cases the check is racy, and some KPI churn will be needed to fix everything. This approach is more straightforward for now. Reported by: syzbot+104d8ee3430361cb2795@syzkaller.appspotmail.com Reported by: syzbot+5bd2e7d05f84a59d0d1b@syzkaller.appspotmail.com Reviewed by: jhb Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D31976 --- sys/kern/uipc_socket.c | 54 ++++++++++++++++++++++++++++++-------------------- sys/sys/sockbuf.h | 5 +++-- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c index c3f52a4640d3..1f999108dd71 100644 --- a/sys/kern/uipc_socket.c +++ b/sys/kern/uipc_socket.c @@ -2847,13 +2847,14 @@ soreceive(struct socket *so, struct sockaddr **psa, struct uio *uio, int soshutdown(struct socket *so, int how) { - struct protosw *pr = so->so_proto; + struct protosw *pr; int error, soerror_enotconn; if (!(how == SHUT_RD || how == SHUT_WR || how == SHUT_RDWR)) return (EINVAL); soerror_enotconn = 0; + SOCK_LOCK(so); if ((so->so_state & (SS_ISCONNECTED | SS_ISCONNECTING | SS_ISDISCONNECTING)) == 0) { /* @@ -2865,21 +2866,26 @@ soshutdown(struct socket *so, int how) * both backward-compatibility and POSIX requirements by forcing * ENOTCONN but still asking protocol to perform pru_shutdown(). */ - if (so->so_type != SOCK_DGRAM && !SOLISTENING(so)) + if (so->so_type != SOCK_DGRAM && !SOLISTENING(so)) { + SOCK_UNLOCK(so); return (ENOTCONN); + } soerror_enotconn = 1; } if (SOLISTENING(so)) { if (how != SHUT_WR) { - SOLISTEN_LOCK(so); so->so_error = ECONNABORTED; solisten_wakeup(so); /* unlocks so */ + } else { + SOCK_UNLOCK(so); } goto done; } + SOCK_UNLOCK(so); CURVNET_SET(so->so_vnet); + pr = so->so_proto; if (pr->pr_usrreqs->pru_flush != NULL) (*pr->pr_usrreqs->pru_flush)(so, how); if (how != SHUT_WR) @@ -2900,20 +2906,21 @@ done: void sorflush(struct socket *so) { - struct sockbuf *sb = &so->so_rcv; - struct protosw *pr = so->so_proto; struct socket aso; + struct protosw *pr; int error; VNET_SO_ASSERT(so); /* * In order to avoid calling dom_dispose with the socket buffer mutex - * held, and in order to generally avoid holding the lock for a long - * time, we make a copy of the socket buffer and clear the original - * (except locks, state). The new socket buffer copy won't have - * initialized locks so we can only call routines that won't use or - * assert those locks. + * held, we make a partial copy of the socket buffer and clear the + * original. The new socket buffer copy won't have initialized locks so + * we can only call routines that won't use or assert those locks. + * Ideally calling socantrcvmore() would prevent data from being added + * to the buffer, but currently it merely prevents buffered data from + * being read by userspace. We make this effort to free buffered data + * nonetheless. * * Dislodge threads currently blocked in receive and wait to acquire * a lock against other simultaneous readers before clearing the @@ -2921,28 +2928,31 @@ sorflush(struct socket *so) * despite any existing socket disposition on interruptable waiting. */ socantrcvmore(so); + error = SOCK_IO_RECV_LOCK(so, SBL_WAIT | SBL_NOINTR); - KASSERT(error == 0, ("%s: cannot lock sock %p recv buffer", - __func__, so)); + if (error != 0) { + KASSERT(SOLISTENING(so), + ("%s: soiolock(%p) failed", __func__, so)); + return; + } - /* - * Invalidate/clear most of the sockbuf structure, but leave selinfo - * and mutex data unchanged. - */ - SOCKBUF_LOCK(sb); + SOCK_RECVBUF_LOCK(so); bzero(&aso, sizeof(aso)); aso.so_pcb = so->so_pcb; - bcopy(&sb->sb_startzero, &aso.so_rcv.sb_startzero, - sizeof(*sb) - offsetof(struct sockbuf, sb_startzero)); - bzero(&sb->sb_startzero, - sizeof(*sb) - offsetof(struct sockbuf, sb_startzero)); - SOCKBUF_UNLOCK(sb); + bcopy(&so->so_rcv.sb_startzero, &aso.so_rcv.sb_startzero, + offsetof(struct sockbuf, sb_endzero) - + offsetof(struct sockbuf, sb_startzero)); + bzero(&so->so_rcv.sb_startzero, + offsetof(struct sockbuf, sb_endzero) - + offsetof(struct sockbuf, sb_startzero)); + SOCK_RECVBUF_UNLOCK(so); SOCK_IO_RECV_UNLOCK(so); /* * Dispose of special rights and flush the copied socket. Don't call * any unsafe routines (that rely on locks being initialized) on aso. */ + pr = so->so_proto; if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL) (*pr->pr_domain->dom_dispose)(&aso); sbrelease_internal(&aso.so_rcv, so); diff --git a/sys/sys/sockbuf.h b/sys/sys/sockbuf.h index 3fc9b10cd240..eb35a372cae5 100644 --- a/sys/sys/sockbuf.h +++ b/sys/sys/sockbuf.h @@ -104,12 +104,13 @@ struct sockbuf { u_int sb_tlsdcc; /* (a) TLS characters being decrypted */ int sb_lowat; /* (a) low water mark */ sbintime_t sb_timeo; /* (a) timeout for read/write */ - uint64_t sb_tls_seqno; /* (a) TLS seqno */ - struct ktls_session *sb_tls_info; /* (a + b) TLS state */ struct mbuf *sb_mtls; /* (a) TLS mbuf chain */ struct mbuf *sb_mtlstail; /* (a) last mbuf in TLS chain */ int (*sb_upcall)(struct socket *, void *, int); /* (a) */ void *sb_upcallarg; /* (a) */ +#define sb_endzero sb_tls_seqno + uint64_t sb_tls_seqno; /* (a) TLS seqno */ + struct ktls_session *sb_tls_info; /* (a + b) TLS state */ TAILQ_HEAD(, kaiocb) sb_aiojobq; /* (a) pending AIO ops */ struct task sb_aiotask; /* AIO task */ };