Re: git: 4f14d4b6b7f0 - main - sctp: cleanup handling of graceful shutdown of the peer

In reply to: FreeBSD User : "Re: git: 4f14d4b6b7f0 - main - sctp: cleanup handling of graceful shutdown of the peer"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: <tuexen_at_freebsd.org>
Date: Sat, 19 Aug 2023 13:04:55 UTC
> On 19. Aug 2023, at 14:14, FreeBSD User <freebsd@walstatt-de.de> wrote:
> 
> 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 ---
> [...]
Thanks for the report. The problem is fixed in:
https://cgit.FreeBSD.org/src/commit/?id=1095da75032b439d893c0947eda2f3738ecfe494 <https://cgit.freebsd.org/src/commit/?id=1095da75032b439d893c0947eda2f3738ecfe494>

Best regards
Michael
> 
> Regards
> 
> oh
> 
> 
> -- 
> O. Hartmann