From: Alexander Bluhm Subject: Re: replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED To: Vitaliy Makkoveev Cc: Vitaliy Makkoveev , tech@openbsd.org Date: Fri, 14 Feb 2025 17:27:30 +0100 On Fri, Feb 14, 2025 at 06:52:55PM +0300, Vitaliy Makkoveev wrote: > On Fri, Feb 14, 2025 at 02:54:57PM +0100, Alexander Bluhm wrote: > > 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 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 > > > > Since we always do sbmtxassertlocked() function call, there is no > performance impact if you just call sbspace_locked() from sbspace() > instead of unrolling it. I prefer to follow this way. > > long sbspace_locked(struct sockbuf *); > > static inline long > sbspace(struct sockbuf *sb) > { > long ret; > > mtx_enter(&sb->sb_mtx); > ret = sbspace_locked(sb); > mtx_leave(&sb->sb_mtx); > > return ret; > } I think we should split discussion about mutex assertion failure and cleaning inline mess. OK for this minimal diff? 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); } /* > You could say, socketvar.h is the mess of declarations and inline > functions, so we could reorder them and move inline functions to the > end. OK bluhm@ for the diff below. After that we can discuss about inline mess and if sbmtxassertlocked() function is neccessary. > 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 15:51:09 -0000 > @@ -205,8 +205,118 @@ struct socket { > #include > #include > > +struct mbuf; > +struct sockaddr; > +struct proc; > +struct msghdr; > +struct stat; > +struct knote; > + > void soassertlocked(struct socket *); > void soassertlocked_readonly(struct socket *); > +void sbmtxassertlocked(struct sockbuf *); > + > +int soo_read(struct file *, struct uio *, int); > +int soo_write(struct file *, struct uio *, int); > +int soo_ioctl(struct file *, u_long, caddr_t, struct proc *); > +int soo_kqfilter(struct file *, struct knote *); > +int soo_close(struct file *, struct proc *); > +int soo_stat(struct file *, struct stat *, struct proc *); > +void sbappend(struct socket *, struct sockbuf *, struct mbuf *); > +void sbappendstream(struct socket *, struct sockbuf *, struct mbuf *); > +int sbappendaddr(struct socket *, struct sockbuf *, > + const struct sockaddr *, struct mbuf *, struct mbuf *); > +int sbappendcontrol(struct socket *, struct sockbuf *, struct mbuf *, > + struct mbuf *); > +void sbappendrecord(struct socket *, struct sockbuf *, struct mbuf *); > +void sbcompress(struct sockbuf *, struct mbuf *, struct mbuf *); > +struct mbuf * > + sbcreatecontrol(const void *, size_t, int, int); > +void sbdrop(struct sockbuf *, int); > +void sbdroprecord(struct sockbuf *); > +void sbflush(struct sockbuf *); > +void sbrelease(struct sockbuf *); > +int sbcheckreserve(u_long, u_long); > +int sbchecklowmem(void); > +int sbreserve(struct socket *, struct sockbuf *, u_long); > +int sbwait(struct sockbuf *); > +void soinit(void); > +void soabort(struct socket *); > +int soaccept(struct socket *, struct mbuf *); > +int sobind(struct socket *, struct mbuf *, struct proc *); > +void socantrcvmore(struct socket *); > +void socantsendmore(struct socket *); > +int soclose(struct socket *, int); > +int soconnect(struct socket *, struct mbuf *); > +int soconnect2(struct socket *, struct socket *); > +int socreate(int, struct socket **, int, int); > +int sodisconnect(struct socket *); > +struct socket *soalloc(const struct protosw *, int); > +void sofree(struct socket *, int); > +void sorele(struct socket *); > +int sogetopt(struct socket *, int, int, struct mbuf *); > +void sohasoutofband(struct socket *); > +void soisconnected(struct socket *); > +void soisconnecting(struct socket *); > +void soisdisconnected(struct socket *); > +void soisdisconnecting(struct socket *); > +int solisten(struct socket *, int); > +struct socket *sonewconn(struct socket *, int, int); > +void soqinsque(struct socket *, struct socket *, int); > +int soqremque(struct socket *, int); > +int soreceive(struct socket *, struct mbuf **, struct uio *, > + struct mbuf **, struct mbuf **, int *, socklen_t); > +int soreserve(struct socket *, u_long, u_long); > +int sosend(struct socket *, struct mbuf *, struct uio *, > + struct mbuf *, struct mbuf *, int); > +int sosetopt(struct socket *, int, int, struct mbuf *); > +int soshutdown(struct socket *, int); > +void sowakeup(struct socket *, struct sockbuf *); > +void sorwakeup(struct socket *); > +void sowwakeup(struct socket *); > +int sockargs(struct mbuf **, const void *, size_t, int); > + > +int sosleep_nsec(struct socket *, void *, int, const char *, uint64_t); > +void solock(struct socket *); > +void solock_shared(struct socket *); > +void solock_nonet(struct socket *); > +int solock_persocket(struct socket *); > +void solock_pair(struct socket *, struct socket *); > +void sounlock(struct socket *); > +void sounlock_shared(struct socket *); > +void sounlock_nonet(struct socket *); > +void sounlock_pair(struct socket *, struct socket *); > + > +int sendit(struct proc *, int, struct msghdr *, int, register_t *); > +int recvit(struct proc *, int, struct msghdr *, caddr_t, register_t *); > +int doaccept(struct proc *, int, struct sockaddr *, socklen_t *, int, > + register_t *); > + > +#ifdef SOCKBUF_DEBUG > +void sblastrecordchk(struct sockbuf *, const char *); > +#define SBLASTRECORDCHK(sb, where) sblastrecordchk((sb), (where)) > + > +void sblastmbufchk(struct sockbuf *, const char *); > +#define SBLASTMBUFCHK(sb, where) sblastmbufchk((sb), (where)) > +void sbcheck(struct socket *, struct sockbuf *); > +#define SBCHECK(so, sb) sbcheck((so), (sb)) > +#else > +#define SBLASTRECORDCHK(sb, where) /* nothing */ > +#define SBLASTMBUFCHK(sb, where) /* nothing */ > +#define SBCHECK(so, sb) /* nothing */ > +#endif /* SOCKBUF_DEBUG */ > + > +/* > + * Flags to sblock() > + */ > +#define SBL_WAIT 0x01 /* Wait if lock not immediately available. */ > +#define SBL_NOINTR 0x02 /* Enforce non-interruptible sleep. */ > + > +int sblock(struct sockbuf *, int); > +void sbunlock(struct sockbuf *); > + > +extern u_long sb_max; > +extern struct pool socket_pool; > > static inline struct socket * > soref(struct socket *so) > @@ -224,8 +334,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? > */ > @@ -323,23 +431,6 @@ sbfree(struct sockbuf *sb, struct mbuf * > sb->sb_mbcnt -= m->m_ext.ext_size; > } > > -/* > - * Flags to sblock() > - */ > -#define SBL_WAIT 0x01 /* Wait if lock not immediately available. */ > -#define SBL_NOINTR 0x02 /* Enforce non-interruptible sleep. */ > - > -/* > - * Set lock on sockbuf sb; sleep if lock is already held. > - * Unless SB_NOINTR is set on sockbuf or SBL_NOINTR passed, > - * sleep is interruptible. Returns error without lock if > - * sleep is interrupted. > - */ > -int sblock(struct sockbuf *, int); > - > -/* release lock on sockbuf sb */ > -void sbunlock(struct sockbuf *); > - > static inline void > sbassertlocked(struct sockbuf *sb) > { > @@ -353,110 +444,5 @@ sbassertlocked(struct sockbuf *sb) > } \ > } while (/*CONSTCOND*/0) > > -extern u_long sb_max; > - > -extern struct pool socket_pool; > - > -struct mbuf; > -struct sockaddr; > -struct proc; > -struct msghdr; > -struct stat; > -struct knote; > - > -/* > - * File operations on sockets. > - */ > -int soo_read(struct file *, struct uio *, int); > -int soo_write(struct file *, struct uio *, int); > -int soo_ioctl(struct file *, u_long, caddr_t, struct proc *); > -int soo_kqfilter(struct file *, struct knote *); > -int soo_close(struct file *, struct proc *); > -int soo_stat(struct file *, struct stat *, struct proc *); > -void sbappend(struct socket *, struct sockbuf *, struct mbuf *); > -void sbappendstream(struct socket *, struct sockbuf *, struct mbuf *); > -int sbappendaddr(struct socket *, struct sockbuf *, > - const struct sockaddr *, struct mbuf *, struct mbuf *); > -int sbappendcontrol(struct socket *, struct sockbuf *, struct mbuf *, > - struct mbuf *); > -void sbappendrecord(struct socket *, struct sockbuf *, struct mbuf *); > -void sbcompress(struct sockbuf *, struct mbuf *, struct mbuf *); > -struct mbuf * > - sbcreatecontrol(const void *, size_t, int, int); > -void sbdrop(struct sockbuf *, int); > -void sbdroprecord(struct sockbuf *); > -void sbflush(struct sockbuf *); > -void sbrelease(struct sockbuf *); > -int sbcheckreserve(u_long, u_long); > -int sbchecklowmem(void); > -int sbreserve(struct socket *, struct sockbuf *, u_long); > -int sbwait(struct sockbuf *); > -void soinit(void); > -void soabort(struct socket *); > -int soaccept(struct socket *, struct mbuf *); > -int sobind(struct socket *, struct mbuf *, struct proc *); > -void socantrcvmore(struct socket *); > -void socantsendmore(struct socket *); > -int soclose(struct socket *, int); > -int soconnect(struct socket *, struct mbuf *); > -int soconnect2(struct socket *, struct socket *); > -int socreate(int, struct socket **, int, int); > -int sodisconnect(struct socket *); > -struct socket *soalloc(const struct protosw *, int); > -void sofree(struct socket *, int); > -void sorele(struct socket *); > -int sogetopt(struct socket *, int, int, struct mbuf *); > -void sohasoutofband(struct socket *); > -void soisconnected(struct socket *); > -void soisconnecting(struct socket *); > -void soisdisconnected(struct socket *); > -void soisdisconnecting(struct socket *); > -int solisten(struct socket *, int); > -struct socket *sonewconn(struct socket *, int, int); > -void soqinsque(struct socket *, struct socket *, int); > -int soqremque(struct socket *, int); > -int soreceive(struct socket *, struct mbuf **, struct uio *, > - struct mbuf **, struct mbuf **, int *, socklen_t); > -int soreserve(struct socket *, u_long, u_long); > -int sosend(struct socket *, struct mbuf *, struct uio *, > - struct mbuf *, struct mbuf *, int); > -int sosetopt(struct socket *, int, int, struct mbuf *); > -int soshutdown(struct socket *, int); > -void sowakeup(struct socket *, struct sockbuf *); > -void sorwakeup(struct socket *); > -void sowwakeup(struct socket *); > -int sockargs(struct mbuf **, const void *, size_t, int); > - > -int sosleep_nsec(struct socket *, void *, int, const char *, uint64_t); > -void solock(struct socket *); > -void solock_shared(struct socket *); > -void solock_nonet(struct socket *); > -int solock_persocket(struct socket *); > -void solock_pair(struct socket *, struct socket *); > -void sounlock(struct socket *); > -void sounlock_shared(struct socket *); > -void sounlock_nonet(struct socket *); > -void sounlock_pair(struct socket *, struct socket *); > - > -int sendit(struct proc *, int, struct msghdr *, int, register_t *); > -int recvit(struct proc *, int, struct msghdr *, caddr_t, register_t *); > -int doaccept(struct proc *, int, struct sockaddr *, socklen_t *, int, > - register_t *); > - > -#ifdef SOCKBUF_DEBUG > -void sblastrecordchk(struct sockbuf *, const char *); > -#define SBLASTRECORDCHK(sb, where) sblastrecordchk((sb), (where)) > - > -void sblastmbufchk(struct sockbuf *, const char *); > -#define SBLASTMBUFCHK(sb, where) sblastmbufchk((sb), (where)) > -void sbcheck(struct socket *, struct sockbuf *); > -#define SBCHECK(so, sb) sbcheck((so), (sb)) > -#else > -#define SBLASTRECORDCHK(sb, where) /* nothing */ > -#define SBLASTMBUFCHK(sb, where) /* nothing */ > -#define SBCHECK(so, sb) /* nothing */ > -#endif /* SOCKBUF_DEBUG */ > - > #endif /* _KERNEL */ > - > #endif /* _SYS_SOCKETVAR_H_ */