Skip to contentSkip to conversation

REVIEW: xfs_bmap_check_leaf_extents() can reference unmapped memory

PATCH Lachlan McIlroy 2 participants 2 messages Mar 20, 2008

lore
Share & tools
lore

Lachlan McIlroy

author
#0 lore raw

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

xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.386 +6 -7
@@ -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;
#1 lore raw
Quoted 12 lines · Lachlan McIlroy
xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.386 +1 -0
+	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;

xfs/xfs_bmap.c xfs_bmap_check_leaf_extents( · line 6289

Quoted 6 lines
xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.386 +3 -0
@@ -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/xfs_bmap.c xfs_bmap_check_leaf_extents( · line 6294

Quoted 2 lines
xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.386 +0 -0
 			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.