Re: git: a5c2009dd8ab - main - sctp: improve handling of sctp inpcb flags
- In reply to: Mark Johnston : "Re: git: a5c2009dd8ab - main - sctp: improve handling of sctp inpcb flags"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 06 Jun 2022 15:49:36 UTC
> On 6. Jun 2022, at 16:59, Mark Johnston <markj@FreeBSD.org> wrote: > > 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: >>> >>> 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 <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 >>>> >>>> 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). OK. Will fix it. > >> >> 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); I don't know. I was assuming/hoping that the compiler does not transform it, since it is not equivalent. >>> >>> ? 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); OK. Right now that function is not used in the code. So I need to figure out how it is done on various platforms... > > 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