From nobody Sat Aug 19 13:04:55 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4RSf9868yWz4qhVY; Sat, 19 Aug 2023 13:05:00 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from drew.franken.de (mail-n.franken.de [193.175.24.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.franken.de", Issuer "Sectigo RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RSf982tyYz3d6j; Sat, 19 Aug 2023 13:05:00 +0000 (UTC) (envelope-from tuexen@freebsd.org) Authentication-Results: mx1.freebsd.org; none Received: from smtpclient.apple (unknown [IPv6:2a02:8109:1140:c3d:70f9:430b:3c08:cd93]) (Authenticated sender: micmac) by mail-n.franken.de (Postfix) with ESMTPSA id C8092772CC29F; Sat, 19 Aug 2023 15:04:55 +0200 (CEST) Content-Type: text/plain; charset=us-ascii List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6\)) Subject: Re: git: 4f14d4b6b7f0 - main - sctp: cleanup handling of graceful shutdown of the peer From: tuexen@freebsd.org In-Reply-To: <20230819141439.613ed198@thor.intern.walstatt.dynvpn.de> Date: Sat, 19 Aug 2023 15:04:55 +0200 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <756F5A29-BA8B-450A-866A-081AFB5E3324@freebsd.org> References: <202308191047.37JAlPN4069693@gitrepo.freebsd.org> <20230819141439.613ed198@thor.intern.walstatt.dynvpn.de> To: FreeBSD User X-Mailer: Apple Mail (2.3731.700.6) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=disabled version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mail-n.franken.de X-Rspamd-Queue-Id: 4RSf982tyYz3d6j X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:680, ipnet:193.174.0.0/15, country:DE] > On 19. Aug 2023, at 14:14, FreeBSD User = wrote: >=20 > Am Sat, 19 Aug 2023 10:47:25 GMT > Michael Tuexen 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 >> AuthorDate: 2023-08-19 10:35:49 +0000 >> Commit: Michael Tuexen >> 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 = Best regards Michael >=20 > Regards >=20 > oh >=20 >=20 > --=20 > O. Hartmann