v4 Try to squash metadump data leaks

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

db/metadump.c +0 -0

index a4867783..39183fb7 100644

db/metadump.c +6 -1

+
+               /* 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

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

db/metadump.c +18 -0

index 39183fb7..bdc6f2f4 100644
@@ -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.

db/metadump.c +0 -0

 	}
 	nametable_clear();
 

Quoted 34 lines · Stefan Ring

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

db/metadump.c +18 -0

index 39183fb7..bdc6f2f4 100644
@@ -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.

db/metadump.c process_inode( · line 2344

Quoted 7 lines

db/metadump.c +3 -1

@@ -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?

Quoted 5 lines

db/metadump.c +0 -0

      }
      nametable_clear();
 

Quoted 51 lines · Eric Sandeen

Quoted 34 lines · Stefan Ring

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

db/metadump.c +18 -0

index 39183fb7..bdc6f2f4 100644
@@ -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.

db/metadump.c process_inode( · line 2344

Quoted 7 lines

db/metadump.c +3 -1

@@ -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;

Quoted 1 line

no?

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

Quoted 5 lines

Quoted 5 lines

db/metadump.c +0 -0

      }
      nametable_clear();
 

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.

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(-)

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"

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.

Quoted 11 lines · 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.

Quoted 4 lines

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.

db/metadump.c +23 -1

@@ -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();

Quoted 11 lines · 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.

Quoted 4 lines

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.

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 ;)

Quoted 2 lines · Eric Sandeen

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.