xfsprogs: metadump/restore fixes
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
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
@@ -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
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
Re: [PATCH 1/2] metadump: bounds check btree block regions being zeroed
In reply to Dave ChinnerQuoted 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.
Reported-by Arkadiusz Miskiewicz <[email protected]>
... 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
PATCH 2/2 xfs_mdrestore: correctly account bytes read
In reply to Dave ChinnerFrom: 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
@@ -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