[PATCH 0/5] v4 Try to squash metadump data leaks

[PATCH 0/5] v4 Try to squash metadump data leaks

[Stefan Ring]

I reorganized the patches as suggested and fixed the btree case.

Stefan Ring (5):
  xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks
  xfs_metadump: Zap multi fsb blocks
  xfs_metadump: Zap freeindex blocks in directory inodes
  xfs_metadump: Zap unused space in inode btrees
  xfs_metadump: Zap dev inodes

 db/metadump.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 105 insertions(+), 14 deletions(-)

-- 
2.14.5

[PATCH 1/5] xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks

[Stefan Ring]

---
 db/metadump.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index cc2ae9af..97d2a490 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1426,7 +1426,7 @@ 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 +1435,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;
 	}
 }
 
-- 
2.14.5

[PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Stefan Ring]

Using the same code as in process_single_fsb_objects.
---
 db/metadump.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 97d2a490..c1bfc818 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1881,6 +1881,7 @@ process_multi_fsb_objects(
 	typnm_t		btype,
 	xfs_fileoff_t	last)
 {
+	char		*dp;
 	int		ret = 0;
 
 	switch (btype) {
@@ -1921,14 +1922,17 @@ 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) {
+				/* TODO, zap any stale data */
+				break;
+			} 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.5

[PATCH 3/5] xfs_metadump: Zap freeindex blocks in directory inodes

[Stefan Ring]

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

diff --git a/db/metadump.c b/db/metadump.c
index c1bfc818..a4867783 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1421,6 +1421,43 @@ 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)
@@ -1518,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;
 	}
 
@@ -1832,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 {
@@ -1924,8 +1960,7 @@ process_multi_fsb_objects(
 
 			dp = iocur_top->data;
 			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 {
-- 
2.14.5

[PATCH 4/5] xfs_metadump: Zap unused space in inode btrees

[Stefan Ring]

---
 db/metadump.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/db/metadump.c b/db/metadump.c
index a4867783..39183fb7 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2173,6 +2173,19 @@ process_btinode(
 	}
 
 	pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
+
+	if (zero_stale_data) {
+		char	*top;
+
+		/* Unused btree key space */
+		top = (char*)XFS_BMDR_KEY_ADDR(dib, nrecs + 1);
+		memset(top, 'a', (char*)pp - top);
+
+		/* Unused btree ptr space */
+		top = (char*)&pp[nrecs];
+		memset(top, 'b', (char*)dib + XFS_DFORK_SIZE(dip, mp, whichfork) - top);
+	}
+
 	for (i = 0; i < nrecs; i++) {
 		xfs_agnumber_t	ag;
 		xfs_agblock_t	bno;
-- 
2.14.5

Re: [PATCH 4/5] xfs_metadump: Zap unused space in inode btrees

[Stefan Ring]

On Fri, Oct 26, 2018 at 10:20 PM Stefan Ring <stefanrin@gmail.com> wrote:
>
> ---
>  db/metadump.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/db/metadump.c b/db/metadump.c
> index a4867783..39183fb7 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2173,6 +2173,19 @@ process_btinode(
>         }
>
>         pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
> +
> +       if (zero_stale_data) {
> +               char    *top;
> +
> +               /* Unused btree key space */
> +               top = (char*)XFS_BMDR_KEY_ADDR(dib, nrecs + 1);
> +               memset(top, 'a', (char*)pp - top);

Oh well. This should be 0 instead of 'a' and 'b'.

> +
> +               /* Unused btree ptr space */
> +               top = (char*)&pp[nrecs];
> +               memset(top, 'b', (char*)dib + XFS_DFORK_SIZE(dip, mp, whichfork) - top);
> +       }
> +
>         for (i = 0; i < nrecs; i++) {
>                 xfs_agnumber_t  ag;
>                 xfs_agblock_t   bno;
> --
> 2.14.5
>

[PATCH 5/5] xfs_metadump: Zap dev inodes

[Stefan Ring]

---
 db/metadump.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/db/metadump.c b/db/metadump.c
index 39183fb7..bdc6f2f4 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2269,6 +2269,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.
@@ -2321,7 +2339,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.5

Re: [PATCH 5/5] xfs_metadump: Zap dev inodes

[Eric Sandeen]

On 10/26/18 3:19 PM, Stefan Ring wrote:
> ---
>  db/metadump.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 39183fb7..bdc6f2f4 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2269,6 +2269,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.
> @@ -2321,7 +2339,9 @@ process_inode(
>  		case S_IFREG:
>  			success = process_inode_data(dip, TYP_DATA);
>  			break;
> -		default: ;
> +		default:
> +			success = process_dev_inode(dip);
> +			break;

Probably:
		case S_IFBLK:
		case S_IFCHR:
			success = process_dev_inode(dip);
			break;
		default:
			break;

no?

>  	}
>  	nametable_clear();
>  
> 

Re: [PATCH 5/5] xfs_metadump: Zap dev inodes

[Stefan Ring]

On Mon, Oct 29, 2018 at 4:38 PM Eric Sandeen <sandeen@sandeen.net> wrote:
> On 10/26/18 3:19 PM, Stefan Ring wrote:
> > ---
> >  db/metadump.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/db/metadump.c b/db/metadump.c
> > index 39183fb7..bdc6f2f4 100644
> > --- a/db/metadump.c
> > +++ b/db/metadump.c
> > @@ -2269,6 +2269,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.
> > @@ -2321,7 +2339,9 @@ process_inode(
> >               case S_IFREG:
> >                       success = process_inode_data(dip, TYP_DATA);
> >                       break;
> > -             default: ;
> > +             default:
> > +                     success = process_dev_inode(dip);
> > +                     break;
>
> Probably:
>                 case S_IFBLK:
>                 case S_IFCHR:
>                         success = process_dev_inode(dip);
>                         break;
>                 default:
>                         break;
>
> no?

Does this catch pipes and sockets, which are also dev inodes?

>
> >       }
> >       nametable_clear();
> >
> >

Re: [PATCH 5/5] xfs_metadump: Zap dev inodes

[Eric Sandeen]

On 10/29/18 1:33 PM, Stefan Ring wrote:
> On Mon, Oct 29, 2018 at 4:38 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>> On 10/26/18 3:19 PM, Stefan Ring wrote:
>>> ---
>>>  db/metadump.c | 22 +++++++++++++++++++++-
>>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/db/metadump.c b/db/metadump.c
>>> index 39183fb7..bdc6f2f4 100644
>>> --- a/db/metadump.c
>>> +++ b/db/metadump.c
>>> @@ -2269,6 +2269,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.
>>> @@ -2321,7 +2339,9 @@ process_inode(
>>>               case S_IFREG:
>>>                       success = process_inode_data(dip, TYP_DATA);
>>>                       break;
>>> -             default: ;
>>> +             default:
>>> +                     success = process_dev_inode(dip);
>>> +                     break;
>>
>> Probably:
>>                 case S_IFBLK:
>>                 case S_IFCHR:
>>                         success = process_dev_inode(dip);
>>                         break;
>>                 default:
>>                         break;
>>
>> no?
> 
> Does this catch pipes and sockets, which are also dev inodes?

heh, whooops, nope.

        case S_IFIFO:
        case S_IFCHR:
        case S_IFBLK:
        case S_IFSOCK:
...

sorry, my point was just that it's probably better to explicitly name them
than to fall through to a default but I forgot about 2 formats.

-Eric

>>
>>>       }
>>>       nametable_clear();
>>>
>>>
> 

Re: [PATCH 5/5] xfs_metadump: Zap dev inodes

[Stefan Ring]

On Mon, Oct 29, 2018 at 7:37 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> sorry, my point was just that it's probably better to explicitly name them
> than to fall through to a default but I forgot about 2 formats.

It's basically down to preference and a choice only you can make.
Either you forget one and do too little, or you forget one and do too
much. If this is the exhaustive list, I believe you ;).

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Eric Sandeen]

On 10/26/18 3:19 PM, Stefan Ring wrote:
> I reorganized the patches as suggested and fixed the btree case.
> 
> Stefan Ring (5):
>   xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks
>   xfs_metadump: Zap multi fsb blocks
>   xfs_metadump: Zap freeindex blocks in directory inodes
>   xfs_metadump: Zap unused space in inode btrees
>   xfs_metadump: Zap dev inodes
> 
>  db/metadump.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 105 insertions(+), 14 deletions(-)
> 


Cool, thanks for splitting these up, will review & test in earnest.

These also need your:

Signed-off-by: Stefan Ring <stefanrin@gmail.com>

But there may be a v5 yet, so stash that away for now :)

-Eric

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Eric Sandeen]

Also, here's an old script I had lying around to test metadump.  It's hacky, sorry.

Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path.
It freezes and unfreezes the test filesystem, if your mount fails it'll freeze
the fs you're on.  ;)  There may be other rough spots.

It also runs the xfs_metadump/xfs_db in your path; you could change that to a
local ./xfs_metadump to run db/xfs_db from a git tree instead for testing
w/o make install.

Right now this is detecting some corruption induced by metadump/mdrestore
with your full patchset in place, FWIW.

Sorry I didn't send this sooner, kinda forgot I had it.  really should turn it into an xfstest.

-----

#!/bin/bash

function _fail () {
	echo $1
	exit 1
}

FSSTRESS=/root/xfstests-dev/ltp/fsstress

mkdir -p mnt
umount mnt &>/dev/null

# Will fill fsfile.img with "cd cd cd"
echo "Patterning 256M image file"
xfs_io -F -f -c "pwrite 0 256m" fsfile.img &>/dev/null
# Make & label the filesystem, and mount it.
echo "mkfs & label the image, and mount it"
mkfs.xfs -b size=2048 -m crc=0 -L "fslabel" fsfile.img
mount -o loop fsfile.img mnt

cd mnt

# Attempt to make files of "every" format for data, dirs, attrs etc.

# ====== File Data ======

echo "Creating file types ..."
# Regular files
# - FMT_EXTENTS
touch S_IFREG.FMT_EXTENTS
xfs_io -c "pwrite 0 4k" S_IFREG.FMT_EXTENTS &>/dev/null
# - FMT_BTREE
touch S_IFREG.FMT_BTREE
for I in `seq 0 8 200`; do
	xfs_io -d -c "pwrite ${I}k 4k" S_IFREG.FMT_BTREE &>/dev/null
done

# ======= Directories =======
echo "Creating directory types ..."
# - FMT_LOCAL
mkdir S_IFDIR.FMT_LOCAL
touch S_IFDIR.FMT_LOCAL/localdirfile

# - FMT_EXTENTS
mkdir S_IFDIR.FMT_EXTENTS
for I in `seq 1 100`; do
	touch S_IFDIR.FMT_EXTENTS/extent_dir_file_$I
done
# With a few missing
for I in `seq 10 2 20` 100; do
	rm -f S_IFDIR.FMT_EXTENTS/extent_dir_file_$I
done

# - FMT_BTREE
mkdir S_IFDIR.FMT_BTREE
for I in `seq 1 1000`; do
	touch S_IFDIR.FMT_BTREE/btree_dir_file_$I
done
# With a few missing
for I in `seq 10 2 20` 1000; do
	rm -f S_IFDIR.FMT_BTREE/btree_dir_file_$I
done

# Dave's special hack - grow freespace tree
mkdir S_IFDIR.FMT_BTREE2
for I in `seq 1 5000`; do
	touch S_IFDIR.FMT_BTREE2/btree2_dir_file_$I
done
# Remove every other
for I in `seq 1 2 5000`; do
	rm -f S_IFDIR.FMT_BTREE2/btree2_dir_file_$I
done

# ======= Symlinks =======
echo "Creating symlink types ..."
# - FMT_LOCAL
ln -s target S_IFLNK.FMT_LOCAL
# - FMT_EXTENTS
# create "strangely_long_path_component/strangely_long_path_component/..."
COMP=strangely_long_path_component
TARGET=$COMP
for I in `seq 1 30`; do
	TARGET=$TARGET/$COMP
done
ln -s $TARGET S_IFLNK.FMT_EXTENTS

# ======= Char & block devices =======
echo "Creating char & block types ..."
mkdir S_IFDIR.DEVICES
mknod S_IFDIR.DEVICES/S_IFCHR c 1 1
mknod S_IFDIR.DEVICES/S_IFBLK c 1 1
# Create an inode with some local data & then remove
echo "Create local symlink"
touch S_IFDIR.DEVICES/longnamenamenamenamenamenamenamenamenamename
ln -s "longnamenamenamenamenamenamenamenamenamename" S_IFDIR.DEVICES/link
xfs_io -c fsync S_IFDIR.DEVICES/link
rm -f S_IFDIR.DEVICES/link
mknod S_IFDIR.DEVICES/S_IFBLK2 c 1 1

# ======= Attributes =======
echo "Creating attribute types ..."
# FMT_LOCAL
touch S_IFREG.ATTR.FMT_LOCAL
setfattr -n user.localattrname -v localattrvalue S_IFREG.ATTR.FMT_LOCAL
# FMT_EXTENTS
touch S_IFREG.ATTR.FMT_EXTENTS
for I in `seq 1 50`; do
	setfattr -n user.extentattrname$I -v extentattrvalue S_IFREG.ATTR.FMT_EXTENTS
done
# With a few missing
for I in 10 12 50; do
	setfattr -x user.extentattrname$I S_IFREG.ATTR.FMT_EXTENTS
done

# FMT_EXTENTS with a remote 3k value, fill with "C"
touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K
xfs_io -f -c "pwrite -S 0x43 0 3k" S_IFREG.ATTRVALFILE &>/dev/null
attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K < S_IFREG.ATTRVALFILE

# FMT_EXTENTS with a remote 4k value, fill with "D"
touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K
xfs_io -f -c "pwrite -S 0x44 0 4k" S_IFREG.ATTRVALFILE &>/dev/null
attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K < S_IFREG.ATTRVALFILE

# FMT_BTREE
touch S_IFREG.ATTR.FMT_BTREE
for I in `seq 1 1000`; do
	setfattr -n user.btreeattrname$I -v btreeattrlongervalue S_IFREG.ATTR.FMT_BTREE
done
# With a few missing
for I in 10 12 1000; do
	setfattr -x user.btreeattrname$I S_IFREG.ATTR.FMT_BTREE
done

# Make an unused inode
mkdir S_IFDIR.DELETED
touch S_IFDIR.DELETED/S_IFREG.DELETED
# Really push this to disk
xfs_freeze -f .
xfs_freeze -u .
rm -f S_IFDIR.DELETED/S_IFREG.DELETED

# =============================

# Now fsstress for some good randomness

mkdir stress
echo "fsstressing"
$FSSTRESS -d stress -p 4 -n 1000
echo "done"

sleep 5

cd -

echo "umount & remount"
umount mnt
mount -o loop fsfile.img mnt

# Get details of what's on disk in the original fs
echo "Get list of original files & attributes"
ls -lR mnt > orig_files
getfattr -m - -dR mnt > orig_attrs 2>/dev/null

echo "FS utilization:"
df mnt/

echo "umount"
umount mnt

# Test that we didn't lose anything with stale-data-zeroing
# turned on (i.e. zap too much)
# dump & restore it, repair it, and compare contents to orig

# ===== NON-OBFUSCATED =====
# do a NON-obfuscated metadump & look for stale pattern coming through

rm -f fsfile-clear.img
echo "Non-obfuscated metadump"
./xfs_metadump -o fsfile.img - | xfs_mdrestore - fsfile-clear.img
# Make sure it's not corrupt
echo "xfs_repair on unobfuscated & stale-zeroed metadump"
xfs_repair -n fsfile-clear.img 2>/dev/null || _fail "Repair failed on fsfile-clear.img"
# Get details of what's on disk in the image
mount -o loop fsfile-clear.img mnt
ls -lR mnt > clear_files
getfattr -m - -dR mnt > clear_attrs 2>/dev/null

echo "Checking for unchanged files & attrs via unobfuscated metadump"
diff -u orig_files clear_files
diff -u orig_attrs clear_attrs

echo "Looking for stale data in unobfuscated dump"
# Generic stale data test - look for original pattern
hexdump -C fsfile-clear.img | grep "cd cd cd"

# ===== OBFUSCATED =====
# Now do OBFUSCATED metadump & look for stale strings coming through,
# as well as looking for any other data we wrote
rm -f fsfile-obfuscated.img
xfs_metadump fsfile.img - | xfs_mdrestore - fsfile-obfuscated.img
# Make sure it's not corrupt
echo "xfs_repair on obfuscated & stale-zeroed metadump"
xfs_repair -n fsfile-obfuscated.img 2>/dev/null || _fail "Repair failed on fsfile-obfuscated.img"

# Generic stale data test - look for original pattern
echo "Looking for stale data in obfuscated dump"
hexdump -C fsfile-obfuscated.img | grep "cd cd cd"

# Look for stuff we explicitly wrote
echo "Looking for our data in obfuscated dump"
strings -t x fsfile-obfuscated.img | grep -i "S_IF\|attr\|name\|value\|btree\|long\|local\|extent\|label"

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Darrick J. Wong]

On Fri, Oct 26, 2018 at 06:33:21PM -0500, Eric Sandeen wrote:
> Also, here's an old script I had lying around to test metadump.  It's hacky, sorry.
> 
> Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path.
> It freezes and unfreezes the test filesystem, if your mount fails it'll freeze
> the fs you're on.  ;)  There may be other rough spots.
> 
> It also runs the xfs_metadump/xfs_db in your path; you could change that to a
> local ./xfs_metadump to run db/xfs_db from a git tree instead for testing
> w/o make install.
> 
> Right now this is detecting some corruption induced by metadump/mdrestore
> with your full patchset in place, FWIW.
> 
> Sorry I didn't send this sooner, kinda forgot I had it.  really should
> turn it into an xfstest.

I did, see xfs/349.

It doesn't test metadump tho... maybe it should?

(Also see xfs/432...)

--D

> 
> -----
> 
> #!/bin/bash
> 
> function _fail () {
> 	echo $1
> 	exit 1
> }
> 
> FSSTRESS=/root/xfstests-dev/ltp/fsstress
> 
> mkdir -p mnt
> umount mnt &>/dev/null
> 
> # Will fill fsfile.img with "cd cd cd"
> echo "Patterning 256M image file"
> xfs_io -F -f -c "pwrite 0 256m" fsfile.img &>/dev/null
> # Make & label the filesystem, and mount it.
> echo "mkfs & label the image, and mount it"
> mkfs.xfs -b size=2048 -m crc=0 -L "fslabel" fsfile.img
> mount -o loop fsfile.img mnt
> 
> cd mnt
> 
> # Attempt to make files of "every" format for data, dirs, attrs etc.
> 
> # ====== File Data ======
> 
> echo "Creating file types ..."
> # Regular files
> # - FMT_EXTENTS
> touch S_IFREG.FMT_EXTENTS
> xfs_io -c "pwrite 0 4k" S_IFREG.FMT_EXTENTS &>/dev/null
> # - FMT_BTREE
> touch S_IFREG.FMT_BTREE
> for I in `seq 0 8 200`; do
> 	xfs_io -d -c "pwrite ${I}k 4k" S_IFREG.FMT_BTREE &>/dev/null
> done
> 
> # ======= Directories =======
> echo "Creating directory types ..."
> # - FMT_LOCAL
> mkdir S_IFDIR.FMT_LOCAL
> touch S_IFDIR.FMT_LOCAL/localdirfile
> 
> # - FMT_EXTENTS
> mkdir S_IFDIR.FMT_EXTENTS
> for I in `seq 1 100`; do
> 	touch S_IFDIR.FMT_EXTENTS/extent_dir_file_$I
> done
> # With a few missing
> for I in `seq 10 2 20` 100; do
> 	rm -f S_IFDIR.FMT_EXTENTS/extent_dir_file_$I
> done
> 
> # - FMT_BTREE
> mkdir S_IFDIR.FMT_BTREE
> for I in `seq 1 1000`; do
> 	touch S_IFDIR.FMT_BTREE/btree_dir_file_$I
> done
> # With a few missing
> for I in `seq 10 2 20` 1000; do
> 	rm -f S_IFDIR.FMT_BTREE/btree_dir_file_$I
> done
> 
> # Dave's special hack - grow freespace tree
> mkdir S_IFDIR.FMT_BTREE2
> for I in `seq 1 5000`; do
> 	touch S_IFDIR.FMT_BTREE2/btree2_dir_file_$I
> done
> # Remove every other
> for I in `seq 1 2 5000`; do
> 	rm -f S_IFDIR.FMT_BTREE2/btree2_dir_file_$I
> done
> 
> # ======= Symlinks =======
> echo "Creating symlink types ..."
> # - FMT_LOCAL
> ln -s target S_IFLNK.FMT_LOCAL
> # - FMT_EXTENTS
> # create "strangely_long_path_component/strangely_long_path_component/..."
> COMP=strangely_long_path_component
> TARGET=$COMP
> for I in `seq 1 30`; do
> 	TARGET=$TARGET/$COMP
> done
> ln -s $TARGET S_IFLNK.FMT_EXTENTS
> 
> # ======= Char & block devices =======
> echo "Creating char & block types ..."
> mkdir S_IFDIR.DEVICES
> mknod S_IFDIR.DEVICES/S_IFCHR c 1 1
> mknod S_IFDIR.DEVICES/S_IFBLK c 1 1
> # Create an inode with some local data & then remove
> echo "Create local symlink"
> touch S_IFDIR.DEVICES/longnamenamenamenamenamenamenamenamenamename
> ln -s "longnamenamenamenamenamenamenamenamenamename" S_IFDIR.DEVICES/link
> xfs_io -c fsync S_IFDIR.DEVICES/link
> rm -f S_IFDIR.DEVICES/link
> mknod S_IFDIR.DEVICES/S_IFBLK2 c 1 1
> 
> # ======= Attributes =======
> echo "Creating attribute types ..."
> # FMT_LOCAL
> touch S_IFREG.ATTR.FMT_LOCAL
> setfattr -n user.localattrname -v localattrvalue S_IFREG.ATTR.FMT_LOCAL
> # FMT_EXTENTS
> touch S_IFREG.ATTR.FMT_EXTENTS
> for I in `seq 1 50`; do
> 	setfattr -n user.extentattrname$I -v extentattrvalue S_IFREG.ATTR.FMT_EXTENTS
> done
> # With a few missing
> for I in 10 12 50; do
> 	setfattr -x user.extentattrname$I S_IFREG.ATTR.FMT_EXTENTS
> done
> 
> # FMT_EXTENTS with a remote 3k value, fill with "C"
> touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K
> xfs_io -f -c "pwrite -S 0x43 0 3k" S_IFREG.ATTRVALFILE &>/dev/null
> attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K < S_IFREG.ATTRVALFILE
> 
> # FMT_EXTENTS with a remote 4k value, fill with "D"
> touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K
> xfs_io -f -c "pwrite -S 0x44 0 4k" S_IFREG.ATTRVALFILE &>/dev/null
> attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K < S_IFREG.ATTRVALFILE
> 
> # FMT_BTREE
> touch S_IFREG.ATTR.FMT_BTREE
> for I in `seq 1 1000`; do
> 	setfattr -n user.btreeattrname$I -v btreeattrlongervalue S_IFREG.ATTR.FMT_BTREE
> done
> # With a few missing
> for I in 10 12 1000; do
> 	setfattr -x user.btreeattrname$I S_IFREG.ATTR.FMT_BTREE
> done
> 
> # Make an unused inode
> mkdir S_IFDIR.DELETED
> touch S_IFDIR.DELETED/S_IFREG.DELETED
> # Really push this to disk
> xfs_freeze -f .
> xfs_freeze -u .
> rm -f S_IFDIR.DELETED/S_IFREG.DELETED
> 
> # =============================
> 
> # Now fsstress for some good randomness
> 
> mkdir stress
> echo "fsstressing"
> $FSSTRESS -d stress -p 4 -n 1000
> echo "done"
> 
> sleep 5
> 
> cd -
> 
> echo "umount & remount"
> umount mnt
> mount -o loop fsfile.img mnt
> 
> # Get details of what's on disk in the original fs
> echo "Get list of original files & attributes"
> ls -lR mnt > orig_files
> getfattr -m - -dR mnt > orig_attrs 2>/dev/null
> 
> echo "FS utilization:"
> df mnt/
> 
> echo "umount"
> umount mnt
> 
> # Test that we didn't lose anything with stale-data-zeroing
> # turned on (i.e. zap too much)
> # dump & restore it, repair it, and compare contents to orig
> 
> # ===== NON-OBFUSCATED =====
> # do a NON-obfuscated metadump & look for stale pattern coming through
> 
> rm -f fsfile-clear.img
> echo "Non-obfuscated metadump"
> ./xfs_metadump -o fsfile.img - | xfs_mdrestore - fsfile-clear.img
> # Make sure it's not corrupt
> echo "xfs_repair on unobfuscated & stale-zeroed metadump"
> xfs_repair -n fsfile-clear.img 2>/dev/null || _fail "Repair failed on fsfile-clear.img"
> # Get details of what's on disk in the image
> mount -o loop fsfile-clear.img mnt
> ls -lR mnt > clear_files
> getfattr -m - -dR mnt > clear_attrs 2>/dev/null
> 
> echo "Checking for unchanged files & attrs via unobfuscated metadump"
> diff -u orig_files clear_files
> diff -u orig_attrs clear_attrs
> 
> echo "Looking for stale data in unobfuscated dump"
> # Generic stale data test - look for original pattern
> hexdump -C fsfile-clear.img | grep "cd cd cd"
> 
> # ===== OBFUSCATED =====
> # Now do OBFUSCATED metadump & look for stale strings coming through,
> # as well as looking for any other data we wrote
> rm -f fsfile-obfuscated.img
> xfs_metadump fsfile.img - | xfs_mdrestore - fsfile-obfuscated.img
> # Make sure it's not corrupt
> echo "xfs_repair on obfuscated & stale-zeroed metadump"
> xfs_repair -n fsfile-obfuscated.img 2>/dev/null || _fail "Repair failed on fsfile-obfuscated.img"
> 
> # Generic stale data test - look for original pattern
> echo "Looking for stale data in obfuscated dump"
> hexdump -C fsfile-obfuscated.img | grep "cd cd cd"
> 
> # Look for stuff we explicitly wrote
> echo "Looking for our data in obfuscated dump"
> strings -t x fsfile-obfuscated.img | grep -i "S_IF\|attr\|name\|value\|btree\|long\|local\|extent\|label"
> 

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Stefan Ring]

On Sat, Oct 27, 2018 at 1:33 AM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> Also, here's an old script I had lying around to test metadump.  It's hacky, sorry.
>
> Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path.
> It freezes and unfreezes the test filesystem, if your mount fails it'll freeze
> the fs you're on.  ;)  There may be other rough spots.
>
> It also runs the xfs_metadump/xfs_db in your path; you could change that to a
> local ./xfs_metadump to run db/xfs_db from a git tree instead for testing
> w/o make install.
>
> Right now this is detecting some corruption induced by metadump/mdrestore
> with your full patchset in place, FWIW.
>
> Sorry I didn't send this sooner, kinda forgot I had it.  really should turn it into an xfstest.

Thanks! The corruption is caused by the last patch in the series. So
dev inodes can have attribute forks. I will have to zap the data and
the attr area separately.

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Stefan Ring]

On Sun, Oct 28, 2018 at 1:38 PM Stefan Ring <stefanrin@gmail.com> wrote:
>
> On Sat, Oct 27, 2018 at 1:33 AM Eric Sandeen <sandeen@sandeen.net> wrote:
> >
> > Also, here's an old script I had lying around to test metadump.  It's hacky, sorry.
> >
> > Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path.
> > It freezes and unfreezes the test filesystem, if your mount fails it'll freeze
> > the fs you're on.  ;)  There may be other rough spots.
> >
> > It also runs the xfs_metadump/xfs_db in your path; you could change that to a
> > local ./xfs_metadump to run db/xfs_db from a git tree instead for testing
> > w/o make install.
> >
> > Right now this is detecting some corruption induced by metadump/mdrestore
> > with your full patchset in place, FWIW.
> >
> > Sorry I didn't send this sooner, kinda forgot I had it.  really should turn it into an xfstest.
>
> Thanks! The corruption is caused by the last patch in the series. So
> dev inodes can have attribute forks. I will have to zap the data and
> the attr area separately.

Ok, this is a better replacement for the fifth patch (copy/pasted into
in-browser e-mail editor, might have broken formatting).

Apparently the dev's inode contents are xfs_dev_t, and the rest of the
data fork can be cleared. The attribute fork is handled later in
process_inode.

--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2270,6 +2270,25 @@ process_inode_data(
        return 1;
 }

+static int
+process_dev_inode(
+       xfs_dinode_t            *dip)
+{
+       if (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 {
+               if (zero_stale_data) {
+                       unsigned int size = sizeof(xfs_dev_t);
+                       memset(XFS_DFORK_DPTR(dip) + size, 0,
+                                       XFS_DFORK_DSIZE(dip, mp) - size);
+               }
+               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.
@@ -2322,7 +2341,10 @@ process_inode(
                case S_IFREG:
                        success = process_inode_data(dip, TYP_DATA);
                        break;
-               default: ;
+               default:
+                       success = process_dev_inode(dip);
+                       need_new_crc = 1;
+                       break;
        }
        nametable_clear();

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Stefan Ring]

On Sun, Oct 28, 2018 at 3:36 PM Stefan Ring <stefanrin@gmail.com> wrote:
>
> Ok, this is a better replacement for the fifth patch (copy/pasted into
> in-browser e-mail editor, might have broken formatting).

I've also put it up at
https://github.com/Ringdingcoder/xfsprogs-dev/commits/metadump-stale-data-v5-prelim
for now.

> Apparently the dev's inode contents are xfs_dev_t, and the rest of the
> data fork can be cleared. The attribute fork is handled later in
> process_inode.
>
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2270,6 +2270,25 @@ process_inode_data(
>         return 1;
>  }
>
> +static int
> +process_dev_inode(
> +       xfs_dinode_t            *dip)
> +{
> +       if (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 {
> +               if (zero_stale_data) {
> +                       unsigned int size = sizeof(xfs_dev_t);
> +                       memset(XFS_DFORK_DPTR(dip) + size, 0,
> +                                       XFS_DFORK_DSIZE(dip, mp) - size);
> +               }
> +               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.
> @@ -2322,7 +2341,10 @@ process_inode(
>                 case S_IFREG:
>                         success = process_inode_data(dip, TYP_DATA);
>                         break;
> -               default: ;
> +               default:
> +                       success = process_dev_inode(dip);
> +                       need_new_crc = 1;
> +                       break;
>         }
>         nametable_clear();

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Eric Sandeen]

On 10/28/18 7:38 AM, Stefan Ring wrote:
> On Sat, Oct 27, 2018 at 1:33 AM Eric Sandeen <sandeen@sandeen.net> wrote:
>>
>> Also, here's an old script I had lying around to test metadump.  It's hacky, sorry.
>>
>> Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path.
>> It freezes and unfreezes the test filesystem, if your mount fails it'll freeze
>> the fs you're on.  ;)  There may be other rough spots.
>>
>> It also runs the xfs_metadump/xfs_db in your path; you could change that to a
>> local ./xfs_metadump to run db/xfs_db from a git tree instead for testing
>> w/o make install.
>>
>> Right now this is detecting some corruption induced by metadump/mdrestore
>> with your full patchset in place, FWIW.
>>
>> Sorry I didn't send this sooner, kinda forgot I had it.  really should turn it into an xfstest.
> 
> Thanks! The corruption is caused by the last patch in the series. So
> dev inodes can have attribute forks. I will have to zap the data and
> the attr area separately.

Cool, glad it was helpful.

Check patch 2 as well, the "break;" is also causing corruption I think,
and it should probably just be a no-op ";"

It may get resolved by the time you get to patch5 but best to not have
regressions along the way.  Sorry for the piecemeal review here ;)

-Eric

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Stefan Ring]

On Sun, Oct 28, 2018 at 6:13 PM Eric Sandeen <sandeen@sandeen.net> wrote:
> Check patch 2 as well, the "break;" is also causing corruption I think,
> and it should probably just be a no-op ";"

Sorry about that. I think it's more than that. The single_fsb function
takes care to only write something out when there is something to
write and to only recalculate the crc when there has been a change.
I'd like to carry over this spirit, but I have the impression that in
the end the two functions would become so similar that they will just
merge into one.

> It may get resolved by the time you get to patch5 but best to not have
> regressions along the way.  Sorry for the piecemeal review here ;)

Re: [PATCH 0/5] v4 Try to squash metadump data leaks

[Eric Sandeen]

On 10/30/18 7:29 AM, Stefan Ring wrote:
> On Sun, Oct 28, 2018 at 6:13 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>> Check patch 2 as well, the "break;" is also causing corruption I think,
>> and it should probably just be a no-op ";"
> 
> Sorry about that. I think it's more than that. The single_fsb function
> takes care to only write something out when there is something to
> write and to only recalculate the crc when there has been a change.
> I'd like to carry over this spirit, but I have the impression that in
> the end the two functions would become so similar that they will just
> merge into one.

I think the "recalculate crc" flagging is a little inconsistent, tbh - 
or maybe I should say it consistently recalculates it more often than it
needs to?  It probably makes sense to follow the existing examples of
when it's set, and if there's any cleanup/optimization to do, that can
be done later.

-Eric