From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: fstat socket buffer mutex
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Sat, 9 Nov 2024 18:46:48 +0300

> 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().

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