git: ade1daa5c0d6 - main - socket: Synchronize soshutdown() with listen(2) and AIO
Mark Johnston
markj at FreeBSD.org
Fri Sep 17 19:12:37 UTC 2021
The branch main has been updated by markj:
URL: https://cgit.FreeBSD.org/src/commit/?id=ade1daa5c0d66b9086f9066297ed984932b5e212
commit ade1daa5c0d66b9086f9066297ed984932b5e212
Author: Mark Johnston <markj at FreeBSD.org>
AuthorDate: 2021-09-17 16:29:28 +0000
Commit: Mark Johnston <markj at FreeBSD.org>
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 at syzkaller.appspotmail.com
Reported by: syzbot+5bd2e7d05f84a59d0d1b at 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 */
};
More information about the dev-commits-src-main
mailing list