Date: Sat, 11 Sep 2021 14:16:21 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 2d5c48eccd9f - main - sctp: Tighten up locking around sctp_aloc_assoc() Message-ID: <202109111416.18BEGLqm071532@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=2d5c48eccd9f29b9df8020bb1c3a8ffda38df37b commit 2d5c48eccd9f29b9df8020bb1c3a8ffda38df37b Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2021-09-11 14:15:21 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2021-09-11 14:15:21 +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 MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D31908 --- 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 34f30799ec79..9e194d5fda78 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 6ae35ae2deba..35a834438895 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 6ccd4ddf96b7..a33acce4380b 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); @@ -4151,8 +4147,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, @@ -4166,6 +4162,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 @@ -4182,7 +4181,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))) { @@ -4192,7 +4195,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); @@ -4201,7 +4203,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); @@ -4245,7 +4246,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); @@ -4265,7 +4265,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); @@ -4276,18 +4275,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); @@ -4320,21 +4317,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); @@ -4342,7 +4324,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? */ @@ -4362,6 +4343,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); @@ -4387,11 +4369,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) { @@ -4505,7 +4533,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++) { @@ -6672,7 +6700,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) { /* @@ -6699,7 +6728,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 309cce10fe9b..690cdf3a3978 100644 --- a/sys/netinet/sctp_usrreq.c +++ b/sys/netinet/sctp_usrreq.c @@ -1416,7 +1416,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, @@ -1425,11 +1425,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) { @@ -7067,7 +7062,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); @@ -7075,11 +7070,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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202109111416.18BEGLqm071532>