Re: [PATCH V2] xfs: add a few more verifier tests

Dave Chinner
2014-09-09T01:47:24Z

On Tue, Aug 19, 2014 at 02:36:06PM -0500, Eric Sandeen wrote:
> These were exposed by fsfuzzer runs; without them we fail
> in various exciting and sometimes convoluted ways when we
> encounter disk corruption.
>
> Without the MAXLEVELS tests we tend to walk off the end of
> an array in a loop like this:
>
> for (i = 0; i < cur->bc_nlevels; i++) {
> if (cur->bc_bufs[i])
>
> Without the dirblklog test we try to allocate more memory
> than we could possibly hope for and loop forever:
>
> xfs_dabuf_map()
> nfsb = mp->m_dir_geo->fsbcount;
> irecs = kmem_zalloc(sizeof(irec) * nfsb, KM_SLEEP...
>
> As for the logbsize check, that's the convoluted one.
>
> If logbsize is specified at mount time, it's sanitized
> in xfs_parseargs; in particular it makes sure that it's
> not > XLOG_MAX_RECORD_BSIZE.
>
> If not specified at mount time, it comes from the superblock
> via sb_logsunit; this is limited to 256k at mkfs time as well;
> it's copied into m_logbsize in xfs_finish_flags().
>
> However, if for some reason the on-disk value is corrupt and
> too large, nothing catches it. It's a circuitous path, but
> that size eventually finds its way to places that make the kernel
> very unhappy, leading to oopses in xlog_pack_data() because we
> use the size as an index into iclog->ic_data, but the array
> is not necessarily that big.
>
> Anyway - bounds checking when we read from disk is a good thing!
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> --

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

--
Dave Chinner
david@fromorbit.com