* [PATCH v3 0/2] Try to squash metadump data leaks @ 2018-10-11 19:44 Stefan Ring 2018-10-11 19:44 ` [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks Stefan Ring ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Stefan Ring @ 2018-10-11 19:44 UTC (permalink / raw) To: linux-xfs This is the same as v2, but with the correct e-mail address this time. Sorry! Stefan Ring (2): xfs_metadump: Extend zapping to multi fsb dir blocks xfs_metadump: Zap more stale data db/metadump.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 107 insertions(+), 14 deletions(-) -- 2.14.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks 2018-10-11 19:44 [PATCH v3 0/2] Try to squash metadump data leaks Stefan Ring @ 2018-10-11 19:44 ` Stefan Ring 2018-10-23 15:10 ` Eric Sandeen 2018-10-11 19:44 ` [PATCH 2/2] xfs_metadump: Zap more stale data Stefan Ring 2018-10-25 15:50 ` [PATCH v3 0/2] Try to squash metadump data leaks Eric Sandeen 2 siblings, 1 reply; 16+ messages in thread From: Stefan Ring @ 2018-10-11 19:44 UTC (permalink / raw) To: linux-xfs The processing for data zeroing was never added to process_multi_fsb_objects. It is now the same thing that process_single_fsb_objects does. --- db/metadump.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/db/metadump.c b/db/metadump.c index cc2ae9af..ff96860d 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -1862,6 +1862,7 @@ process_multi_fsb_objects( typnm_t btype, xfs_fileoff_t last) { + char *dp; int ret = 0; switch (btype) { @@ -1902,14 +1903,16 @@ process_multi_fsb_objects( } - if ((!obfuscate && !zero_stale_data) || - o >= mp->m_dir_geo->leafblk) { - ret = write_buf(iocur_top); - goto out_pop; + dp = iocur_top->data; + if (o >= mp->m_dir_geo->freeblk) { + process_dir_free_block(dp); + } else if (o >= mp->m_dir_geo->leafblk) { + process_dir_leaf_block(dp); + } else { + process_dir_data_block( + dp, o, last == mp->m_dir_geo->fsbcount); } - process_dir_data_block(iocur_top->data, o, - last == mp->m_dir_geo->fsbcount); iocur_top->need_crc = 1; ret = write_buf(iocur_top); out_pop: -- 2.14.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks 2018-10-11 19:44 ` [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks Stefan Ring @ 2018-10-23 15:10 ` Eric Sandeen 2018-10-25 15:08 ` Eric Sandeen 0 siblings, 1 reply; 16+ messages in thread From: Eric Sandeen @ 2018-10-23 15:10 UTC (permalink / raw) To: Stefan Ring, linux-xfs On 10/11/18 2:44 PM, Stefan Ring wrote: > The processing for data zeroing was never added to process_multi_fsb_objects. > It is now the same thing that process_single_fsb_objects does. First, thanks for doing this, seems about right. But I could use more changelog words here. ;) AFAICT, the intent was for process_multi_fsb_objects to call process_dir_data_block() in order to handle the zeroing for multi-fsb objects, so at least some of the cases /were/ handled, right? Your patch seems to be splitting that 3 ways, so we go to process_dir_free_block or process_dir_leaf_block or process_dir_data_block, the first two are new cases that are now handled? (I do see that this is the same as the process_single_fsb_objects code.) Given the old case: if ((!obfuscate && !zero_stale_data) || o >= mp->m_dir_geo->leafblk) { ret = write_buf(iocur_top); it looks like we were just directly writing the leaf blocks and never obfuscating them, is that correct? I guess I need to go make some test filesystems... do you know from your testing if this is true? This seems quite reasonable, I just think it might be doing more than the changelog says it is...? -Eric > --- > db/metadump.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/db/metadump.c b/db/metadump.c > index cc2ae9af..ff96860d 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -1862,6 +1862,7 @@ process_multi_fsb_objects( > typnm_t btype, > xfs_fileoff_t last) > { > + char *dp; > int ret = 0; > > switch (btype) { > @@ -1902,14 +1903,16 @@ process_multi_fsb_objects( > > } > > - if ((!obfuscate && !zero_stale_data) || > - o >= mp->m_dir_geo->leafblk) { > - ret = write_buf(iocur_top); > - goto out_pop; > + dp = iocur_top->data; > + if (o >= mp->m_dir_geo->freeblk) { > + process_dir_free_block(dp); > + } else if (o >= mp->m_dir_geo->leafblk) { > + process_dir_leaf_block(dp); > + } else { > + process_dir_data_block( > + dp, o, last == mp->m_dir_geo->fsbcount); > } > > - process_dir_data_block(iocur_top->data, o, > - last == mp->m_dir_geo->fsbcount); > iocur_top->need_crc = 1; > ret = write_buf(iocur_top); > out_pop: > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks 2018-10-23 15:10 ` Eric Sandeen @ 2018-10-25 15:08 ` Eric Sandeen 2018-10-25 16:44 ` Stefan Ring 0 siblings, 1 reply; 16+ messages in thread From: Eric Sandeen @ 2018-10-25 15:08 UTC (permalink / raw) To: Stefan Ring, linux-xfs On 10/23/18 10:10 AM, Eric Sandeen wrote: > On 10/11/18 2:44 PM, Stefan Ring wrote: >> The processing for data zeroing was never added to process_multi_fsb_objects. >> It is now the same thing that process_single_fsb_objects does. > > First, thanks for doing this, seems about right. > > But I could use more changelog words here. ;) > > AFAICT, the intent was for process_multi_fsb_objects to call > process_dir_data_block() in order to handle the zeroing for multi-fsb > objects, so at least some of the cases /were/ handled, right? > > Your patch seems to be splitting that 3 ways, so we go to > process_dir_free_block or process_dir_leaf_block or process_dir_data_block, > the first two are new cases that are now handled? (I do see that this is > the same as the process_single_fsb_objects code.) > > Given the old case: > > if ((!obfuscate && !zero_stale_data) || > o >= mp->m_dir_geo->leafblk) { > ret = write_buf(iocur_top); > > it looks like we were just directly writing the leaf blocks and > never obfuscating them, is that correct? I guess I need to go make > some test filesystems... do you know from your testing if this is true? Whoops, I forgot what directory leaf blocks were, sorry - there is nothing to obfuscate in them. (but there may be data to zero in them...) -Eric ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks 2018-10-25 15:08 ` Eric Sandeen @ 2018-10-25 16:44 ` Stefan Ring 0 siblings, 0 replies; 16+ messages in thread From: Stefan Ring @ 2018-10-25 16:44 UTC (permalink / raw) To: linux-xfs On Thu, Oct 25, 2018 at 5:08 PM Eric Sandeen <sandeen@sandeen.net> wrote: > On 10/23/18 10:10 AM, Eric Sandeen wrote: > > On 10/11/18 2:44 PM, Stefan Ring wrote: > >> The processing for data zeroing was never added to process_multi_fsb_objects. > >> It is now the same thing that process_single_fsb_objects does. > > > > First, thanks for doing this, seems about right. > > > > But I could use more changelog words here. ;) > > > > AFAICT, the intent was for process_multi_fsb_objects to call > > process_dir_data_block() in order to handle the zeroing for multi-fsb > > objects, so at least some of the cases /were/ handled, right? > > > > Your patch seems to be splitting that 3 ways, so we go to > > process_dir_free_block or process_dir_leaf_block or process_dir_data_block, > > the first two are new cases that are now handled? (I do see that this is > > the same as the process_single_fsb_objects code.) > > > > Given the old case: > > > > if ((!obfuscate && !zero_stale_data) || > > o >= mp->m_dir_geo->leafblk) { > > ret = write_buf(iocur_top); > > > > it looks like we were just directly writing the leaf blocks and > > never obfuscating them, is that correct? I guess I need to go make > > some test filesystems... do you know from your testing if this is true? > > Whoops, I forgot what directory leaf blocks were, sorry - there is nothing > to obfuscate in them. (but there may be data to zero in them...) I really could not make sense of your previous response, but I'll look over it once again anyway. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] xfs_metadump: Zap more stale data 2018-10-11 19:44 [PATCH v3 0/2] Try to squash metadump data leaks Stefan Ring 2018-10-11 19:44 ` [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks Stefan Ring @ 2018-10-11 19:44 ` Stefan Ring 2018-10-23 15:49 ` Darrick J. Wong 2018-10-25 15:50 ` [PATCH v3 0/2] Try to squash metadump data leaks Eric Sandeen 2 siblings, 1 reply; 16+ messages in thread From: Stefan Ring @ 2018-10-11 19:44 UTC (permalink / raw) To: linux-xfs I have empirically found and tried to fix some places where stale data was not properly zeroed out. In the order of the code changes: - The "freeindex" blocks in inode directories, from last entry to end of block. - XFS_DIR{2,3}_LEAFN_MAGIC, from last entry to end of block. - In btree format inodes before as well as after the btree pointers. - In dev inodes, everything after the header. --- db/metadump.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 98 insertions(+), 8 deletions(-) diff --git a/db/metadump.c b/db/metadump.c index ff96860d..c8213d41 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -1421,12 +1421,49 @@ process_sf_attr( memset(asfep, 0, XFS_DFORK_ASIZE(dip, mp) - ino_attr_size); } +static void +process_dir_free_block( + char *block) +{ + struct xfs_dir2_free *free; + struct xfs_dir3_icfree_hdr freehdr; + + if (!zero_stale_data) + return; + + free = (struct xfs_dir2_free *)block; + M_DIROPS(mp)->free_hdr_from_disk(&freehdr, free); + + switch (freehdr.magic) { + case XFS_DIR2_FREE_MAGIC: + case XFS_DIR3_FREE_MAGIC: { + __be16 *bests; + char *high; + int used; + + /* Zero out space from end of bests[] to end of block */ + bests = M_DIROPS(mp)->free_bests_p(free); + high = (char *)&bests[freehdr.nvalid]; + used = high - (char*)free; + memset(high, 0, mp->m_dir_geo->blksize - used); + iocur_top->need_crc = 1; + break; + } + default: + if (show_warnings) + print_warning("invalid magic in dir inode %llu " + "free block", + (unsigned long long)cur_ino); + break; + } +} + static void process_dir_leaf_block( char *block) { struct xfs_dir2_leaf *leaf; - struct xfs_dir3_icleaf_hdr leafhdr; + struct xfs_dir3_icleaf_hdr leafhdr; if (!zero_stale_data) return; @@ -1435,20 +1472,39 @@ process_dir_leaf_block( leaf = (struct xfs_dir2_leaf *)block; M_DIROPS(mp)->leaf_hdr_from_disk(&leafhdr, leaf); - /* Zero out space from end of ents[] to bests */ - if (leafhdr.magic == XFS_DIR2_LEAF1_MAGIC || - leafhdr.magic == XFS_DIR3_LEAF1_MAGIC) { + switch (leafhdr.magic) { + case XFS_DIR2_LEAF1_MAGIC: + case XFS_DIR3_LEAF1_MAGIC: { struct xfs_dir2_leaf_tail *ltp; __be16 *lbp; struct xfs_dir2_leaf_entry *ents; char *free; /* end of ents */ + /* Zero out space from end of ents[] to bests */ ents = M_DIROPS(mp)->leaf_ents_p(leaf); free = (char *)&ents[leafhdr.count]; ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); lbp = xfs_dir2_leaf_bests_p(ltp); memset(free, 0, (char *)lbp - free); iocur_top->need_crc = 1; + break; + } + case XFS_DIR2_LEAFN_MAGIC: + case XFS_DIR3_LEAFN_MAGIC: { + struct xfs_dir2_leaf_entry *ents; + char *free; + int used; + + /* Zero out space from end of ents[] to end of block */ + ents = M_DIROPS(mp)->leaf_ents_p(leaf); + free = (char *)&ents[leafhdr.count]; + used = free - (char*)leaf; + memset(free, 0, mp->m_dir_geo->blksize - used); + iocur_top->need_crc = 1; + break; + } + default: + break; } } @@ -1499,7 +1555,7 @@ process_dir_data_block( if (show_warnings) print_warning( "invalid magic in dir inode %llu block %ld", - (long long)cur_ino, (long)offset); + (unsigned long long)cur_ino, (long)offset); return; } @@ -1813,8 +1869,7 @@ process_single_fsb_objects( switch (btype) { case TYP_DIR2: if (o >= mp->m_dir_geo->freeblk) { - /* TODO, zap any stale data */ - break; + process_dir_free_block(dp); } else if (o >= mp->m_dir_geo->leafblk) { process_dir_leaf_block(dp); } else { @@ -2118,6 +2173,21 @@ process_btinode( } pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs); + + if (zero_stale_data) { + char *top; + int used; + + /* Space before btree pointers */ + top = (char*)XFS_BMDR_PTR_ADDR(dib, 1, nrecs); + memset(top, 0, (char*)pp - top); + + /* Space after btree pointers */ + top = (char*)&pp[nrecs]; + used = top - (char*)dip; + memset(top, 0, mp->m_sb.sb_inodesize - used); + } + for (i = 0; i < nrecs; i++) { xfs_agnumber_t ag; xfs_agblock_t bno; @@ -2201,6 +2271,24 @@ process_inode_data( return 1; } +static int +process_dev_inode( + xfs_dinode_t *dip) +{ + if (XFS_DFORK_NEXTENTS(dip, XFS_ATTR_FORK) || + XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) { + if (show_warnings) + print_warning("inode %llu has unexpected extents", + (unsigned long long)cur_ino); + return 0; + } else { + int used = XFS_DFORK_DPTR(dip) - (char*)dip; + + memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - used); + return 1; + } +} + /* * when we process the inode, we may change the data in the data and/or * attribute fork if they are in short form and we are obfuscating names. @@ -2253,7 +2341,9 @@ process_inode( case S_IFREG: success = process_inode_data(dip, TYP_DATA); break; - default: ; + default: + success = process_dev_inode(dip); + break; } nametable_clear(); -- 2.14.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] xfs_metadump: Zap more stale data 2018-10-11 19:44 ` [PATCH 2/2] xfs_metadump: Zap more stale data Stefan Ring @ 2018-10-23 15:49 ` Darrick J. Wong 2018-10-25 16:42 ` Stefan Ring 2018-10-26 5:32 ` Stefan Ring 0 siblings, 2 replies; 16+ messages in thread From: Darrick J. Wong @ 2018-10-23 15:49 UTC (permalink / raw) To: Stefan Ring; +Cc: linux-xfs On Thu, Oct 11, 2018 at 09:44:24PM +0200, Stefan Ring wrote: > I have empirically found and tried to fix some places where stale data was not > properly zeroed out. > > In the order of the code changes: > > - The "freeindex" blocks in inode directories, from last entry to end of block. > - XFS_DIR{2,3}_LEAFN_MAGIC, from last entry to end of block. > - In btree format inodes before as well as after the btree pointers. > - In dev inodes, everything after the header. > --- > db/metadump.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 98 insertions(+), 8 deletions(-) > > diff --git a/db/metadump.c b/db/metadump.c > index ff96860d..c8213d41 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -1421,12 +1421,49 @@ process_sf_attr( > memset(asfep, 0, XFS_DFORK_ASIZE(dip, mp) - ino_attr_size); > } > > +static void > +process_dir_free_block( I think patch 1 references this function in patch 2? > + char *block) > +{ > + struct xfs_dir2_free *free; > + struct xfs_dir3_icfree_hdr freehdr; > + > + if (!zero_stale_data) > + return; > + > + free = (struct xfs_dir2_free *)block; > + M_DIROPS(mp)->free_hdr_from_disk(&freehdr, free); > + > + switch (freehdr.magic) { > + case XFS_DIR2_FREE_MAGIC: > + case XFS_DIR3_FREE_MAGIC: { > + __be16 *bests; > + char *high; > + int used; > + > + /* Zero out space from end of bests[] to end of block */ > + bests = M_DIROPS(mp)->free_bests_p(free); > + high = (char *)&bests[freehdr.nvalid]; > + used = high - (char*)free; > + memset(high, 0, mp->m_dir_geo->blksize - used); > + iocur_top->need_crc = 1; > + break; > + } > + default: > + if (show_warnings) > + print_warning("invalid magic in dir inode %llu " > + "free block", > + (unsigned long long)cur_ino); > + break; > + } > +} > + > static void > process_dir_leaf_block( > char *block) > { > struct xfs_dir2_leaf *leaf; > - struct xfs_dir3_icleaf_hdr leafhdr; > + struct xfs_dir3_icleaf_hdr leafhdr; > > if (!zero_stale_data) > return; > @@ -1435,20 +1472,39 @@ process_dir_leaf_block( > leaf = (struct xfs_dir2_leaf *)block; > M_DIROPS(mp)->leaf_hdr_from_disk(&leafhdr, leaf); > > - /* Zero out space from end of ents[] to bests */ > - if (leafhdr.magic == XFS_DIR2_LEAF1_MAGIC || > - leafhdr.magic == XFS_DIR3_LEAF1_MAGIC) { > + switch (leafhdr.magic) { > + case XFS_DIR2_LEAF1_MAGIC: > + case XFS_DIR3_LEAF1_MAGIC: { > struct xfs_dir2_leaf_tail *ltp; > __be16 *lbp; > struct xfs_dir2_leaf_entry *ents; > char *free; /* end of ents */ > > + /* Zero out space from end of ents[] to bests */ > ents = M_DIROPS(mp)->leaf_ents_p(leaf); > free = (char *)&ents[leafhdr.count]; > ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); > lbp = xfs_dir2_leaf_bests_p(ltp); > memset(free, 0, (char *)lbp - free); > iocur_top->need_crc = 1; > + break; > + } > + case XFS_DIR2_LEAFN_MAGIC: > + case XFS_DIR3_LEAFN_MAGIC: { > + struct xfs_dir2_leaf_entry *ents; > + char *free; > + int used; > + > + /* Zero out space from end of ents[] to end of block */ > + ents = M_DIROPS(mp)->leaf_ents_p(leaf); > + free = (char *)&ents[leafhdr.count]; > + used = free - (char*)leaf; > + memset(free, 0, mp->m_dir_geo->blksize - used); > + iocur_top->need_crc = 1; > + break; > + } > + default: > + break; > } > } > > @@ -1499,7 +1555,7 @@ process_dir_data_block( > if (show_warnings) > print_warning( > "invalid magic in dir inode %llu block %ld", > - (long long)cur_ino, (long)offset); > + (unsigned long long)cur_ino, (long)offset); > return; > } > > @@ -1813,8 +1869,7 @@ process_single_fsb_objects( > switch (btype) { > case TYP_DIR2: > if (o >= mp->m_dir_geo->freeblk) { > - /* TODO, zap any stale data */ > - break; > + process_dir_free_block(dp); > } else if (o >= mp->m_dir_geo->leafblk) { > process_dir_leaf_block(dp); > } else { > @@ -2118,6 +2173,21 @@ process_btinode( > } > > pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs); > + > + if (zero_stale_data) { > + char *top; > + int used; > + > + /* Space before btree pointers */ > + top = (char*)XFS_BMDR_PTR_ADDR(dib, 1, nrecs); > + memset(top, 0, (char*)pp - top); > + > + /* Space after btree pointers */ > + top = (char*)&pp[nrecs]; > + used = top - (char*)dip; > + memset(top, 0, mp->m_sb.sb_inodesize - used); Er... this zaps everything past the end of the bmbt pointers, which means that this can zero the attr fork root. > + } > + > for (i = 0; i < nrecs; i++) { > xfs_agnumber_t ag; > xfs_agblock_t bno; > @@ -2201,6 +2271,24 @@ process_inode_data( > return 1; > } > > +static int > +process_dev_inode( > + xfs_dinode_t *dip) > +{ > + if (XFS_DFORK_NEXTENTS(dip, XFS_ATTR_FORK) || > + XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) { > + if (show_warnings) > + print_warning("inode %llu has unexpected extents", > + (unsigned long long)cur_ino); > + return 0; > + } else { No need for else here. > + int used = XFS_DFORK_DPTR(dip) - (char*)dip; > + > + memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - used); This zeroes both data fork and attr fork roots for fifo and device inodes. Wouldn't we want to preserve the device number information? And whatever attrs are attached to the inode, if any? --D > + return 1; > + } > +} > + > /* > * when we process the inode, we may change the data in the data and/or > * attribute fork if they are in short form and we are obfuscating names. > @@ -2253,7 +2341,9 @@ process_inode( > case S_IFREG: > success = process_inode_data(dip, TYP_DATA); > break; > - default: ; > + default: > + success = process_dev_inode(dip); > + break; > } > nametable_clear(); > > -- > 2.14.4 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] xfs_metadump: Zap more stale data 2018-10-23 15:49 ` Darrick J. Wong @ 2018-10-25 16:42 ` Stefan Ring 2018-10-26 20:23 ` Stefan Ring 2018-10-26 5:32 ` Stefan Ring 1 sibling, 1 reply; 16+ messages in thread From: Stefan Ring @ 2018-10-25 16:42 UTC (permalink / raw) To: linux-xfs On Tue, Oct 23, 2018 at 5:49 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Oct 11, 2018 at 09:44:24PM +0200, Stefan Ring wrote: > > I have empirically found and tried to fix some places where stale data was not > > properly zeroed out. > > > > In the order of the code changes: > > > > - The "freeindex" blocks in inode directories, from last entry to end of block. > > - XFS_DIR{2,3}_LEAFN_MAGIC, from last entry to end of block. > > - In btree format inodes before as well as after the btree pointers. > > - In dev inodes, everything after the header. > > --- > > db/metadump.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 98 insertions(+), 8 deletions(-) > > > > diff --git a/db/metadump.c b/db/metadump.c > > index ff96860d..c8213d41 100644 > > --- a/db/metadump.c > > +++ b/db/metadump.c > > @@ -1421,12 +1421,49 @@ process_sf_attr( > > memset(asfep, 0, XFS_DFORK_ASIZE(dip, mp) - ino_attr_size); > > } > > > > +static void > > +process_dir_free_block( > > I think patch 1 references this function in patch 2? If this is the case, oops. I thought about this while preparing the diffs but forgot about it again... I should find some time this weekend for another round of polishing these patches. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] xfs_metadump: Zap more stale data 2018-10-25 16:42 ` Stefan Ring @ 2018-10-26 20:23 ` Stefan Ring 0 siblings, 0 replies; 16+ messages in thread From: Stefan Ring @ 2018-10-26 20:23 UTC (permalink / raw) To: linux-xfs On Thu, Oct 25, 2018 at 6:42 PM Stefan Ring <stefanrin@gmail.com> wrote: > > I should find some time this weekend for another round of polishing > these patches. Weekend came early this week (national holiday). The new series is out. I needed this after spending the entire day debugging Java SOAP libraries ;). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] xfs_metadump: Zap more stale data 2018-10-23 15:49 ` Darrick J. Wong 2018-10-25 16:42 ` Stefan Ring @ 2018-10-26 5:32 ` Stefan Ring 1 sibling, 0 replies; 16+ messages in thread From: Stefan Ring @ 2018-10-26 5:32 UTC (permalink / raw) To: darrick.wong; +Cc: linux-xfs On Tue, Oct 23, 2018 at 5:49 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Oct 11, 2018 at 09:44:24PM +0200, Stefan Ring wrote: > > I have empirically found and tried to fix some places where stale data was not > > properly zeroed out. > > > > In the order of the code changes: > > > > - The "freeindex" blocks in inode directories, from last entry to end of block. > > - XFS_DIR{2,3}_LEAFN_MAGIC, from last entry to end of block. > > - In btree format inodes before as well as after the btree pointers. > > - In dev inodes, everything after the header. > > --- > > db/metadump.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 98 insertions(+), 8 deletions(-) > > > > diff --git a/db/metadump.c b/db/metadump.c > > index ff96860d..c8213d41 100644 > > --- a/db/metadump.c > > +++ b/db/metadump.c > > @@ -2118,6 +2173,21 @@ process_btinode( > > } > > > > pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs); > > + > > + if (zero_stale_data) { > > + char *top; > > + int used; > > + > > + /* Space before btree pointers */ > > + top = (char*)XFS_BMDR_PTR_ADDR(dib, 1, nrecs); > > + memset(top, 0, (char*)pp - top); > > + > > + /* Space after btree pointers */ > > + top = (char*)&pp[nrecs]; > > + used = top - (char*)dip; > > + memset(top, 0, mp->m_sb.sb_inodesize - used); > > Er... this zaps everything past the end of the bmbt pointers, which > means that this can zero the attr fork root. True, this is broken. I'll play around a little more with this one. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] Try to squash metadump data leaks 2018-10-11 19:44 [PATCH v3 0/2] Try to squash metadump data leaks Stefan Ring 2018-10-11 19:44 ` [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks Stefan Ring 2018-10-11 19:44 ` [PATCH 2/2] xfs_metadump: Zap more stale data Stefan Ring @ 2018-10-25 15:50 ` Eric Sandeen 2018-10-26 0:36 ` Eric Sandeen 2 siblings, 1 reply; 16+ messages in thread From: Eric Sandeen @ 2018-10-25 15:50 UTC (permalink / raw) To: Stefan Ring, linux-xfs On 10/11/18 2:44 PM, Stefan Ring wrote: > This is the same as v2, but with the correct e-mail address this time. Sorry! > > Stefan Ring (2): > xfs_metadump: Extend zapping to multi fsb dir blocks > xfs_metadump: Zap more stale data > > db/metadump.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 107 insertions(+), 14 deletions(-) Still reading through the patches (this stuff makes my brain hurt) but I think it'd be nice to structure this in a more incremental way, perhaps: 1) fix dir leaf block processing to handle LEAFN 2) add existing leaf block processing to multi-fsb dirs 3) add the dir2 free processing function, and add it to both single & multi-fsb dirs 4) add the process_btinode fixes 5) add the device inode processing 1,2,3 are dependent, 4 and 5 should be independent, they can each be reviewed for their sole purpose. One change, one patch. :) Thanks, -Eric ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] Try to squash metadump data leaks 2018-10-25 15:50 ` [PATCH v3 0/2] Try to squash metadump data leaks Eric Sandeen @ 2018-10-26 0:36 ` Eric Sandeen 2018-10-26 5:29 ` Stefan Ring 0 siblings, 1 reply; 16+ messages in thread From: Eric Sandeen @ 2018-10-26 0:36 UTC (permalink / raw) To: Stefan Ring, linux-xfs On 10/25/18 10:50 AM, Eric Sandeen wrote: > On 10/11/18 2:44 PM, Stefan Ring wrote: >> This is the same as v2, but with the correct e-mail address this time. Sorry! >> >> Stefan Ring (2): >> xfs_metadump: Extend zapping to multi fsb dir blocks >> xfs_metadump: Zap more stale data >> >> db/metadump.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 107 insertions(+), 14 deletions(-) > > Still reading through the patches (this stuff makes my brain hurt) > > but I think it'd be nice to structure this in a more incremental way, perhaps: > > 1) fix dir leaf block processing to handle LEAFN > 2) add existing leaf block processing to multi-fsb dirs > 3) add the dir2 free processing function, and add it to both single & multi-fsb dirs > 4) add the process_btinode fixes > 5) add the device inode processing > > 1,2,3 are dependent, 4 and 5 should be independent, they can each be > reviewed for their sole purpose. One change, one patch. :) Hm, I think another case we're missing is interior nodes on multi-fsb objects, i.e. everything under /* Zero unused part of interior nodes */ seems to be missing for the process_multi_fsb_objects case as well. Urk, what a mess. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] Try to squash metadump data leaks 2018-10-26 0:36 ` Eric Sandeen @ 2018-10-26 5:29 ` Stefan Ring 2018-10-26 12:36 ` Eric Sandeen 0 siblings, 1 reply; 16+ messages in thread From: Stefan Ring @ 2018-10-26 5:29 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Fri, Oct 26, 2018 at 2:36 AM Eric Sandeen <sandeen@sandeen.net> wrote: >> Hm, I think another case we're missing is interior nodes on multi-fsb objects, > i.e. everything under /* Zero unused part of interior nodes */ seems to be > missing for the process_multi_fsb_objects case as well. Urk, what a mess. It is a classic game of whack-a-mole ;). I do understand that it is this way for historical reasons, but if it would be redone today, it should obviously use zeroed buffers for writing out and copy just the bits that are known to be needed. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] Try to squash metadump data leaks 2018-10-26 5:29 ` Stefan Ring @ 2018-10-26 12:36 ` Eric Sandeen 2018-10-26 18:37 ` Stefan Ring 0 siblings, 1 reply; 16+ messages in thread From: Eric Sandeen @ 2018-10-26 12:36 UTC (permalink / raw) To: Stefan Ring; +Cc: linux-xfs On 10/26/18 12:29 AM, Stefan Ring wrote: > On Fri, Oct 26, 2018 at 2:36 AM Eric Sandeen <sandeen@sandeen.net> wrote: >>> Hm, I think another case we're missing is interior nodes on multi-fsb objects, >> i.e. everything under /* Zero unused part of interior nodes */ seems to be >> missing for the process_multi_fsb_objects case as well. Urk, what a mess. > > It is a classic game of whack-a-mole ;). I do understand that it is > this way for historical reasons, but if it would be redone today, it > should obviously use zeroed buffers for writing out and copy just the > bits that are known to be needed. Yeah, I was thinking about that last night. I'm not sure it'd be a whole lot easier, though - for every single type of metadata block we either need to zap the holes or copy the bits, but every boundary is the same and would need to be computed and handled regardless. I'm not sure it'd be a win. -Eric ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] Try to squash metadump data leaks 2018-10-26 12:36 ` Eric Sandeen @ 2018-10-26 18:37 ` Stefan Ring 0 siblings, 0 replies; 16+ messages in thread From: Stefan Ring @ 2018-10-26 18:37 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Fri, Oct 26, 2018 at 2:36 PM Eric Sandeen <sandeen@sandeen.net> wrote: > On 10/26/18 12:29 AM, Stefan Ring wrote: > > On Fri, Oct 26, 2018 at 2:36 AM Eric Sandeen <sandeen@sandeen.net> wrote: > >>> Hm, I think another case we're missing is interior nodes on multi-fsb objects, > >> i.e. everything under /* Zero unused part of interior nodes */ seems to be > >> missing for the process_multi_fsb_objects case as well. Urk, what a mess. > > > > It is a classic game of whack-a-mole ;). I do understand that it is > > this way for historical reasons, but if it would be redone today, it > > should obviously use zeroed buffers for writing out and copy just the > > bits that are known to be needed. > > Yeah, I was thinking about that last night. I'm not sure it'd be a whole > lot easier, though - for every single type of metadata block we either > need to zap the holes or copy the bits, but every boundary is the same > and would need to be computed and handled regardless. > I'm not sure it'd be a win. Yes, it would not be any easier. But it would be (amost) immediately obvious when something was missed. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] Try to squash metadump data leaks @ 2018-10-11 19:23 Stefan Ring 2018-10-11 19:23 ` [PATCH 2/2] xfs_metadump: Zap more stale data Stefan Ring 0 siblings, 1 reply; 16+ messages in thread From: Stefan Ring @ 2018-10-11 19:23 UTC (permalink / raw) To: linux-xfs Since the initial version, I have added the handling of v3 dirs, done some reformatting, added a second changeset because some parts where not processed for zapping on file systems with multi-fsb dir blocks, and also adapted my new code to cope with multi-fsb (which amounted to nothing more than swapping m_sb.sb_blocksize for m_dir_geo->blksize). I tested all my changes with a v3 image and made sure to hit all the touched code paths. Stefan Ring (2): xfs_metadump: Extend zapping to multi fsb dir blocks xfs_metadump: Zap more stale data db/metadump.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 107 insertions(+), 14 deletions(-) -- 2.14.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] xfs_metadump: Zap more stale data 2018-10-11 19:23 [PATCH v2 " Stefan Ring @ 2018-10-11 19:23 ` Stefan Ring 0 siblings, 0 replies; 16+ messages in thread From: Stefan Ring @ 2018-10-11 19:23 UTC (permalink / raw) To: linux-xfs; +Cc: Stefan Ring From: Stefan Ring <str@visotech.com> I have empirically found and tried to fix some places where stale data was not properly zeroed out. In the order of the code changes: - The "freeindex" blocks in inode directories, from last entry to end of block. - XFS_DIR{2,3}_LEAFN_MAGIC, from last entry to end of block. - In btree format inodes before as well as after the btree pointers. - In dev inodes, everything after the header. --- db/metadump.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 98 insertions(+), 8 deletions(-) diff --git a/db/metadump.c b/db/metadump.c index ff96860d..c8213d41 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -1421,12 +1421,49 @@ process_sf_attr( memset(asfep, 0, XFS_DFORK_ASIZE(dip, mp) - ino_attr_size); } +static void +process_dir_free_block( + char *block) +{ + struct xfs_dir2_free *free; + struct xfs_dir3_icfree_hdr freehdr; + + if (!zero_stale_data) + return; + + free = (struct xfs_dir2_free *)block; + M_DIROPS(mp)->free_hdr_from_disk(&freehdr, free); + + switch (freehdr.magic) { + case XFS_DIR2_FREE_MAGIC: + case XFS_DIR3_FREE_MAGIC: { + __be16 *bests; + char *high; + int used; + + /* Zero out space from end of bests[] to end of block */ + bests = M_DIROPS(mp)->free_bests_p(free); + high = (char *)&bests[freehdr.nvalid]; + used = high - (char*)free; + memset(high, 0, mp->m_dir_geo->blksize - used); + iocur_top->need_crc = 1; + break; + } + default: + if (show_warnings) + print_warning("invalid magic in dir inode %llu " + "free block", + (unsigned long long)cur_ino); + break; + } +} + static void process_dir_leaf_block( char *block) { struct xfs_dir2_leaf *leaf; - struct xfs_dir3_icleaf_hdr leafhdr; + struct xfs_dir3_icleaf_hdr leafhdr; if (!zero_stale_data) return; @@ -1435,20 +1472,39 @@ process_dir_leaf_block( leaf = (struct xfs_dir2_leaf *)block; M_DIROPS(mp)->leaf_hdr_from_disk(&leafhdr, leaf); - /* Zero out space from end of ents[] to bests */ - if (leafhdr.magic == XFS_DIR2_LEAF1_MAGIC || - leafhdr.magic == XFS_DIR3_LEAF1_MAGIC) { + switch (leafhdr.magic) { + case XFS_DIR2_LEAF1_MAGIC: + case XFS_DIR3_LEAF1_MAGIC: { struct xfs_dir2_leaf_tail *ltp; __be16 *lbp; struct xfs_dir2_leaf_entry *ents; char *free; /* end of ents */ + /* Zero out space from end of ents[] to bests */ ents = M_DIROPS(mp)->leaf_ents_p(leaf); free = (char *)&ents[leafhdr.count]; ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); lbp = xfs_dir2_leaf_bests_p(ltp); memset(free, 0, (char *)lbp - free); iocur_top->need_crc = 1; + break; + } + case XFS_DIR2_LEAFN_MAGIC: + case XFS_DIR3_LEAFN_MAGIC: { + struct xfs_dir2_leaf_entry *ents; + char *free; + int used; + + /* Zero out space from end of ents[] to end of block */ + ents = M_DIROPS(mp)->leaf_ents_p(leaf); + free = (char *)&ents[leafhdr.count]; + used = free - (char*)leaf; + memset(free, 0, mp->m_dir_geo->blksize - used); + iocur_top->need_crc = 1; + break; + } + default: + break; } } @@ -1499,7 +1555,7 @@ process_dir_data_block( if (show_warnings) print_warning( "invalid magic in dir inode %llu block %ld", - (long long)cur_ino, (long)offset); + (unsigned long long)cur_ino, (long)offset); return; } @@ -1813,8 +1869,7 @@ process_single_fsb_objects( switch (btype) { case TYP_DIR2: if (o >= mp->m_dir_geo->freeblk) { - /* TODO, zap any stale data */ - break; + process_dir_free_block(dp); } else if (o >= mp->m_dir_geo->leafblk) { process_dir_leaf_block(dp); } else { @@ -2118,6 +2173,21 @@ process_btinode( } pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs); + + if (zero_stale_data) { + char *top; + int used; + + /* Space before btree pointers */ + top = (char*)XFS_BMDR_PTR_ADDR(dib, 1, nrecs); + memset(top, 0, (char*)pp - top); + + /* Space after btree pointers */ + top = (char*)&pp[nrecs]; + used = top - (char*)dip; + memset(top, 0, mp->m_sb.sb_inodesize - used); + } + for (i = 0; i < nrecs; i++) { xfs_agnumber_t ag; xfs_agblock_t bno; @@ -2201,6 +2271,24 @@ process_inode_data( return 1; } +static int +process_dev_inode( + xfs_dinode_t *dip) +{ + if (XFS_DFORK_NEXTENTS(dip, XFS_ATTR_FORK) || + XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) { + if (show_warnings) + print_warning("inode %llu has unexpected extents", + (unsigned long long)cur_ino); + return 0; + } else { + int used = XFS_DFORK_DPTR(dip) - (char*)dip; + + memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - used); + return 1; + } +} + /* * when we process the inode, we may change the data in the data and/or * attribute fork if they are in short form and we are obfuscating names. @@ -2253,7 +2341,9 @@ process_inode( case S_IFREG: success = process_inode_data(dip, TYP_DATA); break; - default: ; + default: + success = process_dev_inode(dip); + break; } nametable_clear(); -- 2.14.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-10-27 5:02 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-11 19:44 [PATCH v3 0/2] Try to squash metadump data leaks Stefan Ring 2018-10-11 19:44 ` [PATCH 1/2] xfs_metadump: Extend zapping to multi fsb dir blocks Stefan Ring 2018-10-23 15:10 ` Eric Sandeen 2018-10-25 15:08 ` Eric Sandeen 2018-10-25 16:44 ` Stefan Ring 2018-10-11 19:44 ` [PATCH 2/2] xfs_metadump: Zap more stale data Stefan Ring 2018-10-23 15:49 ` Darrick J. Wong 2018-10-25 16:42 ` Stefan Ring 2018-10-26 20:23 ` Stefan Ring 2018-10-26 5:32 ` Stefan Ring 2018-10-25 15:50 ` [PATCH v3 0/2] Try to squash metadump data leaks Eric Sandeen 2018-10-26 0:36 ` Eric Sandeen 2018-10-26 5:29 ` Stefan Ring 2018-10-26 12:36 ` Eric Sandeen 2018-10-26 18:37 ` Stefan Ring -- strict thread matches above, loose matches on Subject: below -- 2018-10-11 19:23 [PATCH v2 " Stefan Ring 2018-10-11 19:23 ` [PATCH 2/2] xfs_metadump: Zap more stale data Stefan Ring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).