From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: Use WRITE_ONCE() to se set so_error in sosend()/soreceive()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, Alexander Bluhm <bluhm@openbsd.org>, OpenBSD tech <tech@openbsd.org>
Date: Tue, 7 Jan 2025 16:00:31 +0100

On 07/01/25(Tue) 13:57, Vitaliy Makkoveev wrote:
> 
> 
> > 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.

I don't understand why do we need READ_ONCE() to ensure the value is
loaded for every iteration of the loop.  Isn't already the case?  If
not do the other fields `sb_state', `so_state', etc also need it?

> 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");
>