On Tue, Jun 05, 2018 at 02:39:16PM +1000, Dave Chinner wrote: > On Mon, Jun 04, 2018 at 09:02:32PM -0700, Darrick J. Wong wrote: > > On Tue, Jun 05, 2018 at 12:43:13PM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > > > So we don't check the validity of records as we walk the btree. When > > > there are corrupt records in the free space btree (e.g. zero > > > startblock/length or beyond EOAG) we just blindly use it and things > > > go bad from there. That leads to assert failures on debug kernels > > > like this: > .... > > > @@ -222,12 +224,24 @@ xfs_alloc_get_rec( > > > if (error || !(*stat)) > > > return error; > > > if (rec->alloc.ar_blockcount == 0) > > > - return -EFSCORRUPTED; > > > + goto out_bad_rec; > > > > > > *bno = be32_to_cpu(rec->alloc.ar_startblock); > > > *len = be32_to_cpu(rec->alloc.ar_blockcount); > > > > > > - return error; > > > + if (!xfs_verify_agbno(mp, agno, *bno) || > > > + !xfs_verify_agbno(mp, agno, *bno + *len - 1)) > > > > What about overflows? > > I guess that also means I have to add a (*bno > *bno + *len) check > because it's all unsigned, right? > > .... > > > > + xfs_inobt_btrec_to_irec(mp, rec, irec); > > > + > > > + if (!xfs_verify_agino(mp, agno, irec->ir_startino)) > > > + goto out_bad_rec; > > > + if (irec->ir_count < XFS_INODES_PER_HOLEMASK_BIT || > > > + irec->ir_count > XFS_INODES_PER_CHUNK) > > > + goto out_bad_rec; > > > + if (irec->ir_freecount > XFS_INODES_PER_CHUNK) > > > + goto out_bad_rec; > > > + > > > + /* if there are no holes, return the first available offset */ > > > + if (!xfs_inobt_issparse(irec->ir_holemask)) > > > + realfree = irec->ir_free; > > > + else > > > + realfree = irec->ir_free & xfs_inobt_irec_to_allocmask(irec); > > > + if (hweight64(realfree) != irec->ir_freecount) > > > + goto out_bad_rec; > > > > Hmmm... there's a fair amount of shared logic between this and > > xfs_scrub_iallocbt_rec(). > > Probably, I did just wrote the checks from first principles. xfs_scrub_iallocbt_rec() is a lot more heavyweight. It does a lot more stuff that what I just added, but I'm not sure it will improve runtime corruption dtection all that much more. i.e I feel like the additional checking falls on the wrong side of the cost/benefit line. Yeah, I know that line is kinda blurry.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
