From nobody Sun Jun 05 15:48:44 2022 X-Original-To: dev-commits-src-main@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 565871BD437C; Sun, 5 Jun 2022 15:48:57 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qt1-x82e.google.com (mail-qt1-x82e.google.com [IPv6:2607:f8b0:4864:20::82e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4LGLdN0sYwz4g8Z; Sun, 5 Jun 2022 15:48:55 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qt1-x82e.google.com with SMTP id hh4so8937588qtb.10; Sun, 05 Jun 2022 08:48:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=n6dHgSEEZgYjDBgOiUhs4A9zMH752czi25ZF8m7igYs=; b=iOqYLA/Sv6eGQA6ksWKTu6BK1PALEDRXXQFfWhRGXg5+VOLvFGycba2vcbQBhZvzJe Cci0gJ6gmjY1SzcnlAzOjTu6b9qmSQ0xaRh02VkAITDEi1IGiSwep4FLvKYy3b0SXJF+ zKT3wODSOCIN/WJez8uXbHHLR4HhIRQjXGiLvAvZJVb8INDtMAgR7nIRDIip9hWexW0p 1eu8sDYukPCL9eQqlhtD5Sb7uZVX6n1DqEiiHAJPKMQNpis3jnNmOtod39vWWKJBAS1q H00WmXpsSyrniowB1ORGYubl96ZPn+EPXlMroNPDrtnoNmNK+KWitlfD57p99QZSUrH9 CqKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to; bh=n6dHgSEEZgYjDBgOiUhs4A9zMH752czi25ZF8m7igYs=; b=wgK9xYKoU8WRbUSsjPf2R1YVOT7mMATlJZg1jfjWpINEVmFe0mjuaf90URIu9dgWo8 Dv3BQokqhuPrAFx4fv9+SQeqXY/Tn1Zks2Iw7gCsg2wXkPz7Akw2mZZUWz9mb3qrfoUr H5DGWHezl+HdX+n7ikqO7vkH5Q1REezYCKAWZk8oXxVx58c+zzLhWqczn+sIM6W7QKKk FyBpbHq5mIy81KEokkGyYHht21hKHQ7kaN4hr9WFtF6F/hPYeMoId+uV4H84/AYr0dyo f+cLCCI+DYYcYdzZdQLmYV4brtdCxE2VE/oM+85eRBYwEBjomFmxlesR2yd9rJUnt/DR hGww== X-Gm-Message-State: AOAM532FB6qhT42f/Npe0M2GQAiDVog3V3Sw7ohZ3llk3RU2QGC4qo7F dXZhH4/Z98wNbHaPZDmifzGLmmkTYww= X-Google-Smtp-Source: ABdhPJz2nDrC05yvtJUEySC8Yf7nWST8/z5bBseQpOcMHIYUPoJuQP3CWv+UsU1fXN4mlEu0rWC+DA== X-Received: by 2002:ac8:7f41:0:b0:2fa:f70e:8d46 with SMTP id g1-20020ac87f41000000b002faf70e8d46mr15508637qtk.528.1654444128628; Sun, 05 Jun 2022 08:48:48 -0700 (PDT) Received: from nuc (198-84-189-58.cpe.teksavvy.com. [198.84.189.58]) by smtp.gmail.com with ESMTPSA id y19-20020a05620a44d300b006a6a4b43c01sm5301774qkp.38.2022.06.05.08.48.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 05 Jun 2022 08:48:46 -0700 (PDT) Date: Sun, 5 Jun 2022 11:48:44 -0400 From: Mark Johnston To: Michael Tuexen Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: a5c2009dd8ab - main - sctp: improve handling of sctp inpcb flags Message-ID: References: <202206040956.2549uqkY020631@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <202206040956.2549uqkY020631@gitrepo.freebsd.org> X-Rspamd-Queue-Id: 4LGLdN0sYwz4g8Z X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b="iOqYLA/S"; dmarc=none; spf=pass (mx1.freebsd.org: domain of markjdb@gmail.com designates 2607:f8b0:4864:20::82e as permitted sender) smtp.mailfrom=markjdb@gmail.com X-Spamd-Result: default: False [-0.32 / 15.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; NEURAL_HAM_MEDIUM(-0.42)[-0.422]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; MID_RHS_NOT_FQDN(0.50)[]; DMARC_NA(0.00)[freebsd.org]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; NEURAL_HAM_LONG(-1.00)[-1.000]; RCVD_COUNT_THREE(0.00)[3]; NEURAL_SPAM_SHORT(0.80)[0.797]; DKIM_TRACE(0.00)[gmail.com:+]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::82e:from]; MLMMJ_DEST(0.00)[dev-commits-src-all,dev-commits-src-main]; FORGED_SENDER(0.30)[markj@freebsd.org,markjdb@gmail.com]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[markj@freebsd.org,markjdb@gmail.com]; RCVD_TLS_ALL(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-ThisMailContainsUnwantedMimeParts: N Hi Michael, On Sat, Jun 04, 2022 at 09:56:52AM +0000, Michael Tuexen wrote: > The branch main has been updated by tuexen: > > URL: https://cgit.FreeBSD.org/src/commit/?id=a5c2009dd8ab562435fb7cc2ac0922668f9511a8 > > commit a5c2009dd8ab562435fb7cc2ac0922668f9511a8 > Author: Michael Tuexen > AuthorDate: 2022-06-04 05:35:54 +0000 > Commit: Michael Tuexen > CommitDate: 2022-06-04 05:38:19 +0000 > > sctp: improve handling of sctp inpcb flags > > Use an atomic operation when the inp is not write locked. > > Reported by: syzbot+bf27083e9a3f8fde8b4d@syzkaller.appspotmail.com > MFC after: 3 days > --- > sys/netinet/sctp_constants.h | 8 ++++---- > sys/netinet/sctp_input.c | 9 ++++----- > sys/netinet/sctp_pcb.c | 15 +++++++++++++++ > sys/netinet/sctp_pcb.h | 3 +++ > sys/netinet/sctputil.c | 2 +- > 5 files changed, 27 insertions(+), 10 deletions(-) > > diff --git a/sys/netinet/sctp_constants.h b/sys/netinet/sctp_constants.h > index 66f2cca5ab6d..3df6ad6db2aa 100644 > --- a/sys/netinet/sctp_constants.h > +++ b/sys/netinet/sctp_constants.h > @@ -968,7 +968,7 @@ __FBSDID("$FreeBSD$"); > #define sctp_sowwakeup(inp, so) \ > do { \ > if (inp->sctp_flags & SCTP_PCB_FLAGS_DONT_WAKE) { \ > - inp->sctp_flags |= SCTP_PCB_FLAGS_WAKEOUTPUT; \ > + sctp_pcb_add_flags(inp, SCTP_PCB_FLAGS_WAKEOUTPUT); \ > } else { \ > sowwakeup(so); \ > } \ > @@ -977,8 +977,8 @@ do { \ > #define sctp_sowwakeup_locked(inp, so) \ > do { \ > if (inp->sctp_flags & SCTP_PCB_FLAGS_DONT_WAKE) { \ > + sctp_pcb_add_flags(inp, SCTP_PCB_FLAGS_WAKEOUTPUT); \ > SOCKBUF_UNLOCK(&((so)->so_snd)); \ > - inp->sctp_flags |= SCTP_PCB_FLAGS_WAKEOUTPUT; \ > } else { \ > sowwakeup_locked(so); \ > } \ > @@ -987,7 +987,7 @@ do { \ > #define sctp_sorwakeup(inp, so) \ > do { \ > if (inp->sctp_flags & SCTP_PCB_FLAGS_DONT_WAKE) { \ > - inp->sctp_flags |= SCTP_PCB_FLAGS_WAKEINPUT; \ > + sctp_pcb_add_flags(inp, SCTP_PCB_FLAGS_WAKEINPUT); \ > } else { \ > sorwakeup(so); \ > } \ > @@ -996,7 +996,7 @@ do { \ > #define sctp_sorwakeup_locked(inp, so) \ > do { \ > if (inp->sctp_flags & SCTP_PCB_FLAGS_DONT_WAKE) { \ > - inp->sctp_flags |= SCTP_PCB_FLAGS_WAKEINPUT; \ > + sctp_pcb_add_flags(inp, SCTP_PCB_FLAGS_WAKEINPUT); \ > SOCKBUF_UNLOCK(&((so)->so_rcv)); \ > } else { \ > sorwakeup_locked(so); \ > diff --git a/sys/netinet/sctp_input.c b/sys/netinet/sctp_input.c > index ff16654968d5..46b818c9983e 100644 > --- a/sys/netinet/sctp_input.c > +++ b/sys/netinet/sctp_input.c > @@ -1491,8 +1491,7 @@ sctp_process_cookie_existing(struct mbuf *m, int iphlen, int offset, > * init/init-ack/cookie done before the > * init-ack came back.. > */ > - stcb->sctp_ep->sctp_flags |= > - SCTP_PCB_FLAGS_CONNECTED; > + sctp_pcb_add_flags(stcb->sctp_ep, SCTP_PCB_FLAGS_CONNECTED); > soisconnected(stcb->sctp_socket); > } > /* notify upper layer */ > @@ -1689,7 +1688,7 @@ sctp_process_cookie_existing(struct mbuf *m, int iphlen, int offset, > if (((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) || > (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL)) && > (!SCTP_IS_LISTENING(inp))) { > - stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED; > + sctp_pcb_add_flags(stcb->sctp_ep, SCTP_PCB_FLAGS_CONNECTED); > soisconnected(stcb->sctp_socket); > } > if (SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_ECHOED) > @@ -2182,7 +2181,7 @@ sctp_process_cookie_new(struct mbuf *m, int iphlen, int offset, > * > * XXXMJ unlocked This comment could be removed now. > */ > - stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED; > + sctp_pcb_add_flags(stcb->sctp_ep, SCTP_PCB_FLAGS_CONNECTED); > soisconnected(stcb->sctp_socket); > } else if ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) && > (SCTP_IS_LISTENING(inp))) { > @@ -2793,7 +2792,7 @@ sctp_handle_cookie_ack(struct sctp_cookie_ack_chunk *cp SCTP_UNUSED, > sctp_ulp_notify(SCTP_NOTIFY_ASSOC_UP, stcb, 0, NULL, SCTP_SO_NOT_LOCKED); > if ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) || > (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL)) { > - stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_CONNECTED; > + sctp_pcb_add_flags(stcb->sctp_ep, SCTP_PCB_FLAGS_CONNECTED); > if ((stcb->asoc.state & SCTP_STATE_CLOSED_SOCKET) == 0) { > soisconnected(stcb->sctp_socket); > } > diff --git a/sys/netinet/sctp_pcb.c b/sys/netinet/sctp_pcb.c > index 38c88d8ae8e4..bbbec5385c3c 100644 > --- a/sys/netinet/sctp_pcb.c > +++ b/sys/netinet/sctp_pcb.c > @@ -7067,3 +7067,18 @@ sctp_initiate_iterator(inp_func inpf, > /* sa_ignore MEMLEAK {memory is put on the tailq for the iterator} */ > return (0); > } > + > +/* > + * Atomically add flags to the sctp_flags of an inp. > + * To be used when the write lock of the inp is not held. This is only safe if there is some guarantee that a non-atomic update will never race with an atomic update. Right now, it looks like a non-atomic update can occur at the same time as an atomic update, and in that case it's possible that modifications to sctp_flags will be clobbered. > + */ > +void > +sctp_pcb_add_flags(struct sctp_inpcb *inp, uint32_t flags) > +{ > + uint32_t old_flags, new_flags; > + > + do { > + old_flags = inp->sctp_flags; > + new_flags = old_flags | flags; > + } while (atomic_cmpset_int(&inp->sctp_flags, old_flags, new_flags) == 0); Is there anything preventing the compiler from transforming this to: do { new_flags = inp->sctp_flags | flags; old_flags = inp->sctp_flags; } while (atomic_cmpset_int(&inp->sctp_flags, old_flags, new_flags) == 0); ? In this case the function would behave incorrectly, since sctp_flags could be modified by a different thread in between the two loads. I believe it's necessary to write it like this: do { old_flags = atomic_load_32(&inp->sctp_flags); new_flags = old_flags | flags; } while (atomic_cmpset_int(&inp->sctp_flags, old_flags, new_flags) == 0); > +} > diff --git a/sys/netinet/sctp_pcb.h b/sys/netinet/sctp_pcb.h > index 736b0f9d54e9..687ccf6a1c50 100644 > --- a/sys/netinet/sctp_pcb.h > +++ b/sys/netinet/sctp_pcb.h > @@ -619,6 +619,9 @@ int sctp_swap_inpcb_for_listen(struct sctp_inpcb *inp); > > void sctp_clean_up_stream(struct sctp_tcb *stcb, struct sctp_readhead *rh); > > +void > + sctp_pcb_add_flags(struct sctp_inpcb *, uint32_t); > + > /*- > * Null in last arg inpcb indicate run on ALL ep's. Specific inp in last arg > * indicates run on ONLY assoc's of the specified endpoint. > diff --git a/sys/netinet/sctputil.c b/sys/netinet/sctputil.c > index 23f95353323f..bdb35b988ae6 100644 > --- a/sys/netinet/sctputil.c > +++ b/sys/netinet/sctputil.c > @@ -4340,7 +4340,7 @@ sctp_abort_notification(struct sctp_tcb *stcb, bool from_peer, bool timeout, > if ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL) || > ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) && > (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_CONNECTED))) { > - stcb->sctp_ep->sctp_flags |= SCTP_PCB_FLAGS_WAS_ABORTED; > + sctp_pcb_add_flags(stcb->sctp_ep, SCTP_PCB_FLAGS_WAS_ABORTED); > } > if ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) || > (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_SOCKET_ALLGONE) || >