From: Vitaliy Makkoveev Subject: Re: replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED To: Alexander Bluhm Cc: Vitaliy Makkoveev , tech@openbsd.org Date: Fri, 14 Feb 2025 18:52:55 +0300 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; } 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. 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_ */