Hi Michael, On Sun, Apr 12, 2020 at 2:33 AM Michael Tuexen <tue...@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 _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"