xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: add a few more verifier tests
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 19 Aug 2014 11:15:42 -0700
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <53F2C103.8030607@xxxxxxxxxx>
References: <53F2C103.8030607@xxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
> 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.

<Prev in Thread] Current Thread [Next in Thread>