From nobody Mon Jun 06 15:49:36 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 73D6C1BE704E; Mon, 6 Jun 2022 15:49:48 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from drew.franken.de (drew.ipv6.franken.de [IPv6:2001:638:a02:a001:20e:cff:fe4a:feaa]) (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 4LGybw1RS7z4Xmx; Mon, 6 Jun 2022 15:49:48 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from smtpclient.apple (unknown [IPv6:2a02:8109:1140:c3d:893e:8448:864e:a2e9]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTPSA id 5C7BA72276589; Mon, 6 Jun 2022 17:49:37 +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 \(3696.100.31\)) Subject: Re: git: a5c2009dd8ab - main - sctp: improve handling of sctp inpcb flags From: tuexen@freebsd.org In-Reply-To: Date: Mon, 6 Jun 2022 17:49:36 +0200 Cc: src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: <0345BCF3-F11B-4BB4-BA8E-72BFD0FE3370@freebsd.org> References: <202206040956.2549uqkY020631@gitrepo.freebsd.org> <690036B6-CD41-4D8B-8EAD-E5D9BAC4E1AA@freebsd.org> To: Mark Johnston X-Mailer: Apple Mail (2.3696.100.31) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, T_SCC_BODY_TEXT_LINE 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: 4LGybw1RS7z4Xmx 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 6. Jun 2022, at 16:59, Mark Johnston wrote: >=20 > On Sun, Jun 05, 2022 at 08:18:07PM +0200, tuexen@freebsd.org wrote: >>> On 5. Jun 2022, at 17:48, Mark Johnston wrote: >>>=20 >>> Hi Michael, >>>=20 >>> On Sat, Jun 04, 2022 at 09:56:52AM +0000, Michael Tuexen wrote: >>>> The branch main has been updated by tuexen: >>>>=20 >>>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3Da5c2009dd8ab562435fb7cc2ac092266= 8f9511a8 >>>>=20 >>>> commit a5c2009dd8ab562435fb7cc2ac0922668f9511a8 >>>> Author: Michael Tuexen >>>> AuthorDate: 2022-06-04 05:35:54 +0000 >>>> Commit: Michael Tuexen >>>> CommitDate: 2022-06-04 05:38:19 +0000 >>>>=20 >>>> sctp: improve handling of sctp inpcb flags >>>>=20 >>>> Use an atomic operation when the inp is not write locked. >>>>=20 >>>> 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(-) >>>>=20 >>>> [...] >>>> 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. >>>=20 >>> 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... >=20 > 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). OK. Will fix it.=20 >=20 >>=20 >> Best regards >> Michael >>>=20 >>>> + */ >>>> +void >>>> +sctp_pcb_add_flags(struct sctp_inpcb *inp, uint32_t flags) >>>> +{ >>>> + uint32_t old_flags, new_flags; >>>> + >>>> + do { >>>> + old_flags =3D inp->sctp_flags; >>>> + new_flags =3D old_flags | flags; >>>> + } while (atomic_cmpset_int(&inp->sctp_flags, old_flags, = new_flags) =3D=3D 0); >>>=20 >>> Is there anything preventing the compiler from transforming this to:=20= >>>=20 >>> do { >>> new_flags =3D inp->sctp_flags | flags; >>> old_flags =3D inp->sctp_flags; >>> } while (atomic_cmpset_int(&inp->sctp_flags, old_flags, = new_flags) =3D=3D 0); I don't know. I was assuming/hoping that the compiler does not transform = it, since it is not equivalent. >>>=20 >>> ? In this case the function would behave incorrectly, since = sctp_flags >>> could be modified by a different thread in between the two loads. >>>=20 >>> I believe it's necessary to write it like this: >>>=20 >>> do { >>> old_flags =3D atomic_load_32(&inp->sctp_flags); >>> new_flags =3D old_flags | flags; >>> } while (atomic_cmpset_int(&inp->sctp_flags, old_flags, = new_flags) =3D=3D 0); OK. Right now that function is not used in the code. So I need to figure = out how it is done on various platforms... >=20 > Actually, it looks like this loop could instead be a atomic_set_int() > call. Also not yet used... Thanks for the suggestions! Best regards Michael