[PATCH] xfs: mount with corrupted inopblock in superblock

[PATCH] xfs: mount with corrupted inopblock in superblock

[Eryu Guan]

When sb_inopblock is corrupted (out of boundary in this case), XFS
should not crash and refuse to mount.

Kernel commit 392c6de98af1 ("xfs: sanitize sb_inopblock in
xfs_mount_validate_sb") addresses this issue.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 tests/xfs/118     | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/118.out |  2 ++
 tests/xfs/group   |  1 +
 3 files changed, 69 insertions(+)
 create mode 100755 tests/xfs/118
 create mode 100644 tests/xfs/118.out

diff --git a/tests/xfs/118 b/tests/xfs/118
new file mode 100755
index 0000000..680add8
--- /dev/null
+++ b/tests/xfs/118
@@ -0,0 +1,66 @@
+#! /bin/bash
+# FS QA Test 118
+#
+# Mount an XFS with out of bounds inopblock
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2016 Red Hat Inc.,  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs xfs
+_supported_os IRIX Linux
+# we corrupt scratch dev on purpose
+_require_scratch_nocheck
+
+_scratch_mkfs >>$seqres.full
+
+# corrupt sb_inopblock
+_scratch_xfs_db -x -c "sb 0" -c "write -c inopblock 512" >>$seqres.full
+
+# mount corrupted fs
+_scratch_mount >>$seqres.full 2>&1
+
+# no crash, and SCRATCH_DEV should not be mounted either
+_is_mounted $SCRATCH_DEV
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/xfs/118.out b/tests/xfs/118.out
new file mode 100644
index 0000000..3daed86
--- /dev/null
+++ b/tests/xfs/118.out
@@ -0,0 +1,2 @@
+QA output created by 118
+Silence is golden
diff --git a/tests/xfs/group b/tests/xfs/group
index 3296eb9..3bfcb80 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -115,6 +115,7 @@
 115 parent attr
 116 quota auto quick
 117 fuzzers
+118 auto quick metadata
 119 log v2log auto freeze dangerous
 120 fuzzers
 121 log auto quick
-- 
2.7.4

Re: [PATCH] xfs: mount with corrupted inopblock in superblock

[Dave Chinner]

On Thu, Nov 03, 2016 at 05:52:00PM +0800, Eryu Guan wrote:
> When sb_inopblock is corrupted (out of boundary in this case), XFS
> should not crash and refuse to mount.
> 
> Kernel commit 392c6de98af1 ("xfs: sanitize sb_inopblock in
> xfs_mount_validate_sb") addresses this issue.

Ok, seems like something addressed by the fuzzing tests, but we can
ignore that for now.

> +_scratch_mkfs >>$seqres.full
> +
> +# corrupt sb_inopblock
> +_scratch_xfs_db -x -c "sb 0" -c "write -c inopblock 512" >>$seqres.full
> +
> +# mount corrupted fs
> +_scratch_mount >>$seqres.full 2>&1
> +
> +# no crash, and SCRATCH_DEV should not be mounted either
> +_is_mounted $SCRATCH_DEV

Rather than test that we caught /one/ corrupt field, why not
loop here corrupting each superblock field in turn and checking
that the corruption is caught properly?

i.e. shouldn't we be proactively testing that we're catching all the
obvious corruptions, rather than just testing the out-of-bounds
issues that we've already found and fixed?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: mount with corrupted inopblock in superblock

[Darrick J. Wong]

On Fri, Nov 04, 2016 at 11:54:34AM +1100, Dave Chinner wrote:
> On Thu, Nov 03, 2016 at 05:52:00PM +0800, Eryu Guan wrote:
> > When sb_inopblock is corrupted (out of boundary in this case), XFS
> > should not crash and refuse to mount.
> > 
> > Kernel commit 392c6de98af1 ("xfs: sanitize sb_inopblock in
> > xfs_mount_validate_sb") addresses this issue.
> 
> Ok, seems like something addressed by the fuzzing tests, but we can
> ignore that for now.
> 
> > +_scratch_mkfs >>$seqres.full
> > +
> > +# corrupt sb_inopblock
> > +_scratch_xfs_db -x -c "sb 0" -c "write -c inopblock 512" >>$seqres.full
> > +
> > +# mount corrupted fs
> > +_scratch_mount >>$seqres.full 2>&1
> > +
> > +# no crash, and SCRATCH_DEV should not be mounted either
> > +_is_mounted $SCRATCH_DEV
> 
> Rather than test that we caught /one/ corrupt field, why not
> loop here corrupting each superblock field in turn and checking
> that the corruption is caught properly?
> 
> i.e. shouldn't we be proactively testing that we're catching all the
> obvious corruptions, rather than just testing the out-of-bounds
> issues that we've already found and fixed?

I've been working on xfstests that do exactly that for a while now and
am getting ready to do a big patch dump of online scrub/repair + more
dangerous_fuzzers tests to see how well the recovery tools work.

In the meantime I doubt it hurts to have this regression test while I
get my act together. :)

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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