From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Make socket lock optional to sorele()
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 3 Jan 2025 13:53:53 +0300

On Thu, Jan 02, 2025 at 06:04:11PM +0100, Alexander Bluhm wrote:
> On Thu, Jan 02, 2025 at 12:42:42PM +0300, Vitaliy Makkoveev wrote:
> > Previously solock() was required() to call sorele() only to make happy
> > the assertion within sbrelease(). Now nothing within sorele() requires
> > solock(), so it could be called lockless. So I propose to rename the
> > `kkep_lock' arg of sorele to `no_unlock' and conceptually allow this.
> 
> I don't like boolean paramter that have no_ in their name.
> 
> Can we remove the keep_lock parameter and explicitly call sounlock()
> before calling sorele() where needed?
> 
> sorele(so, 0) -> sounlock(so); sorele(so)
> sorele(so, 1) -> sorele(so)
> 

Makes sense. I do many of explicit sounlock() before sorele() with
unlocked socket splicing, so the `keep_lock' arg became useless.

Index: sys/kern/uipc_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
diff -u -p -r1.354 uipc_socket.c
--- sys/kern/uipc_socket.c	3 Jan 2025 09:59:25 -0000	1.354
+++ sys/kern/uipc_socket.c	3 Jan 2025 10:48:19 -0000
@@ -250,11 +250,8 @@ solisten(struct socket *so, int backlog)
 }
 
 void
-sorele(struct socket *so, int keep_lock)
+sorele(struct socket *so)
 {
-	if (keep_lock == 0)
-		sounlock(so);
-
 	if (refcnt_rele(&so->so_refcnt) == 0)
 		return;
 
@@ -314,18 +311,23 @@ sofree(struct socket *so, int keep_lock)
 
 			if (so->so_onq != &head->so_q0) {
 				sounlock(so);
-				sorele(head, 0);
+				sounlock(head);
+				sorele(head);
 				return;
 			}
 		}
 
 		soqremque(so, 0);
 
-		if (persocket)
-			sorele(head, 0);
+		if (persocket) {
+			sounlock(head);
+			sorele(head);
+		}
 	}
 
-	sorele(so, keep_lock);
+	if (!keep_lock)
+		sounlock(so);
+	sorele(so);
 }
 
 static inline uint64_t
@@ -438,8 +440,7 @@ discard:
 			sounlock(soback);
 		}
 		sbunlock(&soback->so_rcv);
-		solock(soback);
-		sorele(soback, 0);
+		sorele(soback);
 
 notsplicedback:
 		sblock(&so->so_rcv, SBL_WAIT | SBL_NOINTR);
Index: sys/kern/uipc_socket2.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
diff -u -p -r1.162 uipc_socket2.c
--- sys/kern/uipc_socket2.c	30 Dec 2024 12:12:35 -0000	1.162
+++ sys/kern/uipc_socket2.c	3 Jan 2025 10:48:19 -0000
@@ -110,7 +110,8 @@ soisconnected(struct socket *so)
 			solock(so);
 
 			if (so->so_onq != &head->so_q0) {
-				sorele(head, 0);
+				sounlock(head);
+				sorele(head);
 				return;
 			}
 
@@ -121,8 +122,10 @@ soisconnected(struct socket *so)
 		sorwakeup(head);
 		wakeup_one(&head->so_timeo);
 
-		if (persocket)
-			sorele(head, 0);
+		if (persocket) {
+			sounlock(head);
+			sorele(head);
+		}
 	} else {
 		wakeup(&so->so_timeo);
 		sorwakeup(so);
Index: sys/netinet/in_pcb.c
===================================================================
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
diff -u -p -r1.308 in_pcb.c
--- sys/netinet/in_pcb.c	3 Jan 2025 00:49:26 -0000	1.308
+++ sys/netinet/in_pcb.c	3 Jan 2025 10:48:19 -0000
@@ -636,7 +636,7 @@ in_pcbsolock(struct inpcb *inp)
 		return NULL;
 
 	rw_enter_write(&so->so_lock);
-	sorele(so, 1);
+	sorele(so);
 
 	return so;
 }
Index: sys/sys/socketvar.h
===================================================================
RCS file: /cvs/src/sys/sys/socketvar.h,v
diff -u -p -r1.137 socketvar.h
--- sys/sys/socketvar.h	3 Jan 2025 00:49:26 -0000	1.137
+++ sys/sys/socketvar.h	3 Jan 2025 10:48:19 -0000
@@ -425,7 +425,7 @@ int	socreate(int, struct socket **, int,
 int	sodisconnect(struct socket *);
 struct socket *soalloc(const struct protosw *, int);
 void	sofree(struct socket *, int);
-void	sorele(struct socket *, int);
+void	sorele(struct socket *);
 int	sogetopt(struct socket *, int, int, struct mbuf *);
 void	sohasoutofband(struct socket *);
 void	soisconnected(struct socket *);