From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: tcp(4): use per-sockbuf mutex to protect `so_snd' socket buffer
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: OpenBSD tech <tech@openbsd.org>
Date: Thu, 26 Dec 2024 03:04:41 +0300

> On 26 Dec 2024, at 02:48, Vitaliy Makkoveev <mvs@openbsd.org> wrote:
> 
>> On 25 Dec 2024, at 14:43, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
>> 
>> On Tue, Dec 24, 2024 at 10:21:55PM +0300, Vitaliy Makkoveev wrote:
>>> @@ -358,20 +353,20 @@ sofree(struct socket *so, int keep_lock)
>>> 		(*so->so_proto->pr_domain->dom_dispose)(so->so_rcv.sb_mb);
>>> 	m_purge(so->so_rcv.sb_mb);
>>> 
>>> -	if (!keep_lock)
>>> +	if (!keep_lock) {
>>> 		sounlock(so);
>>> 
>>> #ifdef SOCKET_SPLICE
>>> -	if (so->so_sp) {
>>> -		/* Reuse splice idle, sounsplice() has been called before. */
>>> -		timeout_set_flags(&so->so_sp->ssp_idleto, soreaper, so,
>>> -		    KCLOCK_NONE, TIMEOUT_PROC | TIMEOUT_MPSAFE);
>>> -		timeout_add(&so->so_sp->ssp_idleto, 0);
>>> -	} else
>>> +		if (so->so_sp) {
>>> +			timeout_del_barrier(&so->so_sp->ssp_idleto);
>>> +			task_del(sosplice_taskq, &so->so_sp->ssp_task);
>>> +			taskq_barrier(sosplice_taskq);
>>> +			pool_put(&sosplice_pool, so->so_sp);
>>> +		}
>>> #endif /* SOCKET_SPLICE */
>>> -	{
>>> -		pool_put(&socket_pool, so);
>>> 	}
>>> +
>>> +	pool_put(&socket_pool, so);
>>> }
>>> 
>>> static inline uint64_t
>> 
>> Here you move the pool_put(&sosplice_pool, so->so_sp) into the
>> if (!keep_lock) block.
>> 
>> Do we leak the so->so_sp when in_pcbdetach() calls sofree(so, 1)?
>> 
>> bluhm
> 
> No we don’t. While following this path, we have two cases:
> 
> 1. Socket was not yet accept()ed, and has no references from
> userland. Such sockets can’t be spliced and so->so_sp is always NULL.
> 
> 2. Socke was accept()ed or was created by socket(). Such sockets
> could be spliced but they also have SS_NOFDREF bit clean. So
> in_pcbdetach() and sofree(so, 1) will leave such sockets with
> destroyed so->so_pcb. It is normal for tcp(4) and udp(4) sockets.
> 
> sofree(struct socket *so, int keep_lock)
> {
>        int persocket = solock_persocket(so);
> 
>        soassertlocked(so);
> 
>        if (so->so_pcb || (so->so_state & SS_NOFDREF) == 0) {
>                if (!keep_lock)
>                        sounlock(so);
>                return;
>        }

We could drop keep_lock arg from sofree() because (keep_lock != 0)
is equal to (so->so_pcb || (so->so_state & SS_NOFDREF) == 0).

But not with this diff.