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?

> solock_shared() was kept because tcp(4) sockets rely on it and I
> see no reason to introduce if-tcp-solock branching here. In the
> future I plan to push solock() down to pru_sence().

Currently I am going through TCP output path and check where
solock_shared() is sufficient.  Then we can set PR_MPSOCKET for
TCP.

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	9 Nov 2024 15:00:38 -0000
@@ -153,8 +153,10 @@ 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);
+	mtx_enter(&so->so_snd.sb_mtx);
 	if ((so->so_snd.sb_state & SS_CANTSENDMORE) == 0)
 		ub->st_mode |= S_IWUSR | S_IWGRP | S_IWOTH;
+	mtx_leave(&so->so_snd.sb_mtx);
 	ub->st_uid = so->so_euid;
 	ub->st_gid = so->so_egid;
 	(void)pru_sense(so, ub);