Re: git: 4f14d4b6b7f0 - main - sctp: cleanup handling of graceful shutdown of the peer
Date: Sat, 19 Aug 2023 12:14:12 UTC
Am Sat, 19 Aug 2023 10:47:25 GMT
Michael Tuexen <tuexen@FreeBSD.org> schrieb:
> The branch main has been updated by tuexen:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=4f14d4b6b7f0ca49b14379e48117121af3ed2669
>
> commit 4f14d4b6b7f0ca49b14379e48117121af3ed2669
> Author: Michael Tuexen <tuexen@FreeBSD.org>
> AuthorDate: 2023-08-19 10:35:49 +0000
> Commit: Michael Tuexen <tuexen@FreeBSD.org>
> CommitDate: 2023-08-19 10:35:49 +0000
>
> sctp: cleanup handling of graceful shutdown of the peer
>
> Don't handle a graceful shutdown of the peer as an implicit signal
> that all partial messages are complete. First, this is not implemented
> correctly and second this should not be done by the peer. It is more
> appropriate to handle this as a protocol violation.
> Remove the incorrect code and leave detecting the protocol violation
> and its handling in a followup commit.
>
> MFC after: 1 week
> ---
> sys/netinet/sctp_input.c | 63 +++++++++++-----------------------------------
> sys/netinet/sctp_pcb.c | 11 +-------
> sys/netinet/sctp_structs.h | 9 -------
> 3 files changed, 16 insertions(+), 67 deletions(-)
>
> diff --git a/sys/netinet/sctp_input.c b/sys/netinet/sctp_input.c
> index 81b011b7e78a..059c6ded2e5e 100644
> --- a/sys/netinet/sctp_input.c
> +++ b/sys/netinet/sctp_input.c
> @@ -837,8 +837,7 @@ sctp_handle_shutdown(struct sctp_shutdown_chunk *cp,
> int some_on_streamwheel;
> int old_state;
>
> - SCTPDBG(SCTP_DEBUG_INPUT2,
> - "sctp_handle_shutdown: handling SHUTDOWN\n");
> + SCTPDBG(SCTP_DEBUG_INPUT2, "sctp_handle_shutdown: handling SHUTDOWN\n");
> if (stcb == NULL)
> return;
> asoc = &stcb->asoc;
> @@ -855,40 +854,12 @@ sctp_handle_shutdown(struct sctp_shutdown_chunk *cp,
> if (*abort_flag) {
> return;
> }
> - if (asoc->control_pdapi) {
> - /*
> - * With a normal shutdown we assume the end of last record.
> - */
> - SCTP_INP_READ_LOCK(stcb->sctp_ep);
> - if (asoc->control_pdapi->on_strm_q) {
> - struct sctp_stream_in *strm;
> -
> - strm = &asoc->strmin[asoc->control_pdapi->sinfo_stream];
> - if (asoc->control_pdapi->on_strm_q == SCTP_ON_UNORDERED) {
> - /* Unordered */
> - TAILQ_REMOVE(&strm->uno_inqueue, asoc->control_pdapi,
> next_instrm);
> - asoc->control_pdapi->on_strm_q = 0;
> - } else if (asoc->control_pdapi->on_strm_q == SCTP_ON_ORDERED) {
> - /* Ordered */
> - TAILQ_REMOVE(&strm->inqueue, asoc->control_pdapi,
> next_instrm);
> - asoc->control_pdapi->on_strm_q = 0;
> -#ifdef INVARIANTS
> - } else {
> - panic("Unknown state on ctrl:%p on_strm_q:%d",
> - asoc->control_pdapi,
> - asoc->control_pdapi->on_strm_q);
> -#endif
> - }
> - }
> - asoc->control_pdapi->end_added = 1;
> - asoc->control_pdapi->pdapi_aborted = 1;
> - asoc->control_pdapi = NULL;
> - SCTP_INP_READ_UNLOCK(stcb->sctp_ep);
> - if (stcb->sctp_socket) {
> - sctp_sorwakeup(stcb->sctp_ep, stcb->sctp_socket);
> - }
> - }
> - /* goto SHUTDOWN_RECEIVED state to block new requests */
> + /*
> + * FIXME MT: Handle the case where there are still incomplete
> + * received user messages or known missing user messages from the
> + * peer. One way to handle this is to abort the associations in this
> + * case.
> + */
> if (stcb->sctp_socket) {
> if ((SCTP_GET_STATE(stcb) != SCTP_STATE_SHUTDOWN_RECEIVED) &&
> (SCTP_GET_STATE(stcb) != SCTP_STATE_SHUTDOWN_ACK_SENT) &&
> @@ -949,8 +920,9 @@ sctp_handle_shutdown_ack(struct sctp_shutdown_ack_chunk *cp SCTP_UNUSED,
>
> SCTPDBG(SCTP_DEBUG_INPUT2,
> "sctp_handle_shutdown_ack: handling SHUTDOWN ACK\n");
> - if (stcb == NULL)
> + if (stcb == NULL) {
> return;
> + }
>
> asoc = &stcb->asoc;
> /* process according to association state */
> @@ -967,17 +939,12 @@ sctp_handle_shutdown_ack(struct sctp_shutdown_ack_chunk *cp
> SCTP_UNUSED, SCTP_TCB_UNLOCK(stcb);
> return;
> }
> - if (asoc->control_pdapi) {
> - /*
> - * With a normal shutdown we assume the end of last record.
> - */
> - SCTP_INP_READ_LOCK(stcb->sctp_ep);
> - asoc->control_pdapi->end_added = 1;
> - asoc->control_pdapi->pdapi_aborted = 1;
> - asoc->control_pdapi = NULL;
> - SCTP_INP_READ_UNLOCK(stcb->sctp_ep);
> - sctp_sorwakeup(stcb->sctp_ep, stcb->sctp_socket);
> - }
> + /*
> + * FIXME MT: Handle the case where there are still incomplete
> + * received user messages or known missing user messages from the
> + * peer. One way to handle this is to abort the associations in this
> + * case.
> + */
> #ifdef INVARIANTS
> if (!TAILQ_EMPTY(&asoc->send_queue) ||
> !TAILQ_EMPTY(&asoc->sent_queue) ||
> diff --git a/sys/netinet/sctp_pcb.c b/sys/netinet/sctp_pcb.c
> index ac47b6aa1bfc..16cde18c4c1d 100644
> --- a/sys/netinet/sctp_pcb.c
> +++ b/sys/netinet/sctp_pcb.c
> @@ -3405,7 +3405,6 @@ sctp_inpcb_free(struct sctp_inpcb *inp, int immediate, int from)
> continue;
> }
> if ((stcb->asoc.size_on_reasm_queue > 0) ||
> - (stcb->asoc.control_pdapi) ||
> (stcb->asoc.size_on_all_streams > 0) ||
> ((so != NULL) && (SCTP_SBAVAIL(&so->so_rcv) > 0))) {
> /* Left with Data unread */
> @@ -4762,18 +4761,10 @@ sctp_free_assoc(struct sctp_inpcb *inp, struct sctp_tcb *stcb, int
> from_inpcbfre
> * now.
> */
> if (sq->end_added == 0) {
> - /* Held for PD-API clear that. */
> + /* Held for PD-API, clear that. */
> sq->pdapi_aborted = 1;
> sq->held_length = 0;
> if (sctp_stcb_is_feature_on(inp, stcb,
> SCTP_PCB_FLAGS_PDAPIEVNT) && (so != NULL)) {
> - /*
> - * Need to add a PD-API
> - * aborted indication.
> - * Setting the control_pdapi
> - * assures that it will be
> - * added right after this
> - * msg.
> - */
> sctp_ulp_notify(SCTP_NOTIFY_PARTIAL_DELVIERY_INDICATION,
> stcb,
> SCTP_PARTIAL_DELIVERY_ABORTED,
> diff --git a/sys/netinet/sctp_structs.h b/sys/netinet/sctp_structs.h
> index 278afb2cc554..53d9bfd4f445 100644
> --- a/sys/netinet/sctp_structs.h
> +++ b/sys/netinet/sctp_structs.h
> @@ -956,15 +956,6 @@ struct sctp_association {
> uint32_t fast_recovery_tsn;
> uint32_t sat_t3_recovery_tsn;
> uint32_t tsn_last_delivered;
> - /*
> - * For the pd-api we should re-write this a bit more efficient. We
> - * could have multiple sctp_queued_to_read's that we are building at
> - * once. Now we only do this when we get ready to deliver to the
> - * socket buffer. Note that we depend on the fact that the struct is
> - * "stuck" on the read queue until we finish all the pd-api.
> - */
> - struct sctp_queued_to_read *control_pdapi;
> -
> uint32_t tsn_of_pdapi_last_delivered;
> uint32_t pdapi_ppid;
> uint32_t context;
>
Something seems to go wrong after this commit:
(buildworld/-kernel with WITHOUT_CLEAN=true):
[...]
===> accf_dns (all)
--- sctp_input.o ---
/usr/src/sys/netinet/sctp_input.c:919:27: error: variable 'asoc' set but not used
[-Werror,-Wunused-but-set-variable] struct sctp_association *asoc;
^
--- modules-all ---
[...]
Regards
oh
--
O. Hartmann