From: Alexander Bluhm <bluhm@openbsd.org>
Subject: replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED
To: tech@openbsd.org
Date: Thu, 13 Feb 2025 23:29:07 +0100

Hi,

Working on socket buffer protection has been stable for a while.
I think it is time to convert this soft failure to mutex assertion.
Fail early, fail hard.

MUTEX_ASSERT_LOCKED() cannot be used in header file due to missing
include dependencies.  So I moved sbspace_locked() into .c file.
It is not more overhead than before when we called sbmtxassertlocked().
I am not a big fan of much code in header inline functions anyway.
The implementation is so trivial that I copied it to sbspace().

ok?

bluhm

Index: kern/uipc_socket2.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket2.c,v
diff -u -p -r1.177 uipc_socket2.c
--- kern/uipc_socket2.c	6 Feb 2025 13:39:31 -0000	1.177
+++ kern/uipc_socket2.c	13 Feb 2025 18:06:59 -0000
@@ -502,13 +502,6 @@ sosleep_nsec(struct socket *so, void *id
 	return ret;
 }
 
-void
-sbmtxassertlocked(struct sockbuf *sb)
-{
-	if (splassert_ctl > 0 && mtx_owned(&sb->sb_mtx) == 0)
-		splassert_fail(0, RW_WRITE, __func__);
-}
-
 /*
  * Wait for data to arrive at/drain from a socket buffer.
  */
@@ -646,7 +639,7 @@ bad:
 int
 sbreserve(struct socket *so, struct sockbuf *sb, u_long cc)
 {
-	sbmtxassertlocked(sb);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 
 	if (cc == 0 || cc > sb_max)
 		return (1);
@@ -688,6 +681,14 @@ sbchecklowmem(void)
 	return (atomic_load_int(&sblowmem));
 }
 
+long
+sbspace_locked(struct sockbuf *sb)
+{
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
+
+	return lmin(sb->sb_hiwat - sb->sb_cc, sb->sb_mbmax - sb->sb_mbcnt);
+}
+
 /*
  * Free mbufs held by a socket, and reserved mbuf space.
  */
@@ -790,10 +791,11 @@ sbappend(struct socket *so, struct sockb
 {
 	struct mbuf *n;
 
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
+
 	if (m == NULL)
 		return;
 
-	sbmtxassertlocked(sb);
 	SBLASTRECORDCHK(sb, "sbappend 1");
 
 	if ((n = sb->sb_lastrecord) != NULL) {
@@ -827,7 +829,7 @@ sbappend(struct socket *so, struct sockb
 void
 sbappendstream(struct socket *so, struct sockbuf *sb, struct mbuf *m)
 {
-	sbmtxassertlocked(sb);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 	KDASSERT(m->m_nextpkt == NULL);
 	KASSERT(sb->sb_mb == sb->sb_lastrecord);
 
@@ -873,7 +875,7 @@ sbappendrecord(struct socket *so, struct
 {
 	struct mbuf *m;
 
-	sbmtxassertlocked(sb);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 
 	if (m0 == NULL)
 		return;
@@ -908,7 +910,7 @@ sbappendaddr(struct socket *so, struct s
 	struct mbuf *m, *n, *nlast;
 	int space = asa->sa_len;
 
-	sbmtxassertlocked(sb);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 
 	if (m0 && (m0->m_flags & M_PKTHDR) == 0)
 		panic("sbappendaddr");
@@ -957,7 +959,7 @@ sbappendcontrol(struct socket *so, struc
 	struct mbuf *m, *mlast, *n;
 	int eor = 0, space = 0;
 
-	sbmtxassertlocked(sb);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 
 	if (control == NULL)
 		panic("sbappendcontrol");
@@ -1082,7 +1084,7 @@ sbdrop(struct sockbuf *sb, int len)
 	struct mbuf *m, *mn;
 	struct mbuf *next;
 
-	sbmtxassertlocked(sb);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 
 	next = (m = sb->sb_mb) ? m->m_nextpkt : NULL;
 	while (len > 0) {
Index: sys/socketvar.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/socketvar.h,v
diff -u -p -r1.149 socketvar.h
--- sys/socketvar.h	6 Feb 2025 13:39:31 -0000	1.149
+++ sys/socketvar.h	13 Feb 2025 18:06:24 -0000
@@ -224,8 +224,6 @@ soref(struct socket *so)
 #define isspliced(so)		((so)->so_sp && (so)->so_sp->ssp_socket)
 #define issplicedback(so)	((so)->so_sp && (so)->so_sp->ssp_soback)
 
-void	sbmtxassertlocked(struct sockbuf *);
-
 /*
  * Do we need to notify the other side when I/O is possible?
  */
@@ -250,20 +248,12 @@ sb_notify(struct sockbuf *sb)
  */
 
 static inline long
-sbspace_locked(struct sockbuf *sb)
-{
-	sbmtxassertlocked(sb);
-
-	return lmin(sb->sb_hiwat - sb->sb_cc, sb->sb_mbmax - sb->sb_mbcnt);
-}
-
-static inline long
 sbspace(struct sockbuf *sb)
 {
 	long ret;
 
 	mtx_enter(&sb->sb_mtx);
-	ret = sbspace_locked(sb);
+	ret = lmin(sb->sb_hiwat - sb->sb_cc, sb->sb_mbmax - sb->sb_mbcnt);
 	mtx_leave(&sb->sb_mtx);
 
 	return ret;
@@ -389,6 +379,7 @@ void	sbflush(struct sockbuf *);
 void	sbrelease(struct socket *, struct sockbuf *);
 int	sbcheckreserve(u_long, u_long);
 int	sbchecklowmem(void);
+long	sbspace_locked(struct sockbuf *);
 int	sbreserve(struct socket *, struct sockbuf *, u_long);
 int	sbwait(struct sockbuf *);
 void	soinit(void);