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

On Fri, Feb 14, 2025 at 12:42:25PM +0300, Vitaliy Makkoveev wrote:
> 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

As we are building all kernels except install kernel with DIAGNOSTIC
it does not make sense to optimize for non DIAGNOSTIC with #ifdef.

A trivial diff would be to call MUTEX_ASSERT_LOCKED() from
sbmtxassertlocked().  But I also wanted to remove abtraction layer.
Why have a function sbmtxassertlocked() instead of using
MUTEX_ASSERT_LOCKED()?

And then dependencies from functions implemented in header files
kicked in.  I think we have too much code in the header.

bluhm

Index: kern/uipc_socket2.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket2.c,v
diff -u -p -r1.178 uipc_socket2.c
--- kern/uipc_socket2.c	13 Feb 2025 14:44:33 -0000	1.178
+++ kern/uipc_socket2.c	14 Feb 2025 13:50:36 -0000
@@ -505,8 +505,7 @@ sosleep_nsec(struct socket *so, void *id
 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);
 }
 
 /*