Download raw body.
replace sbmtxassertlocked with MUTEX_ASSERT_LOCKED
> On 14 Feb 2025, at 19:27, Alexander Bluhm <alexander.bluhm@gmx.net> 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 <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?
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 <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