From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: fstat socket buffer mutex
To: Vitaliy Makkoveev <otto@bsdbox.dev>
Cc: tech@openbsd.org
Date: Sun, 10 Nov 2024 11:08:53 +0100

On Sat, Nov 09, 2024 at 06:46:48PM +0300, Vitaliy Makkoveev wrote:
> > On 9 Nov 2024, at 18:06, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> > 
> > On Sat, Nov 09, 2024 at 05:54:06PM +0300, Vitaliy Makkoveev wrote:
> >>> On 9 Nov 2024, at 17:50, Vitaliy Makkoveev <otto@bsdbox.dev> wrote:
> >>> 
> >>>> On 9 Nov 2024, at 17:38, Alexander Bluhm <bluhm@openbsd.org> wrote:
> >>>> 
> >>>> Hi,
> >>>> 
> >>>> I wonder if we should also lock so_snd in soo_stat().  Take so_rcv
> >>>> and so_snd mutex together to provide consistent view to userland.
> >>>> 
> >>>> ok?
> >>>> 
> >>> 
> >>> We set SS_CANTRCVMORE and SS_CANTSENDMORE independently from each
> >>> other, no consistency between them.
> >>> 
> >>> I locked so_rcv to keep consistency between sb_cc and sb_state,
> >>> but no locks required for so_snd because we only check sb_state.
> > 
> > I am not a fan of unlocked read access.  As this is not performance
> > critical, can we just put a mutex here?  This is easier than adding
> > READ_ONLY() or something like that.  And it is consistent to so_rcv.
> > 
> > ok?
> > 
> 
> No problem. But personally me, I don't see the reason to do
> something extra around reading in the cases like this or like
> sogetopt().

If you prefer, we can also use READ_ONCE() instead of mutex.
But we should mark somehow that here is an unlocked access.

ok?

bluhm

Index: kern/sys_socket.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/sys_socket.c,v
diff -u -p -r1.65 sys_socket.c
--- kern/sys_socket.c	30 Apr 2024 17:59:15 -0000	1.65
+++ kern/sys_socket.c	10 Nov 2024 10:05:55 -0000
@@ -153,7 +153,7 @@ soo_stat(struct file *fp, struct stat *u
 	    so->so_rcv.sb_cc != 0)
 		ub->st_mode |= S_IRUSR | S_IRGRP | S_IROTH;
 	mtx_leave(&so->so_rcv.sb_mtx);
-	if ((so->so_snd.sb_state & SS_CANTSENDMORE) == 0)
+	if ((READ_ONCE(so->so_snd.sb_state) & SS_CANTSENDMORE) == 0)
 		ub->st_mode |= S_IWUSR | S_IWGRP | S_IWOTH;
 	ub->st_uid = so->so_euid;
 	ub->st_gid = so->so_egid;