Date: Sat, 19 Aug 2023 14:14:12 +0200 From: FreeBSD User <freebsd@walstatt-de.de> To: Michael Tuexen <tuexen@FreeBSD.org> Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 4f14d4b6b7f0 - main - sctp: cleanup handling of graceful shutdown of the peer Message-ID: <20230819141439.613ed198@thor.intern.walstatt.dynvpn.de> In-Reply-To: <202308191047.37JAlPN4069693@gitrepo.freebsd.org> References: <202308191047.37JAlPN4069693@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20230819141439.613ed198>