From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: Vitaliy Makkoveev <otto@bsdbox.dev>, tech@openbsd.org
Date: Fri, 14 Feb 2025 12:42:25 +0300

On Fri, Feb 14, 2025 at 01:44:08AM +0100, Alexander Bluhm wrote:
> On Fri, Feb 14, 2025 at 02:11:32AM +0300, Vitaliy Makkoveev wrote:
> > > On 14 Feb 2025, at 01:29, Alexander Bluhm <bluhm@openbsd.org> wrote:
> > > 
> > > 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().
> > > 
> > 
> > As you said, sbspace_locked() is trivial, do we really need assertion
> > within?
> 
> The assertion is not about the trivial code in sbspace_locked(),
> but the code that is calling it.  And not in all callers it is
> obvious that the mutex is held.
> 
> On the other hand, there are a lot of locking assertions around.
> Not sure if we need all of them.
> 
> I am undecided.  I tried both ways and ended with this diff.
> 

This could be something trivial like below. I introduced mtx_owned()
specially for sbmtxassertlocked() so it could go too. 

I tested it with and without DIAGNOSTIC

Index: sys/kern/uipc_socket2.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
diff -u -p -r1.178 uipc_socket2.c
--- sys/kern/uipc_socket2.c	13 Feb 2025 14:44:33 -0000	1.178
+++ sys/kern/uipc_socket2.c	14 Feb 2025 09:40:17 -0000
@@ -502,12 +502,13 @@ sosleep_nsec(struct socket *so, void *id
 	return ret;
 }
 
+#ifdef DIAGNOSTIC
 void
 sbmtxassertlocked(struct sockbuf *sb)
 {
-	if (splassert_ctl > 0 && mtx_owned(&sb->sb_mtx) == 0)
-		splassert_fail(0, RW_WRITE, __func__);
+	MUTEX_ASSERT_LOCKED(&sb->sb_mtx);
 }
+#endif
 
 /*
  * Wait for data to arrive at/drain from a socket buffer.
Index: sys/sys/mutex.h
===================================================================
RCS file: /cvs/src/sys/sys/mutex.h,v
diff -u -p -r1.22 mutex.h
--- sys/sys/mutex.h	16 May 2024 09:30:03 -0000	1.22
+++ sys/sys/mutex.h	14 Feb 2025 09:40:18 -0000
@@ -127,9 +127,6 @@ void	mtx_leave(struct mutex *);
 
 #define mtx_init(m, ipl)	mtx_init_flags(m, ipl, NULL, 0)
 
-#define mtx_owned(mtx) \
-	(((mtx)->mtx_owner == curcpu()) || panicstr || db_active)
-
 #ifdef WITNESS
 
 void	_mtx_init_flags(struct mutex *, int, const char *, int,
Index: sys/sys/socketvar.h
===================================================================
RCS file: /cvs/src/sys/sys/socketvar.h,v
diff -u -p -r1.150 socketvar.h
--- sys/sys/socketvar.h	13 Feb 2025 14:44:33 -0000	1.150
+++ sys/sys/socketvar.h	14 Feb 2025 09:40:18 -0000
@@ -224,7 +224,11 @@ 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)
 
+#ifdef DIAGNOSTIC
 void	sbmtxassertlocked(struct sockbuf *);
+#else
+#define sbmtxassertlocked(sb)	do { (void)(sb); } while(0)
+#endif
 
 /*
  * Do we need to notify the other side when I/O is possible?