From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: fix race in in_pcbsolock_ref()
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Wed, 5 Feb 2025 01:56:17 +0300

> On 5 Feb 2025, at 01:44, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> 
> On Wed, Feb 05, 2025 at 01:29:42AM +0300, Vitaliy Makkoveev wrote:
>>> On 5 Feb 2025, at 01:05, Alexander Bluhm <bluhm@openbsd.org> wrote:
>>> 
>>> Hi,
>>> 
>>> Further testing of parallel TCP input revealed this race in
>>> in_pcbsolock_ref().
>>> 
>>> ok?
>>> 
>> 
>> Do we still need in_pcbsounlock_rele()? We have tcp_sogetpcb() to
>> get inp and tp from socket while performing tcp_usrreqs handlers.
>> Is it reasonable to transform in_pcbsolock_ref() to something like
>> it? I???m asking because this diff implements similar logic.
> 
> in_pcbsolock_ref() is about getting the socket lock from an inpcb.
> tcp_sogetpcb() asumes that the socket is locked and does not care
> about any locks.  I would say they are completely different.
> 
> in_pcbsounlock_rele() exists to have a symmetric unlock function.
> 
> bluhm

I suspect it will be transformed in the future. OK mvs to commit
as is.

> 
>>> Index: netinet/in_pcb.c
>>> ===================================================================
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v
>>> diff -u -p -r1.310 in_pcb.c
>>> --- netinet/in_pcb.c	9 Jan 2025 16:47:24 -0000	1.310
>>> +++ netinet/in_pcb.c	4 Feb 2025 20:55:32 -0000
>>> @@ -635,6 +635,13 @@ in_pcbsolock_ref(struct inpcb *inp)
>>> 	if (so == NULL)
>>> 		return NULL;
>>> 	rw_enter_write(&so->so_lock);
>>> +	/* between mutex and rwlock inpcb could be detached */
>>> +	if (so->so_pcb == NULL) {
>>> +		rw_exit_write(&so->so_lock);
>>> +		sorele(so);
>>> +		return NULL;
>>> +	}
>>> +	KASSERT(inp->inp_socket == so && sotoinpcb(so) == inp);
>>> 	return so;
>>> }
>>> 
>>> @@ -643,7 +650,6 @@ in_pcbsounlock_rele(struct inpcb *inp, s
>>> {
>>> 	if (so == NULL)
>>> 		return;
>>> -	KASSERT(inp->inp_socket == NULL || inp->inp_socket == so);
>>> 	rw_exit_write(&so->so_lock);
>>> 	sorele(so);
>>> }
>>> 
>>