On Mon, Jan 20, 2025 at 03:42:48PM +0100, Alexander Bluhm wrote:
> On Mon, Jan 20, 2025 at 05:29:01PM +0300, Vitaliy Makkoveev wrote:
> > On Mon, Jan 20, 2025 at 01:37:24PM +0100, Alexander Bluhm wrote:
> > > Hi,
> > > 
> > > I want to make UNIX and TCP socket locking more similar.
> > > 
> > > One difference is that UNIX sockets unlock in soabort() while TCP
> > > does not do that.  in_pcbdetach() does keep the lock, so let's
> > > change uipc_abort().  This also gives symetric lock and unlock in
> > > the caller.  Refcount is needed to call unlock on an aborted socket.
> > > 
> > > The queue 0 in soclose() is only used by UNIX sockets, so replace
> > > the "if" with "kassert".
> > > 
> > > ok?
> > > 
> > 
> > The diff is ok mvs@, but do we really need this assertion?
> > 
> > > +		if (!TAILQ_EMPTY(&so->so_q0))
> > > +			KASSERT(persocket);
> 
> Not really, I can remove it.  It passed regress once with assertion,
> that should be enough.  If you want, I can also remove the same
> assertion in soisconnected().
> 

Do it please.

> bluhm
> 
> > > Index: kern/uipc_socket.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> > > diff -u -p -r1.360 uipc_socket.c
> > > --- kern/uipc_socket.c	13 Jan 2025 18:10:20 -0000	1.360
> > > +++ kern/uipc_socket.c	18 Jan 2025 16:40:02 -0000
> > > @@ -399,23 +399,27 @@ drop:
> > >  	if (so->so_options & SO_ACCEPTCONN) {
> > >  		int persocket = solock_persocket(so);
> > >  
> > > +		if (!TAILQ_EMPTY(&so->so_q0))
> > > +			KASSERT(persocket);
> > >  		while ((so2 = TAILQ_FIRST(&so->so_q0)) != NULL) {
> > > -			if (persocket)
> > > -				solock(so2);
> > > +			soref(so2);
> > > +			solock(so2);
> > >  			(void) soqremque(so2, 0);
> > > -			if (persocket)
> > > -				sounlock(so);
> > > +			sounlock(so);
> > >  			soabort(so2);
> > > -			if (persocket)
> > > -				solock(so);
> > > +			sounlock(so2);
> > > +			sorele(so2);
> > > +			solock(so);
> > >  		}
> > >  		while ((so2 = TAILQ_FIRST(&so->so_q)) != NULL) {
> > > -			if (persocket)
> > > -				solock(so2);
> > > +			soref(so2);
> > > +			solock_nonet(so2);
> > >  			(void) soqremque(so2, 1);
> > >  			if (persocket)
> > >  				sounlock(so);
> > >  			soabort(so2);
> > > +			sounlock_nonet(so2);
> > > +			sorele(so2);
> > >  			if (persocket)
> > >  				solock(so);
> > >  		}
> > > Index: kern/uipc_usrreq.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_usrreq.c,v
> > > diff -u -p -r1.212 uipc_usrreq.c
> > > --- kern/uipc_usrreq.c	1 Jan 2025 13:44:22 -0000	1.212
> > > +++ kern/uipc_usrreq.c	18 Jan 2025 16:24:10 -0000
> > > @@ -656,7 +656,7 @@ uipc_abort(struct socket *so)
> > >  	struct unpcb *unp = sotounpcb(so);
> > >  
> > >  	unp_detach(unp);
> > > -	sofree(so, 0);
> > > +	sofree(so, 1);
> > >  }
> > >  
> > >  int
> > > 
>