Skip to contentSkip to conversation

xfsprogs: metadump/restore fixes

PATCH In review Dave Chinner 3 participants 5 messages Dec 14, 2015 – Dec 15, 2015

lore
Share & tools
lore
#0 lore raw

cover xfsprogs: metadump/restore fixes

Hi folks,

A couple of patches from trying to process a rather large, badly broken filesystem from Arkadiusz Miskiewicz. The first patch prevents a crash when zeroing unused regions of corrupt btree blocks. The second patch makes mdrestore report progress correctly, which is kind of important when you've got a 35+GB metadump image being restored.

-Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw

PATCH 1/2 metadump: bounds check btree block regions being zeroed

From: Dave Chinner <[email protected]>

Arkadiusz Miskiewicz reported that metadump was crashing on one of his corrupted filesystems, and the trace indicated that it was zeroing unused regions in inode btree blocks when it failed. The btree block had a corrupt nrecs field, which was resulting in an out of bounds memset() occurring.

Ensure that the region being generated for zeroing is within bounds before executing the zeroing. While there, abstract the repeated boiler plate code so that it is simpler to maintain and extend the zeroing code to new types of btrees in future.

---
 db/metadump.c | 93 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 37 deletions(-)
db/metadump.c +56 -37
index 8cdcb92..cc5d1c7 100644
@@ -246,6 +246,11 @@ write_buf(
 	return seenint() ? -EINTR : 0;
 }
 
+/*
+ * We could be processing a corrupt block, so we can't trust any of
+ * the offsets or lengths to be within the buffer range. Hence check
+ * carefully!
+ */
 static void
 zero_btree_node(
 	struct xfs_btree_block	*block,
@@ -258,55 +263,64 @@ zero_btree_node(
 	xfs_inobt_key_t		*ikp;
 	xfs_alloc_ptr_t		*app;
 	xfs_alloc_key_t		*akp;
-	void			*zp1, *zp2;
-	int			zlen1, zlen2;
+	char			*zp1, *zp2;
+	char			*key_end;
 
 	nrecs = be16_to_cpu(block->bb_numrecs);
+	if (nrecs < 0)
+		return;
 
 	switch (btype) {
 	case TYP_BMAPBTA:
 	case TYP_BMAPBTD:
+		if (nrecs > mp->m_bmap_dmxr[1])
+			return;
+
 		bkp = XFS_BMBT_KEY_ADDR(mp, block, 1);
 		bpp = XFS_BMBT_PTR_ADDR(mp, block, 1, mp->m_bmap_dmxr[1]);
-		zp1 = &bkp[nrecs];
-		zlen1 = (char *)&bpp[0] - (char *)&bkp[nrecs];
-		zp2 = &bpp[nrecs];
-		zlen2 = (char *)block + mp->m_sb.sb_blocksize -
-							(char *)&bpp[nrecs];
+		zp1 = (char *)&bkp[nrecs];
+		zp2 = (char *)&bpp[nrecs];
+		key_end = (char *)bpp;
 		break;
 	case TYP_INOBT:
 	case TYP_FINOBT:
+		if (nrecs > mp->m_inobt_mxr[1])
+			return;
+
 		ikp = XFS_INOBT_KEY_ADDR(mp, block, 1);
 		ipp = XFS_INOBT_PTR_ADDR(mp, block, 1, mp->m_inobt_mxr[1]);
-		zp1 = &ikp[nrecs];
-		zlen1 = (char *)&ipp[0] - (char *)&ikp[nrecs];
-		zp2 = &ipp[nrecs];
-		zlen2 = (char *)block + mp->m_sb.sb_blocksize -
-							(char *)&ipp[nrecs];
+		zp1 = (char *)&ikp[nrecs];
+		zp2 = (char *)&ipp[nrecs];
+		key_end = (char *)ipp;
 		break;
 	case TYP_BNOBT:
 	case TYP_CNTBT:
+		if (nrecs > mp->m_alloc_mxr[1])
+			return;
+
 		akp = XFS_ALLOC_KEY_ADDR(mp, block, 1);
 		app = XFS_ALLOC_PTR_ADDR(mp, block, 1, mp->m_alloc_mxr[1]);
-		zp1 = &akp[nrecs];
-		zlen1 = (char *)&app[0] - (char *)&akp[nrecs];
-		zp2 = &app[nrecs];
-		zlen2 = (char *)block + mp->m_sb.sb_blocksize -
-							(char *)&app[nrecs];
+		zp1 = (char *)&akp[nrecs];
+		zp2 = (char *)&app[nrecs];
+		key_end = (char *)app;
 		break;
 	default:
-		zp1 = NULL;
-		break;
+		return;
 	}
 
-	if (zp1 && zp2) {
-		/* Zero from end of keys to beginning of pointers */
-		memset(zp1, 0, zlen1);
-		/* Zero from end of pointers to end of block */
-		memset(zp2, 0, zlen2);
-	}
+
+	/* Zero from end of keys to beginning of pointers */
+	memset(zp1, 0, key_end - zp1);
+
+	/* Zero from end of pointers to end of block */
+	memset(zp2, 0, (char *)block + mp->m_sb.sb_blocksize - zp2);
 }
 
+/*
+ * We could be processing a corrupt block, so we can't trust any of
+ * the offsets or lengths to be within the buffer range. Hence check
+ * carefully!
+ */
 static void
 zero_btree_leaf(
 	struct xfs_btree_block	*block,
@@ -316,38 +330,43 @@ zero_btree_leaf(
 	struct xfs_bmbt_rec	*brp;
 	struct xfs_inobt_rec	*irp;
 	struct xfs_alloc_rec	*arp;
-	void			*zp;
-	int			zlen;
+	char			*zp;
 
 	nrecs = be16_to_cpu(block->bb_numrecs);
+	if (nrecs < 0)
+		return;
 
 	switch (btype) {
 	case TYP_BMAPBTA:
 	case TYP_BMAPBTD:
+		if (nrecs > mp->m_bmap_dmxr[1])
+			return;
+
 		brp = XFS_BMBT_REC_ADDR(mp, block, 1);
-		zp = &brp[nrecs];
-		zlen = (char *)block + mp->m_sb.sb_blocksize - (char *)&brp[nrecs];
+		zp = (char *)&brp[nrecs];
 		break;
 	case TYP_INOBT:
 	case TYP_FINOBT:
+		if (nrecs > mp->m_inobt_mxr[1])
+			return;
+
 		irp = XFS_INOBT_REC_ADDR(mp, block, 1);
-		zp = &irp[nrecs];
-		zlen = (char *)block + mp->m_sb.sb_blocksize - (char *)&irp[nrecs];
+		zp = (char *)&irp[nrecs];
 		break;
 	case TYP_BNOBT:
 	case TYP_CNTBT:
+		if (nrecs > mp->m_alloc_mxr[1])
+			return;
+
 		arp = XFS_ALLOC_REC_ADDR(mp, block, 1);
-		zp = &arp[nrecs];
-		zlen = (char *)block + mp->m_sb.sb_blocksize - (char *)&arp[nrecs];
+		zp = (char *)&arp[nrecs];
 		break;
 	default:
-		zp = NULL;
-		break;
+		return;
 	}
 
 	/* Zero from end of records to end of block */
-	if (zp && zlen < mp->m_sb.sb_blocksize)
-		memset(zp, 0, zlen);
+	memset(zp, 0, (char *)block + mp->m_sb.sb_blocksize - zp);
 }
 
 static void
Signature · 6 lines
2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#2 lore raw

Re: [PATCH 1/2] metadump: bounds check btree block regions being zeroed

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Arkadiusz Miskiewicz reported that metadump was crashing on one of his corrupted filesystems, and the trace indicated that it was zeroing unused regions in inode btree blocks when it failed. The btree block had a corrupt nrecs field, which was resulting in an out of bounds memset() occurring.

Doh.

Time to run fuzzers+valgrind w/ metadump I guess... /me looks at djwong ...

Quoted 4 lines

Ensure that the region being generated for zeroing is within bounds before executing the zeroing. While there, abstract the repeated boiler plate code so that it is simpler to maintain and extend the zeroing code to new types of btrees in future.

yeah, thanks, not sure why I didn't do that originally. Much better.

Might be nice to split the bugfix & the cleanup into 2 patches, though...

Thanks,
-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#3 lore raw

Re: [PATCH 1/2] metadump: bounds check btree block regions being zeroed

In reply to Dave Chinner
Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Arkadiusz Miskiewicz reported that metadump was crashing on one of his corrupted filesystems, and the trace indicated that it was zeroing unused regions in inode btree blocks when it failed. The btree block had a corrupt nrecs field, which was resulting in an out of bounds memset() occurring.

Quoted 6 lines

Ensure that the region being generated for zeroing is within bounds before executing the zeroing. While there, abstract the repeated boiler plate code so that it is simpler to maintain and extend the zeroing code to new types of btrees in future.

... and Tested-by: Arkadiusz Miskiewicz <[email protected]>

By that I mean that it did dump my broken fs without segfaulting (using -g option only).

Signature · 6 lines
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#4 lore raw

PATCH 2/2 xfs_mdrestore: correctly account bytes read

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

Progess indication comes in the form of a "X MB read" output. This doesn't match up with the actual number of bytes read from the metadump file because it only accounts header blocks in the file, not actual metadata blocks that are restored, Hence the number reported is usually much lower than the size of the metadump file, hence it's impossible to use to guage progress of the restore.

While there, fix the progress output so that it overwrites the previous progress output line correctly.

---
 mdrestore/xfs_mdrestore.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mdrestore/xfs_mdrestore.c +2 -2
index 5764616..0c0e08a 100644
@@ -133,7 +133,7 @@ perform_restore(
 
 	for (;;) {
 		if (show_progress && (bytes_read & ((1 << 20) - 1)) == 0)
-			print_progress("%lld MB read\n", bytes_read >> 20);
+			print_progress("%lld MB read", bytes_read >> 20);
 
 		for (cur_index = 0; cur_index < mb_count; cur_index++) {
 			if (pwrite64(dst_fd, &block_buffer[cur_index <<
@@ -160,7 +160,7 @@ perform_restore(
 								1, src_f) != 1)
 			fatal("error reading from file: %s\n", strerror(errno));
 
-		bytes_read += block_size;
+		bytes_read += block_size + (mb_count << tmb.mb_blocklog);
 	}
 
 	if (progress_since_warning)
Signature · 6 lines
2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs