From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: Use WRITE_ONCE() to se set so_error in sosend()/soreceive()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: bluhm@openbsd.org, tech@openbsd.org
Date: Mon, 06 Jan 2025 17:45:49 +0100

> 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!


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