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...
Maybe the "AGF_GOOD_VERSION" required the negation, and it all got lumped
together?
Thanks,
-Eric
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
