From owner-svn-src-head@freebsd.org Mon Nov 6 06:23:36 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DF4F7E52430; Mon, 6 Nov 2017 06:23:36 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 9DEC62672; Mon, 6 Nov 2017 06:23:36 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.104] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 4ED463C2F94; Mon, 6 Nov 2017 16:21:33 +1100 (AEDT) Date: Mon, 6 Nov 2017 16:21:33 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Warner Losh , "Conrad E. Meyer" , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r325386 - head/sys/kern In-Reply-To: <20171105190214.GG2566@kib.kiev.ua> Message-ID: <20171106160346.D987@besplex.bde.org> References: <201711041049.vA4AnZUE096709@repo.freebsd.org> <20171105130607.GA2566@kib.kiev.ua> <20171105173032.GE2566@kib.kiev.ua> <20171105190214.GG2566@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=x6pKjin5z_QBVLWaVYcA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Mon, 06 Nov 2017 06:23:37 -0000 On Sun, 5 Nov 2017, Konstantin Belousov wrote: > On Sun, Nov 05, 2017 at 11:42:51AM -0700, Warner Losh wrote: >> On Sun, Nov 5, 2017 at 11:32 AM, Conrad Meyer wrote: >> >>> E.g., >>> >>> --- a/sys/ufs/ffs/ffs_alloc.c >>> +++ b/sys/ufs/ffs/ffs_alloc.c >>> @@ -304,8 +304,7 @@ retry: >>> } >>> >>> if (bp->b_blkno == bp->b_lblkno) { >>> - if (lbprev >= UFS_NDADDR) >>> - panic("ffs_realloccg: lbprev out of range"); >>> + ASSERT(lbprev < UFS_NDADDR, "ffs_realloccg: lbprev out >>> of range"); >>> bp->b_blkno = fsbtodb(fs, bprev); >>> } >>> >> >> Just a side point: All these should be programming errors. The bogus data This certainly is a programming error. It has 2 style bugs: - it obfuscates a simple test and panic. This is only 4 bytes shorter - it misformats the new version using a long line. >> that comes or could come from the FS itself should remain always-on panics. >> Well, actually, they should transition from always-on panics to some sort >> of degraded mount that would be more resilient in the face of such >> corruption. But failing that, they should remain always-on panics :) > > This is what I said in my reply before the last. > > I still have no idea what is the point cem tries to express. As described in other replies, it is to obfuscate panic() using a macro in cases where the panic is not controlled by INVARIANTS. > Nor I know what should the ASSERT() macro do in the kernel. If the > patch above really about replacing panic() with _K_ASSERT, then I most > likely agree with it, since the error catched is not due to the on-disk > metadata corruption. KASSERT() can be used in this case. It is unclear which case applies. lbprev is the second parameter and it is unclear if it ever comes directly from the disk or has already been checked by callers when it did. The third parameter bprev often does come directly from the disk. A typical call is ffs_realloccg( ip, nb, dp->db[nb], ...). For this call, undefined behaviour has already occurred if nb is out of bounds, so KASSERT() is good enough, but if nb is really insane then the KASSERT() is unreachale since the invalid array access already paniced. Bruce