From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: fix race in in_pcbsolock_ref()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 4 Feb 2025 23:44:28 +0100

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

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