Date: Sun, 12 Apr 2020 11:33:27 +0200 From: Michael Tuexen <tuexen@freebsd.org> To: cem@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: <55089DD1-2111-4946-964B-0AAC33F9A876@freebsd.org> In-Reply-To: <CAG6CVpVNjZ5PBQxYop-cYb5AbjwNPNJ--dtXoeFC7c2o2sZc0Q@mail.gmail.com> References: <202004112036.03BKatfm047227@repo.freebsd.org> <CAG6CVpVNjZ5PBQxYop-cYb5AbjwNPNJ--dtXoeFC7c2o2sZc0Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 11. Apr 2020, at 23:35, Conrad Meyer <cem@FreeBSD.org> wrote: >=20 > Hi Michael, >=20 > On Sat, Apr 11, 2020 at 1:37 PM Michael Tuexen <tuexen@freebsd.org> = wrote: >>=20 >> Author: tuexen >> Date: Sat Apr 11 20:36:54 2020 >> New Revision: 359809 >> URL: https://svnweb.freebsd.org/changeset/base/359809 >>=20 >> Log: >> Zero out pointers for consistency. >>=20 >> This was found by running syzkaller on an INVARIANTS kernel. >=20 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 =3D NULL; sp->ss_next.tqe_prev =3D 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 =3D NULL; sp->next.tqe_prev =3D 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 =3D NULL; sp->ss_next.tqe_prev =3D 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. >=20 > 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 >=20 > Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55089DD1-2111-4946-964B-0AAC33F9A876>