Date: Sun, 12 Apr 2020 10:40:56 -0700 From: Conrad Meyer <cem@freebsd.org> To: Michael Tuexen <tuexen@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r359809 - head/sys/netinet Message-ID: <CAG6CVpVUydYHha77deAoJ3KNimX1j1sUiG6uwcx9ELOb-tw3Xw@mail.gmail.com> In-Reply-To: <55089DD1-2111-4946-964B-0AAC33F9A876@freebsd.org> References: <202004112036.03BKatfm047227@repo.freebsd.org> <CAG6CVpVNjZ5PBQxYop-cYb5AbjwNPNJ--dtXoeFC7c2o2sZc0Q@mail.gmail.com> <55089DD1-2111-4946-964B-0AAC33F9A876@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Michael,
On Sun, Apr 12, 2020 at 2:33 AM Michael Tuexen <tuexen@freebsd.org> wrote:
> Yes. What I meant is that in the stream scheduler code (sctp_ss_functions=
.c)
> the pattern is
>
> TAILQ_REMOVE(&asoc->ss_data.out.list, sp, ss_next);
> sp->ss_next.tqe_next =3D NULL;
> sp->ss_next.tqe_prev =3D NULL;
>
> which I think is OK, since I'm clearing the pointers related to the remov=
e
> operation. Do you agree?
It is harmless in the sense that it is not a functional change, but I
wouldn't suggest doing so. First, in the location modified,
TAILQ_INSERT immediately subsequent will just overwrite the NULLs.
The compiler almost certainly just optimizes this out. (If you used
TAILQ_CONCAT instead, you can avoid rewriting the pointer chain in the
linked list entirely and only update heads/tails.)
(By the way, I was mistaken about the queue.h TRASH feature being
included in INVARIANTS kernels =E2=80=94 it is instead governed by
QUEUE_MACRO_DEBUG_TRASH. IMO, we should go ahead and enabled
QUEUE_MACRO_DEBUG_TRASH in GENERIC/INVARIANTS =E2=80=94 unlike "TRACE," TRA=
SH
is inexpensive =E2=80=94 and it catches use-after-frees.)
Second, generally one should not manipulate the implementation details
of sys/queue.h directly. In QUEUE_MACRO_DEBUG_TRASH kernels, the
REMOVE operation already stores bogus values in these pointers
("TRASHIT()").
> I totally agree. I'm actually adding more INVARIANTS checks to the SCTP
> code to catch more places where the code does not behave as expected when
> running syzkaller (more on the API testing) and ossfuzz (for the userland
> stack, more on the packet injection side). So you will see more panics
> when using INVARIANTS, for example, now in the timer code. But this point=
s
> me to places I need to look at.
That's good to hear. I agree it is good to assert/panic more in
INVARIANTS, when invariants are violated =E2=80=94 it's why we have it :-).
> > In this use, consider using
> > 'TAILQ_CONCAT(&stcb->asoc.strmout[i].outqueue, &oldstream[i].outqueue,
> > next)' instead of the loop construct.
>
> Thanks for the hint. Wasn't aware of it and didn't consider it more movin=
g over a queue.
No problem. It is often useful to manipulate entire lists cheaply
rather than individual elements. Another common pattern is: under a
lock, TAILQ_SWAP the lock-protected list to an initialized (empty)
stack list-head, drop the lock, and clean up the list outside the
lock.
Best,
Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpVUydYHha77deAoJ3KNimX1j1sUiG6uwcx9ELOb-tw3Xw>