From owner-svn-src-head@freebsd.org Sun Apr 12 09:33:34 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 0531D2B7FAC; Sun, 12 Apr 2020 09:33:34 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from drew.franken.de (drew.ipv6.franken.de [IPv6:2001:638:a02:a001:20e:cff:fe4a:feaa]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.franken.de", Issuer "COMODO RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 490RQ561klz3PNM; Sun, 12 Apr 2020 09:33:33 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from mbp.fritz.box (ip4d16e760.dynamic.kabel-deutschland.de [77.22.231.96]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTPSA id 01BB272243807; Sun, 12 Apr 2020 11:33:27 +0200 (CEST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: svn commit: r359809 - head/sys/netinet From: Michael Tuexen In-Reply-To: Date: Sun, 12 Apr 2020 11:33:27 +0200 Cc: src-committers , svn-src-all , svn-src-head Content-Transfer-Encoding: quoted-printable Message-Id: <55089DD1-2111-4946-964B-0AAC33F9A876@freebsd.org> References: <202004112036.03BKatfm047227@repo.freebsd.org> To: cem@freebsd.org X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=disabled version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mail-n.franken.de X-Rspamd-Queue-Id: 490RQ561klz3PNM X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-6.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.997,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 12 Apr 2020 09:33:34 -0000 > On 11. Apr 2020, at 23:35, Conrad Meyer wrote: >=20 > Hi Michael, >=20 > On Sat, Apr 11, 2020 at 1:37 PM Michael Tuexen = 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