Re: [PATCH 04/32] xfs: check magic numbers in dir3 leaf verifier first

Eric Sandeen
2013-10-08T23:03:49Z

On 9/29/13 10:15 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Calling xfs_dir3_leaf_hdr_from_disk() in a verifier before
> validating the magic numbers in the buffer results in ASSERT
> failures due to mismatching magic numbers when a corruption occurs.
> Seeing as the verifier is supposed to catch the corruption and pass
> it back to the caller, having the verifier assert fail on error
> defeats the purpose of detecting the errors in the first place.
>
> Check the magic numbers direct from the buffer before decoding the
> header.

Looks good; have you sent this for the kernel yet?

(I thought we wanted changes to hit kernelspace first) :)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> libxfs/xfs_dir2_leaf.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/libxfs/xfs_dir2_leaf.c b/libxfs/xfs_dir2_leaf.c
> index 7ec2f19..c035c4d 100644
> --- a/libxfs/xfs_dir2_leaf.c
> +++ b/libxfs/xfs_dir2_leaf.c
> @@ -161,6 +161,11 @@ xfs_dir3_leaf_check_int(
> return true;
> }
>
> +/*
> + * We verify the magic numbers before decoding the leaf header so that on debug
> + * kernels we don't get assertion failures in xfs_dir3_leaf_hdr_from_disk() due
> + * to incorrect magic numbers.
> + */
> static bool
> xfs_dir3_leaf_verify(
> struct xfs_buf *bp,
> @@ -172,24 +177,25 @@ xfs_dir3_leaf_verify(
>
> ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
>
> - xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> + __uint16_t magic3;
>
> - if ((magic == XFS_DIR2_LEAF1_MAGIC &&
> - leafhdr.magic != XFS_DIR3_LEAF1_MAGIC) ||
> - (magic == XFS_DIR2_LEAFN_MAGIC &&
> - leafhdr.magic != XFS_DIR3_LEAFN_MAGIC))
> - return false;
> + magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> + : XFS_DIR3_LEAFN_MAGIC;
>
> + if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> + return false;
> if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_uuid))
> return false;
> if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> return false;
> } else {
> - if (leafhdr.magic != magic)
> + if (leaf->hdr.info.magic != cpu_to_be16(magic))
> return false;
> }
> +
> + xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
> return xfs_dir3_leaf_check_int(mp, &leafhdr, leaf);
> }
>
>