Date: Mon, 6 Nov 2017 16:21:33 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Warner Losh <imp@bsdimp.com>, "Conrad E. Meyer" <cem@freebsd.org>, src-committers <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r325386 - head/sys/kern Message-ID: <20171106160346.D987@besplex.bde.org> In-Reply-To: <20171105190214.GG2566@kib.kiev.ua> References: <201711041049.vA4AnZUE096709@repo.freebsd.org> <CAG6CVpUaXSX26Bc839kn96EXnUjtGyQ3_eNJPhsRB%2Bv6G6gH1Q@mail.gmail.com> <20171105130607.GA2566@kib.kiev.ua> <CAG6CVpV3SfV8VuNeMJEuN%2BMSu3424mK=HO_-YW9vRt9HEdcJZQ@mail.gmail.com> <20171105173032.GE2566@kib.kiev.ua> <CAG6CVpVQ4RSgz883ezsdExVDQ4RbSiR_y%2BZ5XWDNFeYBpMS-cQ@mail.gmail.com> <CANCZdfqApxd99G-HqVfe4x8EysYJVCAcHksqTeUuBAaYR1OjEA@mail.gmail.com> <20171105190214.GG2566@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <cem@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171106160346.D987>