> On 16 May 2023, at 18:35, Alexander Bluhm <[email protected]> wrote:
>
> I saw one issue in sysctl_niq(). Another CPU could write mq_maxlen
> and our logic is inconsistent. Below is a fix with read once. Each
> CPU detects its own change, last write wins. Or should we protect
> everything with mq_mtx? Then sysctl 123 -> 345 would have the
> correct old value on both CPUs.
I don’t assume this case as issue, since if we have two concurrent
sysctl calls modifying the same mib we can’t predict which value will
be set last. To me, the "maxlen != mq->mq_maxlen” check could be
avoided and the "!error” or "error == 0” check is pretty enough:
error = sysctl_int(oldp, oldlenp, newp, newlen, &maxlen);
if (!error)
mq_set_maxlen(mq, maxlen);
If you want to keep the check, I prefer to protect everything with
mq_mtx, and push the check within mq_set_maxlen(). At least this is
clean and strictly predictable comparing with
oldmax = newmax = READ_ONCE().
>
> bluhm
>
> Index: kern/uipc_mbuf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.285
> diff -u -p -r1.285 uipc_mbuf.c
> --- kern/uipc_mbuf.c 5 May 2023 01:19:51 -0000 1.285
> +++ kern/uipc_mbuf.c 16 May 2023 15:05:52 -0000
> @@ -1788,7 +1788,7 @@ int
> sysctl_mq(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> void *newp, size_t newlen, struct mbuf_queue *mq)
> {
> - unsigned int maxlen;
> + unsigned int oldmax, newmax;
> int error;
>
> /* All sysctl names at this level are terminal. */
> @@ -1799,10 +1799,10 @@ sysctl_mq(int *name, u_int namelen, void
> case IFQCTL_LEN:
> return (sysctl_rdint(oldp, oldlenp, newp, mq_len(mq)));
> case IFQCTL_MAXLEN:
> - maxlen = mq->mq_maxlen;
> - error = sysctl_int(oldp, oldlenp, newp, newlen, &maxlen);
> - if (!error && maxlen != mq->mq_maxlen)
> - mq_set_maxlen(mq, maxlen);
> + oldmax = newmax = READ_ONCE(mq->mq_maxlen);
> + error = sysctl_int(oldp, oldlenp, newp, newlen, &newmax);
> + if (!error && oldmax != newmax)
> + mq_set_maxlen(mq, newmax);
> return (error);
> case IFQCTL_DROPS:
> return (sysctl_rdint(oldp, oldlenp, newp, mq_drops(mq)));
