git: f9425b3a85e9 - main - sctp: cleanup locking for notifications
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 08 Sep 2023 14:23:20 UTC
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