REVIEW: xfs_bmap_check_leaf_extents() can reference unmapped memory

REVIEW: xfs_bmap_check_leaf_extents() can reference unmapped memory

[Lachlan McIlroy]

While investigating the extent corruption bug I ran into this bug in
debug only code.  xfs_bmap_check_leaf_extents() loops through the leaf
blocks of the extent btree checking that every extent is entirely
before the next extent.  It also compares the last extent in the previous
block to the first extent in the current block when the previous block
has been released and potentially unmapped.  So take a copy of the last
extent instead of a pointer.  Also move the last extent check out of the
loop because we only need to do it once.

Lachlan


--- fs/xfs/xfs_bmap.c_1.386	2008-03-17 13:37:32.000000000 +1100
+++ fs/xfs/xfs_bmap.c	2008-03-19 14:55:41.000000000 +1100
@@ -6194,7 +6194,7 @@ xfs_bmap_check_leaf_extents(
  	xfs_mount_t		*mp;	/* file system mount structure */
  	__be64			*pp;	/* pointer to block address */
  	xfs_bmbt_rec_t		*ep;	/* pointer to current extent */
-	xfs_bmbt_rec_t		*lastp; /* pointer to previous extent */
+	xfs_bmbt_rec_t		last;   /* last extent in previous block */
  	xfs_bmbt_rec_t		*nextp;	/* pointer to next extent */
  	int			bp_release = 0;

@@ -6264,7 +6264,6 @@ xfs_bmap_check_leaf_extents(
  	/*
  	 * Loop over all leaf nodes checking that all extents are in the right order.
  	 */
-	lastp = NULL;
  	for (;;) {
  		xfs_fsblock_t	nextbno;
  		xfs_extnum_t	num_recs;
@@ -6285,18 +6284,18 @@ xfs_bmap_check_leaf_extents(
  		 */

  		ep = XFS_BTREE_REC_ADDR(xfs_bmbt, block, 1);
+		if (i) {
+			xfs_btree_check_rec(XFS_BTNUM_BMAP, (void *)&last,
+				(void *)ep);
+		}
  		for (j = 1; j < num_recs; j++) {
  			nextp = XFS_BTREE_REC_ADDR(xfs_bmbt, block, j + 1);
-			if (lastp) {
-				xfs_btree_check_rec(XFS_BTNUM_BMAP,
-					(void *)lastp, (void *)ep);
-			}
  			xfs_btree_check_rec(XFS_BTNUM_BMAP, (void *)ep,
  				(void *)(nextp));
-			lastp = ep;
  			ep = nextp;
  		}

+		last = *ep;
  		i += num_recs;
  		if (bp_release) {
  			bp_release = 0;

Re: REVIEW: xfs_bmap_check_leaf_extents() can reference unmapped memory

[Christoph Hellwig]

On Thu, Mar 20, 2008 at 05:11:23PM +1100, Lachlan McIlroy wrote:
> +	xfs_bmbt_rec_t		last;   /* last extent in previous block */
>  	xfs_bmbt_rec_t		*nextp;	/* pointer to next extent */
>  	int			bp_release = 0;
>
> @@ -6264,7 +6264,6 @@ xfs_bmap_check_leaf_extents(
>  	/*
>  	 * Loop over all leaf nodes checking that all extents are in the right order.
>  	 */
>  	for (;;) {
>  		xfs_fsblock_t	nextbno;
>  		xfs_extnum_t	num_recs;
> @@ -6285,18 +6284,18 @@ xfs_bmap_check_leaf_extents(
>  		 */
>
>  		ep = XFS_BTREE_REC_ADDR(xfs_bmbt, block, 1);
> +		if (i) {
> +			xfs_btree_check_rec(XFS_BTNUM_BMAP, (void *)&last,
> +				(void *)ep);

I haven't actually compiled this yet, but I'd expect this to give an
unitialized variable warning with gcc because it can't figure out this
can't happen in the first loop iteration.  You might need and
last = { 0, } somewhere in the beginning of the function.

Also I think the void * casts above are useless.

>  			xfs_btree_check_rec(XFS_BTNUM_BMAP, (void *)ep,
>  				(void *)(nextp));

and at that point you might fix these up aswell, with the added benefit
that now the whole call fits on a single line.