Date: Fri, 8 Sep 2023 14:23:20 GMT From: Michael Tuexen <tuexen@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: f9425b3a85e9 - main - sctp: cleanup locking for notifications Message-ID: <202309081423.388ENKbP056623@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by tuexen: URL: https://cgit.FreeBSD.org/src/commit/?id=f9425b3a85e9e211b61e11ce8115bf73674bdf49 commit f9425b3a85e9e211b61e11ce8115bf73674bdf49 Author: Michael Tuexen <tuexen@FreeBSD.org> AuthorDate: 2023-09-08 14:20:51 +0000 Commit: Michael Tuexen <tuexen@FreeBSD.org> CommitDate: 2023-09-08 14:20:51 +0000 sctp: cleanup locking for notifications All notifications are now queued via sctp_ulp_notify(). Do the locking of the inp read lock there and validate this in all functions being used. This is one step in avoiding race conditions when closing the read end of an SCTP socket. MFC after: 3 days --- sys/netinet/sctp_auth.c | 12 ++--- sys/netinet/sctputil.c | 132 ++++++++++++++++++++++++++++++------------------ 2 files changed, 87 insertions(+), 57 deletions(-) diff --git a/sys/netinet/sctp_auth.c b/sys/netinet/sctp_auth.c index 3c1962233347..8bcb6d5243db 100644 --- a/sys/netinet/sctp_auth.c +++ b/sys/netinet/sctp_auth.c @@ -1706,13 +1706,9 @@ sctp_notify_authentication(struct sctp_tcb *stcb, uint32_t indication, struct sctp_authkey_event *auth; struct sctp_queued_to_read *control; - if ((stcb == NULL) || - (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) || - (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) || - (stcb->asoc.state & SCTP_STATE_CLOSED_SOCKET)) { - /* If the socket is gone we are out of here */ - return; - } + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_AUTHEVNT)) /* event not enabled */ @@ -1757,7 +1753,7 @@ sctp_notify_authentication(struct sctp_tcb *stcb, uint32_t indication, control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } /*- diff --git a/sys/netinet/sctputil.c b/sys/netinet/sctputil.c index 17a5a098dd90..11469236014e 100644 --- a/sys/netinet/sctputil.c +++ b/sys/netinet/sctputil.c @@ -3149,10 +3149,11 @@ sctp_notify_assoc_change(uint16_t state, struct sctp_tcb *stcb, ("sctp_notify_assoc_change: ABORT chunk provided for local termination")); KASSERT(!from_peer || !timedout, ("sctp_notify_assoc_change: timeouts can only be local")); - if (stcb == NULL) { - return; - } + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); inp = stcb->sctp_ep; + SCTP_INP_READ_LOCK_ASSERT(inp); + if (sctp_stcb_is_feature_on(inp, stcb, SCTP_PCB_FLAGS_RECVASSOCEVNT)) { notif_len = (unsigned int)sizeof(struct sctp_assoc_change); if (abort != NULL) { @@ -3233,7 +3234,7 @@ sctp_notify_assoc_change(uint16_t state, struct sctp_tcb *stcb, control->tail_mbuf = m_notify; sctp_add_to_readq(inp, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } else { sctp_m_freem(m_notify); } @@ -3284,11 +3285,15 @@ sctp_notify_peer_addr_change(struct sctp_tcb *stcb, uint32_t state, struct sctp_paddr_change *spc; struct sctp_queued_to_read *control; - if ((stcb == NULL) || - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVPADDREVNT)) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVPADDREVNT)) { /* event not enabled */ return; } + m_notify = sctp_get_mbuf_for_msg(sizeof(struct sctp_paddr_change), 0, M_NOWAIT, 1, MT_DATA); if (m_notify == NULL) return; @@ -3359,7 +3364,7 @@ sctp_notify_peer_addr_change(struct sctp_tcb *stcb, uint32_t state, control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3373,9 +3378,12 @@ sctp_notify_send_failed(struct sctp_tcb *stcb, uint8_t sent, uint32_t error, struct sctp_chunkhdr *chkhdr; int notifhdr_len, chk_len, chkhdr_len, padding_len, payload_len; - if ((stcb == NULL) || - (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVSENDFAILEVNT) && - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVNSENDFAILEVNT))) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVSENDFAILEVNT) && + sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVNSENDFAILEVNT)) { /* event not enabled */ return; } @@ -3488,7 +3496,7 @@ sctp_notify_send_failed(struct sctp_tcb *stcb, uint8_t sent, uint32_t error, control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3501,12 +3509,16 @@ sctp_notify_send_failed2(struct sctp_tcb *stcb, uint32_t error, struct sctp_queued_to_read *control; int notifhdr_len; - if ((stcb == NULL) || - (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVSENDFAILEVNT) && - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVNSENDFAILEVNT))) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVSENDFAILEVNT) && + sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVNSENDFAILEVNT)) { /* event not enabled */ return; } + if (sctp_stcb_is_feature_on(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVNSENDFAILEVNT)) { notifhdr_len = sizeof(struct sctp_send_failed_event); } else { @@ -3584,7 +3596,7 @@ sctp_notify_send_failed2(struct sctp_tcb *stcb, uint32_t error, control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3594,8 +3606,11 @@ sctp_notify_adaptation_layer(struct sctp_tcb *stcb, int so_locked) struct sctp_adaptation_event *sai; struct sctp_queued_to_read *control; - if ((stcb == NULL) || - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_ADAPTATIONEVNT)) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_ADAPTATIONEVNT)) { /* event not enabled */ return; } @@ -3631,7 +3646,7 @@ sctp_notify_adaptation_layer(struct sctp_tcb *stcb, int so_locked) control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3644,15 +3659,16 @@ sctp_notify_partial_delivery_indication(struct sctp_tcb *stcb, uint32_t error, struct sctp_queued_to_read *control; struct sockbuf *sb; - if ((stcb == NULL) || - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_PDAPIEVNT)) { + KASSERT(aborted_control != NULL, ("aborted_control is NULL")); + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_PDAPIEVNT)) { /* event not enabled */ return; } - KASSERT(aborted_control != NULL, ("aborted_control is NULL")); - SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); - m_notify = sctp_get_mbuf_for_msg(sizeof(struct sctp_pdapi_event), 0, M_NOWAIT, 1, MT_DATA); if (m_notify == NULL) /* no space left */ @@ -3705,6 +3721,10 @@ sctp_notify_shutdown_event(struct sctp_tcb *stcb, int so_locked) struct sctp_shutdown_event *sse; struct sctp_queued_to_read *control; + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + /* * For TCP model AND UDP connected sockets we will send an error up * when an SHUTDOWN completes @@ -3714,6 +3734,7 @@ sctp_notify_shutdown_event(struct sctp_tcb *stcb, int so_locked) /* mark socket closed for read/write and wakeup! */ socantsendmore(stcb->sctp_socket); } + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVSHUTDOWNEVNT)) { /* event not enabled */ return; @@ -3748,7 +3769,7 @@ sctp_notify_shutdown_event(struct sctp_tcb *stcb, int so_locked) control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3758,8 +3779,11 @@ sctp_notify_sender_dry_event(struct sctp_tcb *stcb, int so_locked) struct sctp_sender_dry_event *event; struct sctp_queued_to_read *control; - if ((stcb == NULL) || - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_DRYEVNT)) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_DRYEVNT)) { /* event not enabled */ return; } @@ -3795,7 +3819,7 @@ sctp_notify_sender_dry_event(struct sctp_tcb *stcb, int so_locked) control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3805,13 +3829,10 @@ sctp_notify_stream_reset_add(struct sctp_tcb *stcb, int flag, int so_locked) struct sctp_queued_to_read *control; struct sctp_stream_change_event *stradd; - if ((stcb == NULL) || - (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) || - (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) || - (stcb->asoc.state & SCTP_STATE_CLOSED_SOCKET)) { - /* If the socket is gone we are out of here. */ - return; - } + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_STREAM_CHANGEEVNT)) { /* event not enabled */ return; @@ -3858,7 +3879,7 @@ sctp_notify_stream_reset_add(struct sctp_tcb *stcb, int flag, int so_locked) control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3868,13 +3889,10 @@ sctp_notify_stream_reset_tsn(struct sctp_tcb *stcb, int flag, int so_locked) struct sctp_queued_to_read *control; struct sctp_assoc_reset_event *strasoc; - if ((stcb == NULL) || - (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) || - (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) || - (stcb->asoc.state & SCTP_STATE_CLOSED_SOCKET)) { - /* If the socket is gone we are out of here. */ - return; - } + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_ASSOC_RESETEVNT)) { /* event not enabled */ return; @@ -3915,7 +3933,7 @@ sctp_notify_stream_reset_tsn(struct sctp_tcb *stcb, int flag, int so_locked) control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3927,8 +3945,11 @@ sctp_notify_stream_reset(struct sctp_tcb *stcb, struct sctp_stream_reset_event *strreset; int len; - if ((stcb == NULL) || - (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_STREAM_RESETEVNT))) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_STREAM_RESETEVNT)) { /* event not enabled */ return; } @@ -3979,7 +4000,7 @@ sctp_notify_stream_reset(struct sctp_tcb *stcb, control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, SCTP_SO_NOT_LOCKED); + SCTP_READ_LOCK_HELD, so_locked); } static void @@ -3992,10 +4013,14 @@ sctp_notify_remote_error(struct sctp_tcb *stcb, uint16_t error, unsigned int notif_len; uint16_t chunk_len; - if ((stcb == NULL) || - sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVPEERERR)) { + KASSERT(stcb != NULL, ("stcb == NULL")); + SCTP_TCB_LOCK_ASSERT(stcb); + SCTP_INP_READ_LOCK_ASSERT(stcb->sctp_ep); + + if (sctp_stcb_is_feature_off(stcb->sctp_ep, stcb, SCTP_PCB_FLAGS_RECVPEERERR)) { return; } + if (chunk != NULL) { chunk_len = ntohs(chunk->ch.chunk_length); /* @@ -4041,7 +4066,7 @@ sctp_notify_remote_error(struct sctp_tcb *stcb, uint16_t error, control->tail_mbuf = m_notify; sctp_add_to_readq(stcb->sctp_ep, stcb, control, &stcb->sctp_socket->so_rcv, 1, - SCTP_READ_LOCK_NOT_HELD, so_locked); + SCTP_READ_LOCK_HELD, so_locked); } else { sctp_m_freem(m_notify); } @@ -4070,9 +4095,15 @@ sctp_ulp_notify(uint32_t notification, struct sctp_tcb *stcb, return; } } + if (notification != SCTP_NOTIFY_PARTIAL_DELVIERY_INDICATION) { + SCTP_INP_READ_LOCK(inp); + } + SCTP_INP_READ_LOCK_ASSERT(inp); + if ((inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) || (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) || (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_CANT_READ)) { + SCTP_INP_READ_UNLOCK(inp); return; } @@ -4220,6 +4251,9 @@ sctp_ulp_notify(uint32_t notification, struct sctp_tcb *stcb, __func__, notification, notification); break; } + if (notification != SCTP_NOTIFY_PARTIAL_DELVIERY_INDICATION) { + SCTP_INP_READ_UNLOCK(inp); + } } void
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202309081423.388ENKbP056623>