From owner-svn-src-head@freebsd.org Sun Apr 12 17:41:10 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 26F3D2C42CA; Sun, 12 Apr 2020 17:41:10 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-oi1-f173.google.com (mail-oi1-f173.google.com [209.85.167.173]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 490fDk08yGz4QZx; Sun, 12 Apr 2020 17:41:09 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-oi1-f173.google.com with SMTP id v141so1289592oie.1; Sun, 12 Apr 2020 10:41:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc:content-transfer-encoding; bh=sC6XC70gqxWimnmy0FIJpPtV0uQaNc4VQ/9BiS8AfRs=; b=lXNJN9R2IjtY4smKagsCOohonqII01gBAJsCF8PT/dUi5SkIM6D+SlHvoi0ZrFOqM+ eNArheM/YI6ensM4LFERw2BGGmH7sl53vnktISoKYpVSZiNNugjYcv34AH25QWabXMX3 N7CVqQPz7Vnzr3m8llrHbAr530452D4MhqntxyNrRnf2WK1Rhn16jyuJPiCU74Wu5FsJ ifx3FtQePubVeeqTBftO5UO3oSocJ3n7RSG7P79ZhII7iGDFO/T4g7+dOBOQnuCS1FhL axUpysUp0jbaLczmpam4n8NzKaaXuGemg+m0dEuEkUSlCdtyfp9L//LGAU2Kyt4lWu1l EoGQ== X-Gm-Message-State: AGi0PuZNT+ekzIAXuGKkdYOVQ2krNhJr6RutDbNHKyq3v3OBvW5JQNtw JCavqnxd/dJtuoblN76GTtSwht3X X-Google-Smtp-Source: APiQypLCLxdURC0O/CZjnYaSToL8rNc+m2DpJq31sS5ZNGtgoQPM3dzq+V2VjvsjRPxl6hK1JV7ICg== X-Received: by 2002:aca:b20a:: with SMTP id b10mr8934396oif.101.1586713268411; Sun, 12 Apr 2020 10:41:08 -0700 (PDT) Received: from mail-ot1-f54.google.com (mail-ot1-f54.google.com. [209.85.210.54]) by smtp.gmail.com with ESMTPSA id u199sm4219064oif.25.2020.04.12.10.41.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 12 Apr 2020 10:41:08 -0700 (PDT) Received: by mail-ot1-f54.google.com with SMTP id x11so7009311otp.6; Sun, 12 Apr 2020 10:41:07 -0700 (PDT) X-Received: by 2002:a05:6830:1f39:: with SMTP id e25mr11949601oth.135.1586713267513; Sun, 12 Apr 2020 10:41:07 -0700 (PDT) MIME-Version: 1.0 References: <202004112036.03BKatfm047227@repo.freebsd.org> <55089DD1-2111-4946-964B-0AAC33F9A876@freebsd.org> In-Reply-To: <55089DD1-2111-4946-964B-0AAC33F9A876@freebsd.org> Reply-To: cem@freebsd.org From: Conrad Meyer Date: Sun, 12 Apr 2020 10:40:56 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r359809 - head/sys/netinet To: Michael Tuexen Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 490fDk08yGz4QZx X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-6.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; TAGGED_FROM(0.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 17:41:10 -0000 Hi Michael, On Sun, Apr 12, 2020 at 2:33 AM Michael Tuexen 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 =3D NULL; > sp->ss_next.tqe_prev =3D NULL; > > which I think is OK, since I'm clearing the pointers related to the remov= e > 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 =E2=80=94 it is instead governed by QUEUE_MACRO_DEBUG_TRASH. IMO, we should go ahead and enabled QUEUE_MACRO_DEBUG_TRASH in GENERIC/INVARIANTS =E2=80=94 unlike "TRACE," TRA= SH is inexpensive =E2=80=94 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 point= s > 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 =E2=80=94 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 movin= g 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