svn commit: r359809 - head/sys/netinet

[Michael Tuexen]

> On 11. Apr 2020, at 23:35, Conrad Meyer <cem at FreeBSD.org> wrote:
> 
> Hi Michael,
> 
> On Sat, Apr 11, 2020 at 1:37 PM Michael Tuexen <tuexen at freebsd.org> wrote:
>> 
>> Author: tuexen
>> Date: Sat Apr 11 20:36:54 2020
>> New Revision: 359809
>> URL: https://svnweb.freebsd.org/changeset/base/359809
>> 
>> Log:
>>  Zero out pointers for consistency.
>> 
>>  This was found by running syzkaller on an INVARIANTS kernel.
> 
Hi Conrad,
> For consistency?  If syzkaller found something due to INVARIANTS
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?

While looking at the code

	TAILQ_FOREACH_SAFE(sp, &oldstream[i].outqueue, next, nsp) {
		TAILQ_REMOVE(&oldstream[i].outqueue, sp, next);
		TAILQ_INSERT_TAIL(&stcb->asoc.strmout[i].outqueue, sp, next);
	}

I observed that I don't clear the pointers after the remove operation.
The intended change was adding

		sp->next.tqe_next = NULL;
		sp->next.tqe_prev = NULL;

which I guess would be fine. Do you agree?

Due to a copy/paste error the change was (but not intended) adding

		sp->ss_next.tqe_next = NULL;
		sp->ss_next.tqe_prev = NULL;

Unfortunately testing this incorrect and unintended fix, resolved the
kernel panic. BTW, the intended fix doesn't fix the panic.

Therefore I've reverted the fix: https://svnweb.freebsd.org/changeset/base/359822

Thanks a lot for making me aware of my mistake!
> sys/queue.h debugging trashing the pointer values, masking them by
> writing zeroes doesn't help.  Generally, defeating the kernel's
> INVARIANTS system is not wise or useful.
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.
> 
> 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.

Best regards
Michael
> 
> Conrad