[PATCH 0/1] Try to squash metadump data leaks

[PATCH 0/1] Try to squash metadump data leaks

[Stefan Ring]

This is inspired by a thread last year when someone intended to
collect metadata about filesystems and I would have been happy to
help, except that I noticed lots of left-over data in the dump that
should never have been there. I would not have worried that much about
some fragments of Python code or directory listings, but the
possibility of recognizable customer data (potentially even
cryptographic keys) made it unthinkable to share this.

My method of coming up with these patches was: Pipe a metadump of my
reference image through "strings -n 10" and scroll until something
recognizable catches my eye. This did not take too long, usually. Find
the origin of the found leak and squash it (using "XFS File System
Structure" from the wiki). Repeat until there is nothing recognizable
left. Said image is a 1.1 TB volume created in early 2012 and used
daily ever since on our development server, containing about 12
million inodes (mostly hundreds of checkouts of our main Mercurial
repo with about 15000 files in it).

I have not submitted a patch before, and I don't think I will be
particularly pushy with this one. It exists mostly to inform you of my
findings. I have not dealt at all with a v3 filesystem. TBH, I don't
even know what this is and how to create one. Looking at the metadump
code as it exists now, it would likely have been much safer to copy
just the required contents as opposed to copying everything and then
trying to find every nook and cranny where unwanted stuff could seep
through.

Stefan Ring (1):
  xfs_metadump: Zap more stale data

 db/metadump.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 5 deletions(-)

-- 
2.14.4

[PATCH 1/1] xfs_metadump: Zap more stale data

[Stefan Ring]

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_DIR2_LEAF1_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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 5 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index cc2ae9af..e7159cd1 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1421,12 +1421,42 @@ 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);
+
+	/* Zero out space from end of bests[] to end of block */
+	if (freehdr.magic == XFS_DIR2_FREE_MAGIC) {
+		__be16			*bests;
+		char			*high;
+		int			used;
+
+		bests = M_DIROPS(mp)->free_bests_p(free);
+		high = (char *)&bests[freehdr.nvalid];
+		used = high - (char*)free;
+		memset(high, 0, mp->m_sb.sb_blocksize - used);
+		iocur_top->need_crc = 1;
+	}
+	else if (show_warnings)
+		print_warning("invalid magic in dir inode %llu free block",
+				(unsigned long long)cur_ino);
+}
+
 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;
@@ -1449,6 +1479,18 @@ process_dir_leaf_block(
 		lbp = xfs_dir2_leaf_bests_p(ltp);
 		memset(free, 0, (char *)lbp - free);
 		iocur_top->need_crc = 1;
+	} else
+	if (leafhdr.magic == XFS_DIR2_LEAFN_MAGIC ||
+	    leafhdr.magic == XFS_DIR3_LEAFN_MAGIC) {
+		struct xfs_dir2_leaf_entry	*ents;
+		char				*free;
+		int				used;
+
+		ents = M_DIROPS(mp)->leaf_ents_p(leaf);
+		free = (char *)&ents[leafhdr.count];
+		used = free - (char*)leaf;
+		memset(free, 0, mp->m_sb.sb_blocksize - used);
+		iocur_top->need_crc = 1;
 	}
 }
 
@@ -1499,7 +1541,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 +1855,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 {
@@ -2115,6 +2156,20 @@ process_btinode(
 	}
 
 	pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
+
+	if (zero_stale_data) {
+		/* Space before btree pointers */
+		char	*top;
+		int	used;
+		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;
@@ -2250,7 +2305,16 @@ process_inode(
 		case S_IFREG:
 			success = process_inode_data(dip, TYP_DATA);
 			break;
-		default: ;
+		default:
+			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);
+				success = 0;
+			}
+			else
+				memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - (XFS_DFORK_DPTR(dip) - (char*)dip));
 	}
 	nametable_clear();
 
-- 
2.14.4

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Darrick J. Wong]

On Thu, Oct 04, 2018 at 10:57:49PM +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_DIR2_LEAF1_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.

Mostly looks ok, but with some style issues:

> ---
>  db/metadump.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index cc2ae9af..e7159cd1 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -1421,12 +1421,42 @@ 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);
> +
> +	/* Zero out space from end of bests[] to end of block */
> +	if (freehdr.magic == XFS_DIR2_FREE_MAGIC) {

How about XFS_DIR3_FREE_MAGIC ?

> +		__be16			*bests;
> +		char			*high;
> +		int			used;
> +
> +		bests = M_DIROPS(mp)->free_bests_p(free);
> +		high = (char *)&bests[freehdr.nvalid];
> +		used = high - (char*)free;
> +		memset(high, 0, mp->m_sb.sb_blocksize - used);
> +		iocur_top->need_crc = 1;
> +	}
> +	else if (show_warnings)

Though perhaps this should be structured as:

switch (freehdr.magic) {
case XFS_DIR3_FREE_MAGIC:
case XFS_DIR2_FREE_MAGIC:
	/* clear stuff */
	break;
default:
	if (show_warnings)
		print_warning(...);
	break;
}

> +		print_warning("invalid magic in dir inode %llu free block",
> +				(unsigned long long)cur_ino);
> +}
> +
>  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;
> @@ -1449,6 +1479,18 @@ process_dir_leaf_block(
>  		lbp = xfs_dir2_leaf_bests_p(ltp);
>  		memset(free, 0, (char *)lbp - free);
>  		iocur_top->need_crc = 1;
> +	} else
> +	if (leafhdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> +	    leafhdr.magic == XFS_DIR3_LEAFN_MAGIC) {


Convert this whole thing to a switch?

> +		struct xfs_dir2_leaf_entry	*ents;
> +		char				*free;
> +		int				used;
> +
> +		ents = M_DIROPS(mp)->leaf_ents_p(leaf);
> +		free = (char *)&ents[leafhdr.count];
> +		used = free - (char*)leaf;
> +		memset(free, 0, mp->m_sb.sb_blocksize - used);
> +		iocur_top->need_crc = 1;
>  	}
>  }
>  
> @@ -1499,7 +1541,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);

Unrelated (but still good) change.

>  		return;
>  	}
>  
> @@ -1813,8 +1855,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 {
> @@ -2115,6 +2156,20 @@ process_btinode(
>  	}
>  
>  	pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
> +
> +	if (zero_stale_data) {
> +		/* Space before btree pointers */
> +		char	*top;
> +		int	used;
> +		top = (char*)XFS_BMDR_PTR_ADDR(dib, 1, nrecs);

Blank line between "int used;" and "top =...", please.

> +		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;
> @@ -2250,7 +2305,16 @@ process_inode(
>  		case S_IFREG:
>  			success = process_inode_data(dip, TYP_DATA);
>  			break;
> -		default: ;
> +		default:
> +			if (XFS_DFORK_NEXTENTS(dip, XFS_ATTR_FORK) ||
> +					XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) {

Please line up the XFS_DFORK_NEXTENTS(...) calls.

> +				if (show_warnings)
> +					print_warning("inode %llu has unexpected extents",
> +							(unsigned long long)cur_ino);
> +				success = 0;
> +			}
> +			else

} else
	memset(...)


> +				memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - (XFS_DFORK_DPTR(dip) - (char*)dip));

Please put this in a separate helper that isn't triple-indented and
overflows 80 columns.

--D

>  	}
>  	nametable_clear();
>  
> -- 
> 2.14.4
> 

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Stefan Ring]

On Fri, Oct 5, 2018 at 12:23 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Thu, Oct 04, 2018 at 10:57:49PM +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_DIR2_LEAF1_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.
>
> Mostly looks ok, but with some style issues:
>
> > ---
> >  db/metadump.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 69 insertions(+), 5 deletions(-)
> >
> > diff --git a/db/metadump.c b/db/metadump.c
> > index cc2ae9af..e7159cd1 100644
> > --- a/db/metadump.c
> > +++ b/db/metadump.c
> > @@ -1421,12 +1421,42 @@ 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);
> > +
> > +     /* Zero out space from end of bests[] to end of block */
> > +     if (freehdr.magic == XFS_DIR2_FREE_MAGIC) {
>
> How about XFS_DIR3_FREE_MAGIC ?

Is there any documentation on DIR3? I left it out on purpose and hoped
someone would fill in the blanks for me ;).

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Darrick J. Wong]

On Fri, Oct 05, 2018 at 10:46:28PM +0200, Stefan Ring wrote:
> On Fri, Oct 5, 2018 at 12:23 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > On Thu, Oct 04, 2018 at 10:57:49PM +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_DIR2_LEAF1_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.
> >
> > Mostly looks ok, but with some style issues:
> >
> > > ---
> > >  db/metadump.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 69 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/db/metadump.c b/db/metadump.c
> > > index cc2ae9af..e7159cd1 100644
> > > --- a/db/metadump.c
> > > +++ b/db/metadump.c
> > > @@ -1421,12 +1421,42 @@ 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);
> > > +
> > > +     /* Zero out space from end of bests[] to end of block */
> > > +     if (freehdr.magic == XFS_DIR2_FREE_MAGIC) {
> >
> > How about XFS_DIR3_FREE_MAGIC ?
> 
> Is there any documentation on DIR3? I left it out on purpose and hoped
> someone would fill in the blanks for me ;).

Fairly similar to DIR2, but with bigger headers, I think.

https://djwong.org/docs/kdoc/filesystems/xfs-data-structures/dynamic.html#leaf-directories

--D

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Stefan Ring]

On Thu, Oct 4, 2018 at 10:57 PM Stefan Ring <stefanrin@gmail.com> wrote:
> diff --git a/db/metadump.c b/db/metadump.c
> index cc2ae9af..e7159cd1 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -1421,12 +1421,42 @@ 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);
> +
> +       /* Zero out space from end of bests[] to end of block */
> +       if (freehdr.magic == XFS_DIR2_FREE_MAGIC) {
> +               __be16                  *bests;
> +               char                    *high;
> +               int                     used;
> +
> +               bests = M_DIROPS(mp)->free_bests_p(free);
> +               high = (char *)&bests[freehdr.nvalid];
> +               used = high - (char*)free;
> +               memset(high, 0, mp->m_sb.sb_blocksize - used);
> +               iocur_top->need_crc = 1;
> +       }
> +       else if (show_warnings)
> +               print_warning("invalid magic in dir inode %llu free block",
> +                               (unsigned long long)cur_ino);
> +}
> +
>  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;
> @@ -1449,6 +1479,18 @@ process_dir_leaf_block(
>                 lbp = xfs_dir2_leaf_bests_p(ltp);
>                 memset(free, 0, (char *)lbp - free);
>                 iocur_top->need_crc = 1;
> +       } else
> +       if (leafhdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> +           leafhdr.magic == XFS_DIR3_LEAFN_MAGIC) {
> +               struct xfs_dir2_leaf_entry      *ents;
> +               char                            *free;
> +               int                             used;
> +
> +               ents = M_DIROPS(mp)->leaf_ents_p(leaf);
> +               free = (char *)&ents[leafhdr.count];
> +               used = free - (char*)leaf;
> +               memset(free, 0, mp->m_sb.sb_blocksize - used);

I forgot to mention this: sb_blocksize is used conspicuously rarely in
this file. Is this the right metric to lean on?

> +               iocur_top->need_crc = 1;
>         }
>  }
>
> @@ -1499,7 +1541,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 +1855,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 {
> @@ -2115,6 +2156,20 @@ process_btinode(
>         }
>
>         pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
> +
> +       if (zero_stale_data) {
> +               /* Space before btree pointers */
> +               char    *top;
> +               int     used;
> +               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;
> @@ -2250,7 +2305,16 @@ process_inode(
>                 case S_IFREG:
>                         success = process_inode_data(dip, TYP_DATA);
>                         break;
> -               default: ;
> +               default:
> +                       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);
> +                               success = 0;
> +                       }
> +                       else
> +                               memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - (XFS_DFORK_DPTR(dip) - (char*)dip));
>         }
>         nametable_clear();
>
> --
> 2.14.4
>

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Darrick J. Wong]

On Fri, Oct 05, 2018 at 10:35:34PM +0200, Stefan Ring wrote:
> On Thu, Oct 4, 2018 at 10:57 PM Stefan Ring <stefanrin@gmail.com> wrote:
> > diff --git a/db/metadump.c b/db/metadump.c
> > index cc2ae9af..e7159cd1 100644
> > --- a/db/metadump.c
> > +++ b/db/metadump.c
> > @@ -1421,12 +1421,42 @@ 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);
> > +
> > +       /* Zero out space from end of bests[] to end of block */
> > +       if (freehdr.magic == XFS_DIR2_FREE_MAGIC) {
> > +               __be16                  *bests;
> > +               char                    *high;
> > +               int                     used;
> > +
> > +               bests = M_DIROPS(mp)->free_bests_p(free);
> > +               high = (char *)&bests[freehdr.nvalid];
> > +               used = high - (char*)free;
> > +               memset(high, 0, mp->m_sb.sb_blocksize - used);
> > +               iocur_top->need_crc = 1;
> > +       }
> > +       else if (show_warnings)
> > +               print_warning("invalid magic in dir inode %llu free block",
> > +                               (unsigned long long)cur_ino);
> > +}
> > +
> >  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;
> > @@ -1449,6 +1479,18 @@ process_dir_leaf_block(
> >                 lbp = xfs_dir2_leaf_bests_p(ltp);
> >                 memset(free, 0, (char *)lbp - free);
> >                 iocur_top->need_crc = 1;
> > +       } else
> > +       if (leafhdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> > +           leafhdr.magic == XFS_DIR3_LEAFN_MAGIC) {
> > +               struct xfs_dir2_leaf_entry      *ents;
> > +               char                            *free;
> > +               int                             used;
> > +
> > +               ents = M_DIROPS(mp)->leaf_ents_p(leaf);
> > +               free = (char *)&ents[leafhdr.count];
> > +               used = free - (char*)leaf;
> > +               memset(free, 0, mp->m_sb.sb_blocksize - used);
> 
> I forgot to mention this: sb_blocksize is used conspicuously rarely in
> this file. Is this the right metric to lean on?

No, because directory blocks span multiple fs blocks (good self catch!).

You could look it up in the directory geometry structure in the
xfs_mount *.

--D

> > +               iocur_top->need_crc = 1;
> >         }
> >  }
> >
> > @@ -1499,7 +1541,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 +1855,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 {
> > @@ -2115,6 +2156,20 @@ process_btinode(
> >         }
> >
> >         pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
> > +
> > +       if (zero_stale_data) {
> > +               /* Space before btree pointers */
> > +               char    *top;
> > +               int     used;
> > +               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;
> > @@ -2250,7 +2305,16 @@ process_inode(
> >                 case S_IFREG:
> >                         success = process_inode_data(dip, TYP_DATA);
> >                         break;
> > -               default: ;
> > +               default:
> > +                       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);
> > +                               success = 0;
> > +                       }
> > +                       else
> > +                               memset(XFS_DFORK_DPTR(dip), 0, mp->m_sb.sb_inodesize - (XFS_DFORK_DPTR(dip) - (char*)dip));
> >         }
> >         nametable_clear();
> >
> > --
> > 2.14.4
> >

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Stefan Ring]

On Fri, Oct 5, 2018 at 10:41 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > I forgot to mention this: sb_blocksize is used conspicuously rarely in
> > this file. Is this the right metric to lean on?
>
> No, because directory blocks span multiple fs blocks (good self catch!).
>
> You could look it up in the directory geometry structure in the
> xfs_mount *.

Interestingly my file system has dirblklog = 0. I guess I'll have to
start creating some toy file system images for experimentation.

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Stefan Ring]

On Sun, Oct 7, 2018 at 11:43 AM Stefan Ring <stefanrin@gmail.com> wrote:
>
> Interestingly my file system has dirblklog = 0. I guess I'll have to
> start creating some toy file system images for experimentation.

Ok, I've found out how to create one with dirblklog > 0, but the
default seems to be, and have been for a very long time, zero
(directory blocks are the same size as file system blocks). I'm afraid
of uncovering all kinds of nastiness going this untrodden path…

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Darrick J. Wong]

On Sun, Oct 07, 2018 at 01:57:10PM +0200, Stefan Ring wrote:
> On Sun, Oct 7, 2018 at 11:43 AM Stefan Ring <stefanrin@gmail.com> wrote:
> >
> > Interestingly my file system has dirblklog = 0. I guess I'll have to
> > start creating some toy file system images for experimentation.
> 
> Ok, I've found out how to create one with dirblklog > 0, but the
> default seems to be, and have been for a very long time, zero
> (directory blocks are the same size as file system blocks). I'm afraid
> of uncovering all kinds of nastiness going this untrodden path…

Nothing is ever simple in xfs, sadly. :/

That said, I think the "recommended" ceph configuration still has
multiblock directories, so I think those code paths are somewhat
frequently used by some of the users.

(I guess you could temporarily change the patch to memset(buf, 'a', ...)
to make sure it only ever touches the empty parts of the dir block buffer.)

--D

Re: [PATCH 1/1] xfs_metadump: Zap more stale data

[Stefan Ring]

On Sun, Oct 7, 2018 at 1:57 PM Stefan Ring <stefanrin@gmail.com> wrote:
>
> On Sun, Oct 7, 2018 at 11:43 AM Stefan Ring <stefanrin@gmail.com> wrote:
> >
> > Interestingly my file system has dirblklog = 0. I guess I'll have to
> > start creating some toy file system images for experimentation.
>
> Ok, I've found out how to create one with dirblklog > 0, but the
> default seems to be, and have been for a very long time, zero
> (directory blocks are the same size as file system blocks). I'm afraid
> of uncovering all kinds of nastiness going this untrodden path…

I think I'm fine now. I'll post an updated patch soon.