* [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs @ 2017-08-02 2:35 Eric Sandeen 2017-08-02 13:54 ` Brian Foster 2017-08-10 21:30 ` [PATCH V2] " Eric Sandeen 0 siblings, 2 replies; 6+ messages in thread From: Eric Sandeen @ 2017-08-02 2:35 UTC (permalink / raw) To: linux-xfs; +Cc: Stefan Ring xfs_metadump attempts to zero out unused regions of metadata blocks to prevent data leaks when sharing metadata images. However, Stefan Ring reported a significant number of leaked strings when dumping his 1T filesystem. Based on a reduced metadata set, I was able to identify "leaf" directories (with XFS_DIR2_LEAF1_MAGIC magic) as the primary culprit; the region between the end of the entries array and the start of the bests array was not getting zeroed out. This patch seems to remedy that problem. Reported-by: Stefan Ring <stefanrin@gmail.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- I may have missed some handy macro to work out some of the math below, if so I'd be perfectly happy to hear about it ;) diff --git a/db/metadump.c b/db/metadump.c index 96641e0..6d77d61 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -1459,6 +1459,37 @@ process_dir_data_block( int wantmagic; struct xfs_dir2_data_hdr *datahdr; + if (offset >= mp->m_dir_geo->freeblk) { + /* TODO */ + return; + } else if (offset >= mp->m_dir_geo->leafblk) { + struct xfs_dir2_leaf *leaf; + struct xfs_dir2_leaf_tail *ltp; + struct xfs_dir3_icleaf_hdr leafhdr; + + if (!zero_stale_data) + return; + + leaf = (struct xfs_dir2_leaf *)block; + ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); + 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) { + struct xfs_dir2_leaf_entry *ents; + __be16 *lbp; + char *start; /* end of ents */ + + ents = M_DIROPS(mp)->leaf_ents_p(leaf); + start = (char *)&ents[leafhdr.count + leafhdr.stale]; + lbp = xfs_dir2_leaf_bests_p(ltp); + memset(start, 0, (char *)lbp - start); + iocur_top->need_crc = 1; + } + return; + } + + /* It's a data block. */ datahdr = (struct xfs_dir2_data_hdr *)block; if (is_block_format) { @@ -1800,9 +1831,6 @@ process_single_fsb_objects( dp = iocur_top->data; switch (btype) { case TYP_DIR2: - if (o >= mp->m_dir_geo->leafblk) - break; - process_dir_data_block(dp, o, last == mp->m_dir_geo->fsbcount); iocur_top->need_crc = 1; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs 2017-08-02 2:35 [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs Eric Sandeen @ 2017-08-02 13:54 ` Brian Foster 2017-08-02 15:16 ` Eric Sandeen 2017-08-10 21:30 ` [PATCH V2] " Eric Sandeen 1 sibling, 1 reply; 6+ messages in thread From: Brian Foster @ 2017-08-02 13:54 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs, Stefan Ring On Tue, Aug 01, 2017 at 09:35:27PM -0500, Eric Sandeen wrote: > xfs_metadump attempts to zero out unused regions of metadata > blocks to prevent data leaks when sharing metadata images. > > However, Stefan Ring reported a significant number of leaked > strings when dumping his 1T filesystem. Based on a reduced > metadata set, I was able to identify "leaf" directories > (with XFS_DIR2_LEAF1_MAGIC magic) as the primary culprit; > the region between the end of the entries array and the start > of the bests array was not getting zeroed out. This patch > seems to remedy that problem. > > Reported-by: Stefan Ring <stefanrin@gmail.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > I may have missed some handy macro to work out some of the > math below, if so I'd be perfectly happy to hear about it ;) > Looks good to me. A couple nits... > diff --git a/db/metadump.c b/db/metadump.c > index 96641e0..6d77d61 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -1459,6 +1459,37 @@ process_dir_data_block( > int wantmagic; > struct xfs_dir2_data_hdr *datahdr; > > + if (offset >= mp->m_dir_geo->freeblk) { > + /* TODO */ > + return; TODO what exactly? Zero from the end of the bests array to the end of the block? We could at least add the if (!zero_stale_data) check here too. > + } else if (offset >= mp->m_dir_geo->leafblk) { > + struct xfs_dir2_leaf *leaf; > + struct xfs_dir2_leaf_tail *ltp; > + struct xfs_dir3_icleaf_hdr leafhdr; > + > + if (!zero_stale_data) > + return; > + > + leaf = (struct xfs_dir2_leaf *)block; > + ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); ltp isn't used until the block of code below (which already has locally scoped vars). Brian > + 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) { > + struct xfs_dir2_leaf_entry *ents; > + __be16 *lbp; > + char *start; /* end of ents */ > + > + ents = M_DIROPS(mp)->leaf_ents_p(leaf); > + start = (char *)&ents[leafhdr.count + leafhdr.stale]; > + lbp = xfs_dir2_leaf_bests_p(ltp); > + memset(start, 0, (char *)lbp - start); > + iocur_top->need_crc = 1; > + } > + return; > + } > + > + /* It's a data block. */ > datahdr = (struct xfs_dir2_data_hdr *)block; > > if (is_block_format) { > @@ -1800,9 +1831,6 @@ process_single_fsb_objects( > dp = iocur_top->data; > switch (btype) { > case TYP_DIR2: > - if (o >= mp->m_dir_geo->leafblk) > - break; > - > process_dir_data_block(dp, o, > last == mp->m_dir_geo->fsbcount); > iocur_top->need_crc = 1; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs 2017-08-02 13:54 ` Brian Foster @ 2017-08-02 15:16 ` Eric Sandeen 2017-08-02 16:09 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Eric Sandeen @ 2017-08-02 15:16 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs, Stefan Ring On 8/2/17 8:54 AM, Brian Foster wrote: > On Tue, Aug 01, 2017 at 09:35:27PM -0500, Eric Sandeen wrote: >> xfs_metadump attempts to zero out unused regions of metadata >> blocks to prevent data leaks when sharing metadata images. >> >> However, Stefan Ring reported a significant number of leaked >> strings when dumping his 1T filesystem. Based on a reduced >> metadata set, I was able to identify "leaf" directories >> (with XFS_DIR2_LEAF1_MAGIC magic) as the primary culprit; >> the region between the end of the entries array and the start >> of the bests array was not getting zeroed out. This patch >> seems to remedy that problem. >> >> Reported-by: Stefan Ring <stefanrin@gmail.com> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> I may have missed some handy macro to work out some of the >> math below, if so I'd be perfectly happy to hear about it ;) >> > > Looks good to me. A couple nits... > >> diff --git a/db/metadump.c b/db/metadump.c >> index 96641e0..6d77d61 100644 >> --- a/db/metadump.c >> +++ b/db/metadump.c >> @@ -1459,6 +1459,37 @@ process_dir_data_block( >> int wantmagic; >> struct xfs_dir2_data_hdr *datahdr; >> >> + if (offset >= mp->m_dir_geo->freeblk) { >> + /* TODO */ >> + return; > > TODO what exactly? Zero from the end of the bests array to the end of > the block? We could at least add the if (!zero_stale_data) check here > too. TODO: see if there's any work to do here ;) Yes, end of bests to end of block, I think. >> + } else if (offset >= mp->m_dir_geo->leafblk) { >> + struct xfs_dir2_leaf *leaf; >> + struct xfs_dir2_leaf_tail *ltp; >> + struct xfs_dir3_icleaf_hdr leafhdr; >> + >> + if (!zero_stale_data) >> + return; >> + >> + leaf = (struct xfs_dir2_leaf *)block; >> + ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); > > ltp isn't used until the block of code below (which already has locally > scoped vars). Ok, oops, moved most. thanks. Dave also points out that I should be checking DIR3_LEAF1_MAGIC as well. Thanks, -Eric > > Brian > >> + 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) { >> + struct xfs_dir2_leaf_entry *ents; >> + __be16 *lbp; >> + char *start; /* end of ents */ >> + >> + ents = M_DIROPS(mp)->leaf_ents_p(leaf); >> + start = (char *)&ents[leafhdr.count + leafhdr.stale]; >> + lbp = xfs_dir2_leaf_bests_p(ltp); >> + memset(start, 0, (char *)lbp - start); >> + iocur_top->need_crc = 1; >> + } >> + return; >> + } >> + >> + /* It's a data block. */ >> datahdr = (struct xfs_dir2_data_hdr *)block; >> >> if (is_block_format) { >> @@ -1800,9 +1831,6 @@ process_single_fsb_objects( >> dp = iocur_top->data; >> switch (btype) { >> case TYP_DIR2: >> - if (o >= mp->m_dir_geo->leafblk) >> - break; >> - >> process_dir_data_block(dp, o, >> last == mp->m_dir_geo->fsbcount); >> iocur_top->need_crc = 1; >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs 2017-08-02 15:16 ` Eric Sandeen @ 2017-08-02 16:09 ` Darrick J. Wong 0 siblings, 0 replies; 6+ messages in thread From: Darrick J. Wong @ 2017-08-02 16:09 UTC (permalink / raw) To: Eric Sandeen; +Cc: Brian Foster, linux-xfs, Stefan Ring On Wed, Aug 02, 2017 at 10:16:06AM -0500, Eric Sandeen wrote: > On 8/2/17 8:54 AM, Brian Foster wrote: > > On Tue, Aug 01, 2017 at 09:35:27PM -0500, Eric Sandeen wrote: > >> xfs_metadump attempts to zero out unused regions of metadata > >> blocks to prevent data leaks when sharing metadata images. > >> > >> However, Stefan Ring reported a significant number of leaked > >> strings when dumping his 1T filesystem. Based on a reduced > >> metadata set, I was able to identify "leaf" directories > >> (with XFS_DIR2_LEAF1_MAGIC magic) as the primary culprit; > >> the region between the end of the entries array and the start > >> of the bests array was not getting zeroed out. This patch > >> seems to remedy that problem. > >> > >> Reported-by: Stefan Ring <stefanrin@gmail.com> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > >> > >> I may have missed some handy macro to work out some of the > >> math below, if so I'd be perfectly happy to hear about it ;) > >> > > > > Looks good to me. A couple nits... > > > >> diff --git a/db/metadump.c b/db/metadump.c > >> index 96641e0..6d77d61 100644 > >> --- a/db/metadump.c > >> +++ b/db/metadump.c > >> @@ -1459,6 +1459,37 @@ process_dir_data_block( > >> int wantmagic; > >> struct xfs_dir2_data_hdr *datahdr; > >> > >> + if (offset >= mp->m_dir_geo->freeblk) { > >> + /* TODO */ > >> + return; > > > > TODO what exactly? Zero from the end of the bests array to the end of > > the block? We could at least add the if (!zero_stale_data) check here > > too. > > TODO: see if there's any work to do here ;) Yes, end of bests > to end of block, I think. > > >> + } else if (offset >= mp->m_dir_geo->leafblk) { > >> + struct xfs_dir2_leaf *leaf; > >> + struct xfs_dir2_leaf_tail *ltp; > >> + struct xfs_dir3_icleaf_hdr leafhdr; > >> + > >> + if (!zero_stale_data) > >> + return; > >> + > >> + leaf = (struct xfs_dir2_leaf *)block; > >> + ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf); > > > > ltp isn't used until the block of code below (which already has locally > > scoped vars). > > Ok, oops, moved most. thanks. > > Dave also points out that I should be checking DIR3_LEAF1_MAGIC > as well. > > Thanks, > -Eric > > > > > Brian > > > >> + 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) { > >> + struct xfs_dir2_leaf_entry *ents; > >> + __be16 *lbp; > >> + char *start; /* end of ents */ > >> + > >> + ents = M_DIROPS(mp)->leaf_ents_p(leaf); > >> + start = (char *)&ents[leafhdr.count + leafhdr.stale]; > >> + lbp = xfs_dir2_leaf_bests_p(ltp); > >> + memset(start, 0, (char *)lbp - start); > >> + iocur_top->need_crc = 1; > >> + } > >> + return; > >> + } Ugh, this function is losing cohesion. Can we give the leaf and free block handlers a separate function and dispatch them directly from the case TYP_DIR2 clause below, instead of cluttering up the dir data/block processing function? --D > >> + > >> + /* It's a data block. */ > >> datahdr = (struct xfs_dir2_data_hdr *)block; > >> > >> if (is_block_format) { > >> @@ -1800,9 +1831,6 @@ process_single_fsb_objects( > >> dp = iocur_top->data; > >> switch (btype) { > >> case TYP_DIR2: > >> - if (o >= mp->m_dir_geo->leafblk) > >> - break; > >> - > >> process_dir_data_block(dp, o, > >> last == mp->m_dir_geo->fsbcount); > >> iocur_top->need_crc = 1; > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] xfs_metadump: zap stale date in DIR2_LEAF1 dirs 2017-08-02 2:35 [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs Eric Sandeen 2017-08-02 13:54 ` Brian Foster @ 2017-08-10 21:30 ` Eric Sandeen 2017-08-14 12:40 ` Brian Foster 1 sibling, 1 reply; 6+ messages in thread From: Eric Sandeen @ 2017-08-10 21:30 UTC (permalink / raw) To: Eric Sandeen, linux-xfs; +Cc: Stefan Ring xfs_metadump attempts to zero out unused regions of metadata blocks to prevent data leaks when sharing metadata images. However, Stefan Ring reported a significant number of leaked strings when dumping his 1T filesystem. Based on a reduced metadata set, I was able to identify "leaf" directories (with XFS_DIR2_LEAF1_MAGIC magic) as the primary culprit; the region between the end of the entries array and the start of the bests array was not getting zeroed out. This patch seems to remedy that problem. Reported-by: Stefan Ring <stefanrin@gmail.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: - factor into new function, process_dir_leaf_block - add DIR3_LEAF1_MAGIC - don't add count+stale; count includes stale entries - move ltp into code block diff --git a/db/metadump.c b/db/metadump.c index 3967df6..a8756d6 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -1442,6 +1442,37 @@ process_sf_attr( } static void +process_dir_leaf_block( + char *block) +{ + struct xfs_dir2_leaf *leaf; + struct xfs_dir3_icleaf_hdr leafhdr; + + if (!zero_stale_data) + return; + + /* Yes, this works for dir2 & dir3. Difference is padding. */ + 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) { + struct xfs_dir2_leaf_tail *ltp; + __be16 *lbp; + struct xfs_dir2_leaf_entry *ents; + char *free; /* end of ents */ + + 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; + } +} + +static void process_dir_data_block( char *block, xfs_fileoff_t offset, @@ -1801,11 +1832,15 @@ process_single_fsb_objects( dp = iocur_top->data; switch (btype) { case TYP_DIR2: - if (o >= mp->m_dir_geo->leafblk) + if (o >= mp->m_dir_geo->freeblk) { + /* TODO, zap any stale data */ break; - - process_dir_data_block(dp, o, + } 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); + } iocur_top->need_crc = 1; break; case TYP_SYMLINK: ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] xfs_metadump: zap stale date in DIR2_LEAF1 dirs 2017-08-10 21:30 ` [PATCH V2] " Eric Sandeen @ 2017-08-14 12:40 ` Brian Foster 0 siblings, 0 replies; 6+ messages in thread From: Brian Foster @ 2017-08-14 12:40 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs, Stefan Ring On Thu, Aug 10, 2017 at 02:30:10PM -0700, Eric Sandeen wrote: > xfs_metadump attempts to zero out unused regions of metadata > blocks to prevent data leaks when sharing metadata images. > > However, Stefan Ring reported a significant number of leaked > strings when dumping his 1T filesystem. Based on a reduced > metadata set, I was able to identify "leaf" directories > (with XFS_DIR2_LEAF1_MAGIC magic) as the primary culprit; > the region between the end of the entries array and the start > of the bests array was not getting zeroed out. This patch > seems to remedy that problem. > > Reported-by: Stefan Ring <stefanrin@gmail.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > > V2: > > - factor into new function, process_dir_leaf_block > - add DIR3_LEAF1_MAGIC > - don't add count+stale; count includes stale entries > - move ltp into code block > > diff --git a/db/metadump.c b/db/metadump.c > index 3967df6..a8756d6 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -1442,6 +1442,37 @@ process_sf_attr( > } > > static void > +process_dir_leaf_block( > + char *block) > +{ > + struct xfs_dir2_leaf *leaf; > + struct xfs_dir3_icleaf_hdr leafhdr; > + > + if (!zero_stale_data) > + return; > + > + /* Yes, this works for dir2 & dir3. Difference is padding. */ > + 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) { > + struct xfs_dir2_leaf_tail *ltp; > + __be16 *lbp; > + struct xfs_dir2_leaf_entry *ents; > + char *free; /* end of ents */ > + > + 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; > + } > +} > + > +static void > process_dir_data_block( > char *block, > xfs_fileoff_t offset, > @@ -1801,11 +1832,15 @@ process_single_fsb_objects( > dp = iocur_top->data; > switch (btype) { > case TYP_DIR2: > - if (o >= mp->m_dir_geo->leafblk) > + if (o >= mp->m_dir_geo->freeblk) { > + /* TODO, zap any stale data */ > break; > - > - process_dir_data_block(dp, o, > + } 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); > + } > iocur_top->need_crc = 1; > break; > case TYP_SYMLINK: > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-14 12:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-02 2:35 [PATCH] xfs_metadump: zap stale date in DIR2_LEAF1 dirs Eric Sandeen 2017-08-02 13:54 ` Brian Foster 2017-08-02 15:16 ` Eric Sandeen 2017-08-02 16:09 ` Darrick J. Wong 2017-08-10 21:30 ` [PATCH V2] " Eric Sandeen 2017-08-14 12:40 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox