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

> 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?

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