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

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.

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);
> > 
> 
>