Re: git: a5c2009dd8ab - main - sctp: improve handling of sctp inpcb flags
Date: Mon, 06 Jun 2022 14:59:32 UTC
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).
>
> 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.