Re: git: a5c2009dd8ab - main - sctp: improve handling of sctp inpcb flags

> 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