> On 4 May 2023, at 20:52, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
>
> Hi,
>
> The mbuf_queue API allows read access to integer variables which
> another CPU may change simultaneously. To prevent miss-optimisations
> by the compiler, they need the READ_ONCE() macro. Otherwise there
> could be two read operations with inconsistent values.
>
> Writing to integer in mq_set_maxlen() needs mutex protection.
> Otherwise the value could change during critical sections. Again
> the compiler could optimize to multiple read operations within the
> critical section. With inconsistent values, the behavior is
> undefined.
>
> The header file diff survived a make release.
>
> ok?
ok
>
> bluhm
>
> Index: kern/uipc_mbuf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.284
> diff -u -p -r1.284 uipc_mbuf.c
> --- kern/uipc_mbuf.c 14 Aug 2022 01:58:28 -0000 1.284
> +++ kern/uipc_mbuf.c 4 May 2023 07:24:13 -0000
> @@ -1776,6 +1776,14 @@ mq_hdatalen(struct mbuf_queue *mq)
> return (hdatalen);
> }
>
> +void
> +mq_set_maxlen(struct mbuf_queue *mq, u_int maxlen)
> +{
> + mtx_enter(&mq->mq_mtx);
> + mq->mq_maxlen = maxlen;
> + mtx_leave(&mq->mq_mtx);
> +}
> +
> int
> sysctl_mq(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> void *newp, size_t newlen, struct mbuf_queue *mq)
> @@ -1793,11 +1801,8 @@ sysctl_mq(int *name, u_int namelen, void
> case IFQCTL_MAXLEN:
> maxlen = mq->mq_maxlen;
> error = sysctl_int(oldp, oldlenp, newp, newlen, &maxlen);
> - if (!error && maxlen != mq->mq_maxlen) {
> - mtx_enter(&mq->mq_mtx);
> - mq->mq_maxlen = maxlen;
> - mtx_leave(&mq->mq_mtx);
> - }
> + if (!error && maxlen != mq->mq_maxlen)
> + mq_set_maxlen(mq, maxlen);
> return (error);
> case IFQCTL_DROPS:
> return (sysctl_rdint(oldp, oldlenp, newp, mq_drops(mq)));
> Index: sys/mbuf.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/mbuf.h,v
> retrieving revision 1.255
> diff -u -p -r1.255 mbuf.h
> --- sys/mbuf.h 15 Aug 2022 16:15:37 -0000 1.255
> +++ sys/mbuf.h 4 May 2023 07:26:00 -0000
> @@ -526,6 +526,8 @@ unsigned int ml_hdatalen(struct mbuf_li
> * mbuf queues
> */
>
> +#include <sys/atomic.h>
> +
> #define MBUF_QUEUE_INITIALIZER(_maxlen, _ipl) \
> { MUTEX_INITIALIZER(_ipl), MBUF_LIST_INITIALIZER(), (_maxlen), 0 }
>
> @@ -538,12 +540,12 @@ void mq_delist(struct mbuf_queue *, st
> struct mbuf * mq_dechain(struct mbuf_queue *);
> unsigned int mq_purge(struct mbuf_queue *);
> unsigned int mq_hdatalen(struct mbuf_queue *);
> +void mq_set_maxlen(struct mbuf_queue *, u_int);
>
> -#define mq_len(_mq) ml_len(&(_mq)->mq_list)
> -#define mq_empty(_mq) ml_empty(&(_mq)->mq_list)
> -#define mq_full(_mq) (mq_len((_mq)) >= (_mq)->mq_maxlen)
> -#define mq_drops(_mq) ((_mq)->mq_drops)
> -#define mq_set_maxlen(_mq, _l) ((_mq)->mq_maxlen = (_l))
> +#define mq_len(_mq) READ_ONCE((_mq)->mq_list.ml_len)
> +#define mq_empty(_mq) (mq_len(_mq) == 0)
> +#define mq_full(_mq) (mq_len((_mq)) >= READ_ONCE((_mq)->mq_maxlen))
> +#define mq_drops(_mq) READ_ONCE((_mq)->mq_drops)
>
> #endif /* _KERNEL */
> #endif /* _SYS_MBUF_H_ */
>
