git: aa16c2fcaf1d - stable/13 - sctp: Tighten up locking around sctp_aloc_assoc()
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 22 Feb 2022 22:48:16 UTC
The branch stable/13 has been updated by tuexen:
URL: https://cgit.FreeBSD.org/src/commit/?id=aa16c2fcaf1d5e541fc91a66bbba352ba313429c
commit aa16c2fcaf1d5e541fc91a66bbba352ba313429c
Author: Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2021-09-11 14:15:21 +0000
Commit: Michael Tuexen <tuexen@FreeBSD.org>
CommitDate: 2022-02-22 22:47:28 +0000
sctp: Tighten up locking around sctp_aloc_assoc()
All callers of sctp_aloc_assoc() mark the PCB as connected after a
successful call (for one-to-one-style sockets). In all cases this is
done without the PCB lock, so the PCB's flags can be corrupted. We also
do not atomically check whether a one-to-one-style socket is a listening
socket, which violates various assumptions in solisten_proto().
We need to hold the PCB lock across all of sctp_aloc_assoc() to fix
this. In order to do that without introducing lock order reversals, we
have to hold the global info lock as well.
So:
- Convert sctp_aloc_assoc() so that the inp and info locks are
consistently held. It returns with the association lock held, as
before.
- Fix an apparent bug where we failed to remove an association from a
global hash if sctp_add_remote_addr() fails.
- sctp_select_a_tag() is called when initializing an association, and it
acquires the global info lock. To avoid lock recursion, push locking
into its callers.
- Introduce sctp_aloc_assoc_connected(), which atomically checks for a
listening socket and sets SCTP_PCB_FLAGS_CONNECTED.
There is still one edge case in sctp_process_cookie_new() where we do
not update PCB/socket state correctly.
Reviewed by: tuexen
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D31908
(cherry picked from commit 2d5c48eccd9f29b9df8020bb1c3a8ffda38df37b)
---
sys/netinet/sctp_input.c | 4 ++
sys/netinet/sctp_output.c | 14 +++----
sys/netinet/sctp_pcb.c | 96 +++++++++++++++++++++++++++++----------------
sys/netinet/sctp_pcb.h | 4 ++
sys/netinet/sctp_usrreq.c | 14 +------
sys/netinet6/sctp6_usrreq.c | 7 +---
6 files changed, 78 insertions(+), 61 deletions(-)
diff --git a/sys/netinet/sctp_input.c b/sys/netinet/sctp_input.c
index b257a62e02f8..84d2a303303d 100644
--- a/sys/netinet/sctp_input.c
+++ b/sys/netinet/sctp_input.c
@@ -679,7 +679,9 @@ sctp_handle_nat_colliding_state(struct sctp_tcb *stcb)
if ((SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_WAIT) ||
(SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_ECHOED)) {
+ SCTP_INP_INFO_RLOCK();
new_vtag = sctp_select_a_tag(stcb->sctp_ep, stcb->sctp_ep->sctp_lport, stcb->rport, 1);
+ SCTP_INP_INFO_RLOCK();
atomic_add_int(&stcb->asoc.refcnt, 1);
SCTP_TCB_UNLOCK(stcb);
SCTP_INP_INFO_WLOCK();
@@ -2183,6 +2185,8 @@ sctp_process_cookie_new(struct mbuf *m, int iphlen, int offset,
* INIT/INIT-ACK/COOKIE arrived. But of course then it
* should have went to the other code.. not here.. oh well..
* a bit of protection is worth having..
+ *
+ * XXXMJ unlocked
*/
stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED;
soisconnected(stcb->sctp_socket);
diff --git a/sys/netinet/sctp_output.c b/sys/netinet/sctp_output.c
index ce08c8139015..99b1b2660573 100644
--- a/sys/netinet/sctp_output.c
+++ b/sys/netinet/sctp_output.c
@@ -5817,7 +5817,9 @@ do_a_abort:
atomic_add_int(&asoc->refcnt, 1);
SCTP_TCB_UNLOCK(stcb);
new_tag:
+ SCTP_INP_INFO_RLOCK();
vtag = sctp_select_a_tag(inp, inp->sctp_lport, sh->src_port, 1);
+ SCTP_INP_INFO_RUNLOCK();
if ((asoc->peer_supports_nat) && (vtag == asoc->my_vtag)) {
/*
* Got a duplicate vtag on some guy behind a
@@ -5834,7 +5836,9 @@ do_a_abort:
} else {
SCTP_INP_INCR_REF(inp);
SCTP_INP_RUNLOCK(inp);
+ SCTP_INP_INFO_RLOCK();
vtag = sctp_select_a_tag(inp, inp->sctp_lport, sh->src_port, 1);
+ SCTP_INP_INFO_RUNLOCK();
initack->init.initiate_tag = htonl(vtag);
/* get a TSN to use too */
initack->init.initial_tsn = htonl(sctp_select_initial_TSN(&inp->sctp_ep));
@@ -12722,7 +12726,7 @@ sctp_lower_sosend(struct socket *so,
panic("Error, should hold create lock and I don't?");
}
#endif
- stcb = sctp_aloc_assoc(inp, addr, &error, 0, 0, vrf_id,
+ stcb = sctp_aloc_assoc_connected(inp, addr, &error, 0, 0, vrf_id,
inp->sctp_ep.pre_open_stream_count,
inp->sctp_ep.port,
p,
@@ -12731,14 +12735,6 @@ sctp_lower_sosend(struct socket *so,
/* Error is setup for us in the call */
goto out_unlocked;
}
- if (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) {
- stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED;
- /*
- * Set the connected flag so we can queue
- * data
- */
- soisconnecting(so);
- }
hold_tcblock = 1;
if (create_lock_applied) {
SCTP_ASOC_CREATE_UNLOCK(inp);
diff --git a/sys/netinet/sctp_pcb.c b/sys/netinet/sctp_pcb.c
index b5cb9bb04c11..5bbd254e3d25 100644
--- a/sys/netinet/sctp_pcb.c
+++ b/sys/netinet/sctp_pcb.c
@@ -1546,10 +1546,6 @@ sctp_findasoc_ep_asocid_locked(struct sctp_inpcb *inp, sctp_assoc_t asoc_id, int
struct sctp_tcb *stcb;
uint32_t id;
- if (inp == NULL) {
- SCTP_PRINTF("TSNH ep_associd\n");
- return (NULL);
- }
if (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) {
SCTP_PRINTF("TSNH ep_associd0\n");
return (NULL);
@@ -4169,8 +4165,8 @@ try_again:
* careful to add all additional addresses once they are know right away or
* else the assoc will be may experience a blackout scenario.
*/
-struct sctp_tcb *
-sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
+static struct sctp_tcb *
+sctp_aloc_assoc_locked(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
int *error, uint32_t override_tag, uint32_t initial_tsn,
uint32_t vrf_id, uint16_t o_streams, uint16_t port,
struct thread *p,
@@ -4184,6 +4180,9 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
uint16_t rport;
int err;
+ SCTP_INP_INFO_WLOCK_ASSERT();
+ SCTP_INP_WLOCK_ASSERT(inp);
+
/*
* Assumption made here: Caller has done a
* sctp_findassociation_ep_addr(ep, addr's); to make sure the
@@ -4200,7 +4199,11 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
*error = EINVAL;
return (NULL);
}
- SCTP_INP_RLOCK(inp);
+ if (inp->sctp_flags & (SCTP_PCB_FLAGS_SOCKET_GONE | SCTP_PCB_FLAGS_SOCKET_ALLGONE)) {
+ SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
+ *error = EINVAL;
+ return (NULL);
+ }
if ((inp->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL) &&
((sctp_is_feature_off(inp, SCTP_PCB_FLAGS_PORTREUSE)) ||
(inp->sctp_flags & SCTP_PCB_FLAGS_CONNECTED))) {
@@ -4210,7 +4213,6 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
* sctp_aloc_assoc.. or the one-2-many socket. If a peeled
* off, or connected one does this.. its an error.
*/
- SCTP_INP_RUNLOCK(inp);
SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
*error = EINVAL;
return (NULL);
@@ -4219,7 +4221,6 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
(inp->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE)) {
if ((inp->sctp_flags & SCTP_PCB_FLAGS_WAS_CONNECTED) ||
(inp->sctp_flags & SCTP_PCB_FLAGS_WAS_ABORTED)) {
- SCTP_INP_RUNLOCK(inp);
SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
*error = EINVAL;
return (NULL);
@@ -4263,7 +4264,6 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
(((inp->sctp_flags & SCTP_PCB_FLAGS_BOUND_V6) != 0) &&
(SCTP_IPV6_V6ONLY(inp) != 0))) {
/* Invalid address */
- SCTP_INP_RUNLOCK(inp);
SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
*error = EINVAL;
return (NULL);
@@ -4283,7 +4283,6 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
IN6_IS_ADDR_MULTICAST(&sin6->sin6_addr) ||
((inp->sctp_flags & SCTP_PCB_FLAGS_BOUND_V6) == 0)) {
/* Invalid address */
- SCTP_INP_RUNLOCK(inp);
SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
*error = EINVAL;
return (NULL);
@@ -4294,18 +4293,16 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
#endif
default:
/* not supported family type */
- SCTP_INP_RUNLOCK(inp);
SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
*error = EINVAL;
return (NULL);
}
- SCTP_INP_RUNLOCK(inp);
if (inp->sctp_flags & SCTP_PCB_FLAGS_UNBOUND) {
/*
* If you have not performed a bind, then we need to do the
* ephemeral bind for you.
*/
- if ((err = sctp_inpcb_bind(inp->sctp_socket, NULL, NULL, p))) {
+ if ((err = sctp_inpcb_bind_locked(inp, NULL, NULL, p))) {
/* bind error, probably perm */
*error = err;
return (NULL);
@@ -4338,21 +4335,6 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
*error = err;
return (NULL);
}
- /* and the port */
- SCTP_INP_INFO_WLOCK();
- SCTP_INP_WLOCK(inp);
- if (inp->sctp_flags & (SCTP_PCB_FLAGS_SOCKET_GONE | SCTP_PCB_FLAGS_SOCKET_ALLGONE)) {
- /* inpcb freed while alloc going on */
- SCTP_TCB_LOCK_DESTROY(stcb);
- SCTP_TCB_SEND_LOCK_DESTROY(stcb);
- SCTP_ZONE_FREE(SCTP_BASE_INFO(ipi_zone_asoc), stcb);
- SCTP_INP_WUNLOCK(inp);
- SCTP_INP_INFO_WUNLOCK();
- SCTP_DECR_ASOC_COUNT();
- SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL);
- *error = EINVAL;
- return (NULL);
- }
SCTP_TCB_LOCK(stcb);
asoc->assoc_id = sctp_aloc_a_assoc_id(inp, stcb);
@@ -4360,7 +4342,6 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
head = &SCTP_BASE_INFO(sctp_asochash)[SCTP_PCBHASH_ASOC(stcb->asoc.my_vtag, SCTP_BASE_INFO(hashasocmark))];
/* put it in the bucket in the vtag hash of assoc's for the system */
LIST_INSERT_HEAD(head, stcb, sctp_asocs);
- SCTP_INP_INFO_WUNLOCK();
if (sctp_add_remote_addr(stcb, firstaddr, NULL, port, SCTP_DO_SETSCOPE, SCTP_ALLOC_ASOC)) {
/* failure.. memory error? */
@@ -4380,6 +4361,7 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
SCTP_TCB_UNLOCK(stcb);
SCTP_TCB_LOCK_DESTROY(stcb);
SCTP_TCB_SEND_LOCK_DESTROY(stcb);
+ LIST_REMOVE(stcb, sctp_asocs);
LIST_REMOVE(stcb, sctp_tcbasocidhash);
SCTP_ZONE_FREE(SCTP_BASE_INFO(ipi_zone_asoc), stcb);
SCTP_INP_WUNLOCK(inp);
@@ -4405,11 +4387,57 @@ sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
if (initialize_auth_params == SCTP_INITIALIZE_AUTH_PARAMS) {
sctp_initialize_auth_params(inp, stcb);
}
- SCTP_INP_WUNLOCK(inp);
SCTPDBG(SCTP_DEBUG_PCB1, "Association %p now allocated\n", (void *)stcb);
return (stcb);
}
+struct sctp_tcb *
+sctp_aloc_assoc(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
+ int *error, uint32_t override_tag, uint32_t initial_tsn,
+ uint32_t vrf_id, uint16_t o_streams, uint16_t port,
+ struct thread *p,
+ int initialize_auth_params)
+{
+ struct sctp_tcb *stcb;
+
+ SCTP_INP_INFO_WLOCK();
+ SCTP_INP_WLOCK(inp);
+ stcb = sctp_aloc_assoc_locked(inp, firstaddr, error, override_tag,
+ initial_tsn, vrf_id, o_streams, port, p, initialize_auth_params);
+ SCTP_INP_INFO_WUNLOCK();
+ SCTP_INP_WUNLOCK(inp);
+ return (stcb);
+}
+
+struct sctp_tcb *
+sctp_aloc_assoc_connected(struct sctp_inpcb *inp, struct sockaddr *firstaddr,
+ int *error, uint32_t override_tag, uint32_t initial_tsn,
+ uint32_t vrf_id, uint16_t o_streams, uint16_t port,
+ struct thread *p,
+ int initialize_auth_params)
+{
+ struct sctp_tcb *stcb;
+
+ SCTP_INP_INFO_WLOCK();
+ SCTP_INP_WLOCK(inp);
+ if ((inp->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) &&
+ SCTP_IS_LISTENING(inp)) {
+ SCTP_INP_INFO_WUNLOCK();
+ SCTP_INP_WUNLOCK(inp);
+ *error = EINVAL;
+ return (NULL);
+ }
+ stcb = sctp_aloc_assoc_locked(inp, firstaddr, error, override_tag,
+ initial_tsn, vrf_id, o_streams, port, p, initialize_auth_params);
+ SCTP_INP_INFO_WUNLOCK();
+ if (stcb != NULL && (inp->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE)) {
+ inp->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED;
+ soisconnecting(inp->sctp_socket);
+ }
+ SCTP_INP_WUNLOCK(inp);
+ return (stcb);
+}
+
void
sctp_remove_net(struct sctp_tcb *stcb, struct sctp_nets *net)
{
@@ -4523,7 +4551,7 @@ sctp_is_in_timewait(uint32_t tag, uint16_t lport, uint16_t rport, uint32_t now)
struct sctp_tagblock *twait_block;
int i;
- SCTP_INP_INFO_RLOCK_ASSERT();
+ SCTP_INP_INFO_LOCK_ASSERT();
chain = &SCTP_BASE_INFO(vtag_timewait)[(tag % SCTP_STACK_VTAG_HASH_SIZE)];
LIST_FOREACH(twait_block, chain, sctp_nxt_tagblock) {
for (i = 0; i < SCTP_NUMBER_IN_VTAG_BLOCK; i++) {
@@ -6690,7 +6718,8 @@ sctp_is_vtag_good(uint32_t tag, uint16_t lport, uint16_t rport, struct timeval *
struct sctp_tcb *stcb;
bool result;
- SCTP_INP_INFO_RLOCK();
+ SCTP_INP_INFO_LOCK_ASSERT();
+
head = &SCTP_BASE_INFO(sctp_asochash)[SCTP_PCBHASH_ASOC(tag, SCTP_BASE_INFO(hashasocmark))];
LIST_FOREACH(stcb, head, sctp_asocs) {
/*
@@ -6717,7 +6746,6 @@ sctp_is_vtag_good(uint32_t tag, uint16_t lport, uint16_t rport, struct timeval *
}
result = !sctp_is_in_timewait(tag, lport, rport, (uint32_t)now->tv_sec);
out:
- SCTP_INP_INFO_RUNLOCK();
return (result);
}
diff --git a/sys/netinet/sctp_pcb.h b/sys/netinet/sctp_pcb.h
index a3d0b489eb82..adbecf9bd00f 100644
--- a/sys/netinet/sctp_pcb.h
+++ b/sys/netinet/sctp_pcb.h
@@ -577,6 +577,10 @@ struct sctp_tcb *
sctp_aloc_assoc(struct sctp_inpcb *, struct sockaddr *,
int *, uint32_t, uint32_t, uint32_t, uint16_t, uint16_t,
struct thread *, int);
+struct sctp_tcb *
+sctp_aloc_assoc_connected(struct sctp_inpcb *, struct sockaddr *,
+ int *, uint32_t, uint32_t, uint32_t, uint16_t, uint16_t,
+ struct thread *, int);
int sctp_free_assoc(struct sctp_inpcb *, struct sctp_tcb *, int, int);
diff --git a/sys/netinet/sctp_usrreq.c b/sys/netinet/sctp_usrreq.c
index c7cbc9fc682c..7a9716716069 100644
--- a/sys/netinet/sctp_usrreq.c
+++ b/sys/netinet/sctp_usrreq.c
@@ -1417,7 +1417,7 @@ sctp_do_connect_x(struct socket *so, struct sctp_inpcb *inp, void *optval,
vrf_id = inp->def_vrf_id;
/* We are GOOD to go */
- stcb = sctp_aloc_assoc(inp, sa, &error, 0, 0, vrf_id,
+ stcb = sctp_aloc_assoc_connected(inp, sa, &error, 0, 0, vrf_id,
inp->sctp_ep.pre_open_stream_count,
inp->sctp_ep.port,
(struct thread *)p,
@@ -1426,11 +1426,6 @@ sctp_do_connect_x(struct socket *so, struct sctp_inpcb *inp, void *optval,
/* Gak! no memory */
goto out_now;
}
- if (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) {
- stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED;
- /* Set the connected flag so we can queue data */
- soisconnecting(so);
- }
SCTP_SET_STATE(stcb, SCTP_STATE_COOKIE_WAIT);
/* move to second address */
switch (sa->sa_family) {
@@ -7084,7 +7079,7 @@ sctp_connect(struct socket *so, struct sockaddr *addr, struct thread *p)
vrf_id = inp->def_vrf_id;
/* We are GOOD to go */
- stcb = sctp_aloc_assoc(inp, addr, &error, 0, 0, vrf_id,
+ stcb = sctp_aloc_assoc_connected(inp, addr, &error, 0, 0, vrf_id,
inp->sctp_ep.pre_open_stream_count,
inp->sctp_ep.port, p,
SCTP_INITIALIZE_AUTH_PARAMS);
@@ -7092,11 +7087,6 @@ sctp_connect(struct socket *so, struct sockaddr *addr, struct thread *p)
/* Gak! no memory */
goto out_now;
}
- if (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) {
- stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED;
- /* Set the connected flag so we can queue data */
- soisconnecting(so);
- }
SCTP_SET_STATE(stcb, SCTP_STATE_COOKIE_WAIT);
(void)SCTP_GETTIME_TIMEVAL(&stcb->asoc.time_entered);
diff --git a/sys/netinet6/sctp6_usrreq.c b/sys/netinet6/sctp6_usrreq.c
index fcbb566b53d3..516107caf408 100644
--- a/sys/netinet6/sctp6_usrreq.c
+++ b/sys/netinet6/sctp6_usrreq.c
@@ -945,7 +945,7 @@ sctp6_connect(struct socket *so, struct sockaddr *addr, struct thread *p)
return (EALREADY);
}
/* We are GOOD to go */
- stcb = sctp_aloc_assoc(inp, addr, &error, 0, 0, vrf_id,
+ stcb = sctp_aloc_assoc_connected(inp, addr, &error, 0, 0, vrf_id,
inp->sctp_ep.pre_open_stream_count,
inp->sctp_ep.port, p,
SCTP_INITIALIZE_AUTH_PARAMS);
@@ -954,11 +954,6 @@ sctp6_connect(struct socket *so, struct sockaddr *addr, struct thread *p)
/* Gak! no memory */
return (error);
}
- if (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) {
- stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED;
- /* Set the connected flag so we can queue data */
- soisconnecting(so);
- }
SCTP_SET_STATE(stcb, SCTP_STATE_COOKIE_WAIT);
(void)SCTP_GETTIME_TIMEVAL(&stcb->asoc.time_entered);
NET_EPOCH_ENTER(et);