From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Use WRITE_ONCE() to se set so_error in sosend()/soreceive()
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: Alexander Bluhm <bluhm@openbsd.org>, OpenBSD tech <tech@openbsd.org>
Date: Tue, 7 Jan 2025 13:57:28 +0300



> On 7 Jan 2025, at 13:45, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
>> From: Vitaliy Makkoveev <mvs@openbsd.org>
>> Date: Mon, 6 Jan 2025 20:07:19 +0300
>> 
>>> On 6 Jan 2025, at 19:45, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>> 
>>>> Date: Mon, 6 Jan 2025 18:58:16 +0300
>>>> From: Vitaliy Makkoveev <mvs@openbsd.org>
>>>> 
>>>> This is the lockless access, so do it.
>>> 
>>> Sorry, but once again, I don't think this makes sense.  Atomic access
>>> doesn't mean blindly replacing assignments with
>>> READ_ONCE()/WRITE_ONCE() or atomic_load_int()/atomic_store_int().  The
>>> diff below makes me strongly suspect accessing so_error without
>>> holding some sort of lock isn't actually safe.  At the very minimum
>>> you're missing some memory barriers here!
>>> 
>>> 
>> 
>> This is not the blindly replacing, we need WRITE_ONCE()/READ_ONCE()
>> here to prevent reordering and be sure, so_error value is not cached.
> 
> But using READ_ONCE()/WRITE_ONCE() isn't enough to guarantee ordering
> between threads.  You need proper memory barriers for that.
> 

I’m not interesting in order between threads here, so_error value
could be lost even in the good old kernel locked or uniprocessor
times. I only want it to be loaded or writed each do {} while()
iteration, that’s the reason of *_ONCE() to exist.

The synchronisation in this place made by dedicated locks.

> Now the question is what are you ordering against?  That isn't
> immediately obvious to me; marking a variable or struct member as
> "atomic" in the header file doesn't really help here...
> 
>>>> Index: sys/kern/uipc_socket.c
>>>> ===================================================================
>>>> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
>>>> diff -u -p -r1.356 uipc_socket.c
>>>> --- sys/kern/uipc_socket.c	4 Jan 2025 15:57:02 -0000	1.356
>>>> +++ sys/kern/uipc_socket.c	6 Jan 2025 15:53:54 -0000
>>>> @@ -640,7 +640,7 @@ restart:
>>>> 		if (so->so_snd.sb_state & SS_CANTSENDMORE)
>>>> 			snderr(EPIPE);
>>>> 		if ((error = READ_ONCE(so->so_error))) {
>>>> -			so->so_error = 0;
>>>> +			WRITE_ONCE(so->so_error, 0);
>>>> 			snderr(error);
>>>> 		}
>>>> 		if ((so->so_state & SS_ISCONNECTED) == 0) {
>>>> @@ -934,7 +934,7 @@ restart:
>>>> 				goto dontblock;
>>>> 			error = error2;
>>>> 			if ((flags & MSG_PEEK) == 0)
>>>> -				so->so_error = 0;
>>>> +				WRITE_ONCE(so->so_error, 0);
>>>> 			goto release;
>>>> 		}
>>>> 		if (so->so_rcv.sb_state & SS_CANTRCVMORE) {
>>>> @@ -1204,7 +1204,7 @@ dontblock:
>>>> 		while (flags & MSG_WAITALL && m == NULL && uio->uio_resid > 0 &&
>>>> 		    !sosendallatonce(so) && !nextrecord) {
>>>> 			if (so->so_rcv.sb_state & SS_CANTRCVMORE ||
>>>> -			    so->so_error)
>>>> +			    READ_ONCE(so->so_error))
>>>> 				break;
>>>> 			SBLASTRECORDCHK(&so->so_rcv, "soreceive sbwait 2");
>>>> 			SBLASTMBUFCHK(&so->so_rcv, "soreceive sbwait 2");