Skip to contentSkip to conversation

metadump/restore: fix up zeroing and accounting

PATCH Superseded Dave Chinner 3 participants 8 messages Feb 4, 2016 – Feb 8, 2016

lore
Share & tools
lore

Dave Chinner

author
#0 lore raw

v2 cover metadump/restore: fix up zeroing and accounting

Hi folks,

These are my pending metadump fixes - the second patch in the series still needs review - the only changes there are to fix up the leaf/node max record count indexes that Brian noticed were wrong.

First posted here:

http://oss.sgi.com/archives/xfs/2015-12/msg00557.html

-Dave.

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

author
#1 lore raw

PATCH v2 1/3 metadump: clean up btree block region zeroing

From: Dave Chinner <[email protected]>

Abstract out all the common operations in the btree block zeroing so that we only have one copy of offset/len calculations and don't require lots of casts for the pointer arithmetic.

Signed-off-by Dave Chinner <[email protected]>
Tested-by Arkadiusz Miskiewicz <[email protected]>
Reviewed-by Brian Foster <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
---
 db/metadump.c | 61 +++++++++++++++++++++++------------------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)
db/metadump.c +24 -37
index 8455fdd..a185da5 100644
@@ -258,8 +258,8 @@ 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);
 

@@ -268,43 +268,36 @@ zero_btree_node(
 	case TYP_BMAPBTD:
 		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:
 		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:
 		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);
 }
 
 static void

@@ -316,8 +309,7 @@ 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);
 

@@ -325,29 +317,24 @@ zero_btree_leaf(
 	case TYP_BMAPBTA:
 	case TYP_BMAPBTD:
 		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:
 		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:
 		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
author
#3 lore raw

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

In reply to 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. Ensure that the region being generated for zeroing is within bounds before executing the zeroing.

Reported-by Arkadiusz Miskiewicz <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
---
 db/metadump.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
db/metadump.c +32 -0
index a185da5..26a3bd5 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,

@@ -262,10 +267,15 @@ zero_btree_node(
 	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 = (char *)&bkp[nrecs];

@@ -274,6 +284,9 @@ zero_btree_node(
 		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 = (char *)&ikp[nrecs];

@@ -282,6 +295,9 @@ zero_btree_node(
 		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 = (char *)&akp[nrecs];

@@ -300,6 +316,11 @@ zero_btree_node(
 	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,

@@ -312,20 +333,31 @@ zero_btree_leaf(
 	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[0])
+			return;
+
 		brp = XFS_BMBT_REC_ADDR(mp, block, 1);
 		zp = (char *)&brp[nrecs];
 		break;
 	case TYP_INOBT:
 	case TYP_FINOBT:
+		if (nrecs > mp->m_inobt_mxr[0])
+			return;
+
 		irp = XFS_INOBT_REC_ADDR(mp, block, 1);
 		zp = (char *)&irp[nrecs];
 		break;
 	case TYP_BNOBT:
 	case TYP_CNTBT:
+		if (nrecs > mp->m_alloc_mxr[0])
+			return;
+
 		arp = XFS_ALLOC_REC_ADDR(mp, block, 1);
 		zp = (char *)&arp[nrecs];
 		break;
Signature · 6 lines
2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v2 2/3
#4 lore raw

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

Quoted 9 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. Ensure that the region being generated for zeroing is within bounds before executing the zeroing.

Quoted 3 lines
Reported-by Arkadiusz Miskiewicz <[email protected]>
Signed-off-by Dave Chinner <[email protected]>

---

Reviewed-by Brian Foster <[email protected]>
Quoted 106 lines
 db/metadump.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
db/metadump.c +32 -0
index a185da5..26a3bd5 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,

@@ -262,10 +267,15 @@ zero_btree_node(
 	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 = (char *)&bkp[nrecs];

@@ -274,6 +284,9 @@ zero_btree_node(
 		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 = (char *)&ikp[nrecs];

@@ -282,6 +295,9 @@ zero_btree_node(
 		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 = (char *)&akp[nrecs];

@@ -300,6 +316,11 @@ zero_btree_node(
 	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,

@@ -312,20 +333,31 @@ zero_btree_leaf(
 	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[0])
+			return;
+
 		brp = XFS_BMBT_REC_ADDR(mp, block, 1);
 		zp = (char *)&brp[nrecs];
 		break;
 	case TYP_INOBT:
 	case TYP_FINOBT:
+		if (nrecs > mp->m_inobt_mxr[0])
+			return;
+
 		irp = XFS_INOBT_REC_ADDR(mp, block, 1);
 		zp = (char *)&irp[nrecs];
 		break;
 	case TYP_BNOBT:
 	case TYP_CNTBT:
+		if (nrecs > mp->m_alloc_mxr[0])
+			return;
+
 		arp = XFS_ALLOC_REC_ADDR(mp, block, 1);
 		zp = (char *)&arp[nrecs];
 		break;
Signature · 6 lines
2.5.0

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

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

author
#6 lore raw

PATCH v2 3/3 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 gauge progress of the restore.

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

Signed-off-by Dave Chinner <[email protected]>
Reviewed-by Brian Foster <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
---
 mdrestore/xfs_mdrestore.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mdrestore/xfs_mdrestore.c +2 -2
index ebc5e54..70a160c 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