From nobody Sat Aug 19 12:14:12 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 4RSd3H1fTtz4qdBv; Sat, 19 Aug 2023 12:14:51 +0000 (UTC) (envelope-from freebsd@walstatt-de.de) Received: from smtp6.goneo.de (smtp6.goneo.de [85.220.129.31]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4RSd3G6b10z3Ymq; Sat, 19 Aug 2023 12:14:50 +0000 (UTC) (envelope-from freebsd@walstatt-de.de) Authentication-Results: mx1.freebsd.org; none Received: from hub1.goneo.de (hub1.goneo.de [85.220.129.52]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by smtp6.goneo.de (Postfix) with ESMTPS id 1206510A3312; Sat, 19 Aug 2023 14:14:43 +0200 (CEST) Received: from hub1.goneo.de (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by hub1.goneo.de (Postfix) with ESMTPS id 55D81105C65A; Sat, 19 Aug 2023 14:14:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=walstatt-de.de; s=DKIM001; t=1692447280; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uUX0PRTLRrvawcFdumWny+HqHTToNmi5DandxkLu4ps=; b=hJblctCBWgOpHqiQJhcOzEpHSyVonDov9gUOWqn3iEQ6dWd+21OQhe1Hscqqnej0rZr6aV fO4BdEaGwcNQf9bOCvOtgbz5HwBcpHufTPiMVB4rPtuw2/dknygTJpVkDHbuMPOpUrECu/ YpV8rdJ03kcYNMJZNv1rx978Z9cxBAWFx2GVk6FdvhE3HvbAi0I2sV9HsySXlf+5LSY3gW hOblKdKD+w8rdPBwBGHqxLZsB5zgXJl+JuCLqXPWEks7tVAfFT9/8umXS1bbvhVw57bIoc wn9eIjfCyQYYJH47qTDRIWkAXeInBS0Uc+RgLOT7BvaFI1M+NU+Z7cJXra9bxw== Received: from thor.intern.walstatt.dynvpn.de (dynamic-089-012-150-235.89.12.pool.telefonica.de [89.12.150.235]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by hub1.goneo.de (Postfix) with ESMTPSA id 092E21060974; Sat, 19 Aug 2023 14:14:40 +0200 (CEST) Date: Sat, 19 Aug 2023 14:14:12 +0200 From: FreeBSD User To: Michael Tuexen 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> Organization: walstatt-de.de 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 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Rspamd-UID: 058395 X-Rspamd-UID: f676d7 X-Rspamd-Queue-Id: 4RSd3G6b10z3Ymq 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:25394, ipnet:85.220.128.0/17, country:DE] Am Sat, 19 Aug 2023 10:47:25 GMT Michael Tuexen schrieb: > The branch main has been updated by tuexen: > > URL: https://cgit.FreeBSD.org/src/commit/?id=4f14d4b6b7f0ca49b14379e48117121af3ed2669 > > commit 4f14d4b6b7f0ca49b14379e48117121af3ed2669 > Author: Michael Tuexen > AuthorDate: 2023-08-19 10:35:49 +0000 > Commit: Michael Tuexen > 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