From nobody Mon Jun 06 14:59:32 2022 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 B458F1BDD25A; Mon, 6 Jun 2022 14:59:37 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com [IPv6:2607:f8b0:4864:20::72f]) (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 4LGxV12Ybgz3thH; Mon, 6 Jun 2022 14:59:37 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qk1-x72f.google.com with SMTP id d128so3188707qkg.8; Mon, 06 Jun 2022 07:59:37 -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=WTyU9/dGLvKSehR3ySTlgR9LsjWYHslkqmpY1ZJGv98=; b=j83iqezse6wI7odyA3SXdFy+g6SSz7+EkZ/Rp2P1bgCvmKYFKrzwOPGVxI9r8iPpJT dnfGnRIOwW0GlkID6BREiZqoLkIKCNmhvAesBN2Xbn5rRMZMxDd0HugiY7mezRsYdFaF jqyDcHvrSaC/KYKDy6BdmGnyW0OKOYoeqpmxWT8Y6I5GIb+sdZ03yFgRYjmo+GxcS1R8 TjuEuRmE+8U4AwUWxXJa5SzX4L6i8Bc0usZ8oGoKJER84rBKOkNn2U/HKx7WEWFXMHZo 5/+EnxM9+rHfAkoZD05/iaSAKix5ZbvpBAdE7fgr9Pb/GVbU1AypqQmlFJ0acssIYfyM 2iWg== 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=WTyU9/dGLvKSehR3ySTlgR9LsjWYHslkqmpY1ZJGv98=; b=m5qrUfJVZSJxx+w6S7A1JyMFOPN1oeqz+TTCpd3T9zlnljAeqGcwj8O00MHfgcx4Xx QPI6x5yw4ezZ/waMJ62KxAV8bYryoBaimwjv24VhxbId5cbzMSHgk++gR/3Xm03F2Q1f Geq/+PpEbpqwqHmBc+tkgpB2mKOtR0RUZB+5eahm58JoGulls4WSG5is9A5vMnHVJXgE PIDDhuMYsQF8VqeWnGVjsj+KRR42tdzgtCCXJry62K4lDk40mVJ5s4j+pnPSvsN5oGdo InR+b1vWt0DSJQwAM6H1+GfrMWbNR0kT9E7q+XrK63tGYkhcPGzDsRyr2lNNLA66xAp4 l8rA== X-Gm-Message-State: AOAM533ek2uuivBSkE1B7fM0TBzAHOinRuAnf1h7bt7NgDprls8MtaMr lHWZr+3iQAZbtR9XkNrrBBFIQ/0x2bU= X-Google-Smtp-Source: ABdhPJyIgGWetKADcMUVIcAuvgCtr4/kZYM6atHY0ZpW0aW7pIhm9ksCnZvnh2NZ2lt+/OYNJqk0fg== X-Received: by 2002:a05:620a:2482:b0:6a5:c051:398 with SMTP id i2-20020a05620a248200b006a5c0510398mr15646441qkn.429.1654527576402; Mon, 06 Jun 2022 07:59:36 -0700 (PDT) Received: from nuc (198-84-189-58.cpe.teksavvy.com. [198.84.189.58]) by smtp.gmail.com with ESMTPSA id 14-20020a37030e000000b006a6b498e23esm4081798qkd.81.2022.06.06.07.59.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jun 2022 07:59:35 -0700 (PDT) Date: Mon, 6 Jun 2022 10:59:32 -0400 From: Mark Johnston To: tuexen@freebsd.org 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> <690036B6-CD41-4D8B-8EAD-E5D9BAC4E1AA@freebsd.org> 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-Disposition: inline In-Reply-To: <690036B6-CD41-4D8B-8EAD-E5D9BAC4E1AA@freebsd.org> X-Rspamd-Queue-Id: 4LGxV12Ybgz3thH X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On Sun, Jun 05, 2022 at 08:18:07PM +0200, tuexen@freebsd.org wrote: > > On 5. Jun 2022, at 17:48, Mark Johnston wrote: > > > > 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_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. > In most of the cases the inp write lock is held when changing the flags. > The places I changed, added flag, but did not hold the write lock. > Are you suggesting that all places should hold the inp write lock or > do the setting atomically? In some places it might he hard to get > the inp lock due to lock order constraints... Right. If some of the updates are non-atomic (i.e., protected only by the inp write lock), then it's still possible for an atomic update to clobber the non-atomic update. Either all updates must be protected by the inp write lock, or all updates must be atomic (including those already protected by the write lock). > > Best regards > Michael > > > >> + */ > >> +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); Actually, it looks like this loop could instead be a atomic_set_int() call.