From: Vitaliy Makkoveev Subject: Re: replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED To: Alexander Bluhm Cc: tech@openbsd.org Date: Sat, 15 Feb 2025 01:26:28 +0300 > On 14 Feb 2025, at 19:27, Alexander Bluhm wrote: > > 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? Sure, commit it please. > > 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_ */