Download raw body.
replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED
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 <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.
> > > >
> > >
> > > 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 <sys/protosw.h>
> #include <lib/libkern/libkern.h>
>
> +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_ */
replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED