On Tue, Aug 19, 2014 at 02:07:39PM -0500, Eric Sandeen wrote: > On 8/19/14, 1:15 PM, Christoph Hellwig wrote: > >> Anyway - bounds checking when we read from disk is a good thing! > > > > Absolutelt! > > > > Looks good modulo a few nitpicks below. > > > > Reviewed-by: Christoph Hellwig <hch@xxxxxx> > > > >> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > >> index 4bffffe..a4a9e0e 100644 > >> --- a/fs/xfs/libxfs/xfs_alloc.c > >> +++ b/fs/xfs/libxfs/xfs_alloc.c > >> @@ -2209,6 +2209,10 @@ xfs_agf_verify( > >> be32_to_cpu(agf->agf_flcount) <= XFS_AGFL_SIZE(mp))) > >> return false; > >> > >> + if (!(be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) <= XFS_BTREE_MAXLEVELS && > >> + be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]) <= XFS_BTREE_MAXLEVELS)) > >> + return false; > > > > Maybe it's just me, but negated numeric comparisms always confuse the > > hell out of me, why not simply: > > > > if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) > XFS_BTREE_MAXLEVELS) > > return false; > > if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]) > XFS_BTREE_MAXLEVELS) > > return false; > > > >> --- a/fs/xfs/libxfs/xfs_ialloc.c > >> +++ b/fs/xfs/libxfs/xfs_ialloc.c > >> @@ -2051,6 +2051,8 @@ xfs_agi_verify( > >> if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum))) > >> return false; > >> > >> + if (!(be32_to_cpu(agi->agi_level) <= XFS_BTREE_MAXLEVELS)) > >> + return false; > > > > Same here. > > yeah; just following the style of the functions as they exist today... > > if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) && > XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) && > be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) && > ... > > dunno. Don't care too much either way, but consistency and all that... I prefer the metho Christoph suggested - most of the verifiers use that "single check per if statement" pattern because it makes the checks being performed so much easier to read. > Maybe the "AGF_GOOD_VERSION" required the negation, and it all got lumped > together? Those should probably be cleaned up - they were done like that originally as a direct transcript from pre-existing code checks to simplify review, not because it was "nice" code. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
