Date: Sat, 19 Aug 2023 15:04:55 +0200 From: tuexen@freebsd.org To: FreeBSD User <freebsd@walstatt-de.de> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 4f14d4b6b7f0 - main - sctp: cleanup handling of graceful shutdown of the peer Message-ID: <756F5A29-BA8B-450A-866A-081AFB5E3324@freebsd.org> In-Reply-To: <20230819141439.613ed198@thor.intern.walstatt.dynvpn.de> References: <202308191047.37JAlPN4069693@gitrepo.freebsd.org> <20230819141439.613ed198@thor.intern.walstatt.dynvpn.de>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 19. Aug 2023, at 14:14, FreeBSD User <freebsd@walstatt-de.de> = wrote: >=20 > Am Sat, 19 Aug 2023 10:47:25 GMT > Michael Tuexen <tuexen@FreeBSD.org> schrieb: >=20 >> The branch main has been updated by tuexen: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D4f14d4b6b7f0ca49b14379e48117121a= f3ed2669 >>=20 >> 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 >>=20 >> sctp: cleanup handling of graceful shutdown of the peer >>=20 >> 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. >>=20 >> 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(-) >>=20 >> 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; >>=20 >> - SCTPDBG(SCTP_DEBUG_INPUT2, >> - "sctp_handle_shutdown: handling SHUTDOWN\n"); >> + SCTPDBG(SCTP_DEBUG_INPUT2, "sctp_handle_shutdown: handling = SHUTDOWN\n"); >> if (stcb =3D=3D NULL) >> return; >> asoc =3D &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 =3D &asoc->strmin[asoc->control_pdapi->sinfo_stream]; >> - if (asoc->control_pdapi->on_strm_q =3D=3D SCTP_ON_UNORDERED) { >> - /* Unordered */ >> - TAILQ_REMOVE(&strm->uno_inqueue, asoc->control_pdapi, >> next_instrm); >> - asoc->control_pdapi->on_strm_q =3D 0; >> - } else if (asoc->control_pdapi->on_strm_q =3D=3D SCTP_ON_ORDERED) { >> - /* Ordered */ >> - TAILQ_REMOVE(&strm->inqueue, asoc->control_pdapi, >> next_instrm); >> - asoc->control_pdapi->on_strm_q =3D 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 =3D 1; >> - asoc->control_pdapi->pdapi_aborted =3D 1; >> - asoc->control_pdapi =3D 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) !=3D SCTP_STATE_SHUTDOWN_RECEIVED) && >> (SCTP_GET_STATE(stcb) !=3D SCTP_STATE_SHUTDOWN_ACK_SENT) && >> @@ -949,8 +920,9 @@ sctp_handle_shutdown_ack(struct = sctp_shutdown_ack_chunk *cp SCTP_UNUSED, >>=20 >> SCTPDBG(SCTP_DEBUG_INPUT2, >> "sctp_handle_shutdown_ack: handling SHUTDOWN ACK\n"); >> - if (stcb =3D=3D NULL) >> + if (stcb =3D=3D NULL) { >> return; >> + } >>=20 >> asoc =3D &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 =3D 1; >> - asoc->control_pdapi->pdapi_aborted =3D 1; >> - asoc->control_pdapi =3D 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 !=3D 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 =3D=3D 0) { >> - /* Held for PD-API clear that. */ >> + /* Held for PD-API, clear that. */ >> sq->pdapi_aborted =3D 1; >> sq->held_length =3D 0; >> if (sctp_stcb_is_feature_on(inp, stcb, >> SCTP_PCB_FLAGS_PDAPIEVNT) && (so !=3D 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; >>=20 >=20 > Something seems to go wrong after this commit: >=20 > (buildworld/-kernel with WITHOUT_CLEAN=3Dtrue): > [...] > =3D=3D=3D> 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=3D1095da75032b439d893c0947eda2f373= 8ecfe494 = <https://cgit.freebsd.org/src/commit/?id=3D1095da75032b439d893c0947eda2f37= 38ecfe494> Best regards Michael >=20 > Regards >=20 > oh >=20 >=20 > --=20 > O. Hartmann
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?756F5A29-BA8B-450A-866A-081AFB5E3324>