Date: Mon, 6 Jun 2022 17:49:36 +0200 From: tuexen@freebsd.org To: Mark Johnston <markj@FreeBSD.org> Cc: src-committers <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: <0345BCF3-F11B-4BB4-BA8E-72BFD0FE3370@freebsd.org> In-Reply-To: <Yp4WVGg2FtzDnt3E@nuc> References: <202206040956.2549uqkY020631@gitrepo.freebsd.org> <YpzQXIHfDWI3vPWt@nuc> <690036B6-CD41-4D8B-8EAD-E5D9BAC4E1AA@freebsd.org> <Yp4WVGg2FtzDnt3E@nuc>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 6. Jun 2022, at 16:59, Mark Johnston <markj@FreeBSD.org> 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 <markj@FreeBSD.org> 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 <tuexen@FreeBSD.org> >>>> AuthorDate: 2022-06-04 05:35:54 +0000 >>>> Commit: Michael Tuexen <tuexen@FreeBSD.org> >>>> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0345BCF3-F11B-4BB4-BA8E-72BFD0FE3370>