svn commit: r359809 - head/sys/netinet

[Conrad Meyer]

Hi Michael,

On Sun, Apr 12, 2020 at 2:33 AM Michael Tuexen <tuexen at 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 = NULL;
>         sp->ss_next.tqe_prev = NULL;
>
> which I think is OK, since I'm clearing the pointers related to the remove
> 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 — it is instead governed by
QUEUE_MACRO_DEBUG_TRASH.  IMO, we should go ahead and enabled
QUEUE_MACRO_DEBUG_TRASH in GENERIC/INVARIANTS — unlike "TRACE," TRASH
is inexpensive — 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 points
> 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 — 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 moving 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