[PATCH] mkfs: validate extent size hint parameters

[PATCH] mkfs: validate extent size hint parameters

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Validate extent and cow extent size hints that are passed to mkfs so
that we avoid formatting a filesystem that will never mount.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/xfs_mkfs.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 46fd7c25..28d91253 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2222,6 +2222,66 @@ validate_rtextsize(
 	ASSERT(cfg->rtextblocks);
 }
 
+/* Validate the incoming extsize hint as if we were a file. */
+static void
+validate_extsize_hint(
+	struct xfs_mount	*mp,
+	struct cli_params	*cli)
+{
+	xfs_failaddr_t		fa;
+	bool			rt;
+	uint16_t		flags = 0;
+
+	rt = (cli->fsx.fsx_xflags & XFS_DIFLAG_RTINHERIT);
+
+	if (cli->fsx.fsx_xflags & XFS_DIFLAG_EXTSZINHERIT)
+		flags |= XFS_DIFLAG_EXTSIZE;
+
+	fa = libxfs_inode_validate_extsize(mp, cli->fsx.fsx_extsize, S_IFREG,
+			flags);
+	if (rt && fa == NULL)
+		fa = libxfs_inode_validate_extsize(mp, cli->fsx.fsx_extsize,
+				S_IFREG, flags | XFS_DIFLAG_REALTIME);
+	if (fa == NULL)
+		return;
+
+	if (rt && mp->m_sb.sb_rextsize > 1)
+		fprintf(stderr,
+	_("illegal extent size hint %lld, must be less than %u and a multiple of %u.\n"),
+				(long long)cli->fsx.fsx_extsize,
+				min(MAXEXTLEN, mp->m_sb.sb_agblocks / 2),
+				mp->m_sb.sb_rextsize);
+	else
+		fprintf(stderr,
+	_("illegal extent size hint %lld, must be less than %u.\n"),
+				(long long)cli->fsx.fsx_extsize,
+				min(MAXEXTLEN, mp->m_sb.sb_agblocks / 2));
+	usage();
+}
+
+/* Validate the incoming CoW extsize hint as if we were a file. */
+static void
+validate_cowextsize_hint(
+	struct xfs_mount	*mp,
+	struct cli_params	*cli)
+{
+	xfs_failaddr_t		fa;
+	uint64_t		flags2 = 0;
+
+	if (cli->fsx.fsx_xflags & FS_XFLAG_COWEXTSIZE)
+		flags2 |= XFS_DIFLAG2_COWEXTSIZE;
+	fa = libxfs_inode_validate_cowextsize(mp, cli->fsx.fsx_cowextsize,
+			S_IFREG, 0, flags2);
+	if (fa == NULL)
+		return;
+
+	fprintf(stderr,
+_("illegal CoW extent size hint %lld, must be less than %u.\n"),
+			(long long)cli->fsx.fsx_cowextsize,
+			min(MAXEXTLEN, mp->m_sb.sb_agblocks / 2));
+	usage();
+}
+
 /*
  * Validate the configured stripe geometry, or is none is specified, pull
  * the configuration from the underlying device.
@@ -3982,6 +4042,10 @@ main(
 
 	finish_superblock_setup(&cfg, mp, sbp);
 
+	/* Validate the extent size hints now that @mp is fully set up. */
+	validate_extsize_hint(mp, &cli);
+	validate_cowextsize_hint(mp, &cli);
+
 	/* Print the intended geometry of the fs. */
 	if (!quiet || dry_run) {
 		struct xfs_fsop_geom	geo;

[PATCH] xfs: test mkfs extent size hint validation

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure mkfs won't format filesystems that fail extent size hint
validation.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/xfs/738     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/738.out |    2 ++
 tests/xfs/group   |    1 +
 3 files changed, 51 insertions(+)
 create mode 100755 tests/xfs/738
 create mode 100644 tests/xfs/738.out

diff --git a/tests/xfs/738 b/tests/xfs/738
new file mode 100755
index 00000000..ee0ffaae
--- /dev/null
+++ b/tests/xfs/738
@@ -0,0 +1,48 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 738
+#
+# Make sure we can't format a filesystem with insane extent hints.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+tmp=/tmp/$$
+status=1	# failure is the default!
+testfile=$TEST_DIR/$seq.txt
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+
+# real QA test starts here
+_supported_fs xfs
+_supported_os Linux
+_require_scratch_nocheck
+
+rm -f $seqres.full
+
+# Try regular extent size hint
+echo extszinherit >> $seqres.full
+_scratch_mkfs -d extszinherit=8388608 >> $seqres.full 2>&1
+test $? -eq 1 || echo "should have failed extszinherit"
+
+# Try cow extent size hint if supported
+$MKFS_XFS_PROG -N -f $SCRATCH_DEV > $tmp.mkfs
+if grep -q 'reflink=' $tmp.mkfs ; then
+	echo cowextsize >> $seqres.full
+	_scratch_mkfs -m reflink=1 -d cowextsize=8388608 >> $seqres.full 2>&1
+	test $? -eq 1 || echo "should have failed cowextsize"
+fi
+
+echo silence is golden
+status=0
+exit
diff --git a/tests/xfs/738.out b/tests/xfs/738.out
new file mode 100644
index 00000000..703fe23b
--- /dev/null
+++ b/tests/xfs/738.out
@@ -0,0 +1,2 @@
+QA output created by 738
+silence is golden
diff --git a/tests/xfs/group b/tests/xfs/group
index 677396fb..b2fea9ef 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -515,6 +515,7 @@
 731 dangerous_scrub dangerous_online_repair
 736 auto quick unlink
 737 auto quick unlink
+738 auto quick
 1500 auto prealloc rw
 1501 auto prealloc rw
 1502 auto prealloc rw

Re: [PATCH] xfs: test mkfs extent size hint validation

[Dave Chinner]

On Fri, Feb 15, 2019 at 04:41:19PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure mkfs won't format filesystems that fail extent size hint
> validation.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  tests/xfs/738     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/738.out |    2 ++
>  tests/xfs/group   |    1 +
>  3 files changed, 51 insertions(+)
>  create mode 100755 tests/xfs/738
>  create mode 100644 tests/xfs/738.out
> 
> diff --git a/tests/xfs/738 b/tests/xfs/738
> new file mode 100755
> index 00000000..ee0ffaae
> --- /dev/null
> +++ b/tests/xfs/738
> @@ -0,0 +1,48 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
> +#
> +# FS QA Test No. 738
> +#
> +# Make sure we can't format a filesystem with insane extent hints.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +testfile=$TEST_DIR/$seq.txt
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_supported_os Linux
> +_require_scratch_nocheck
> +
> +rm -f $seqres.full
> +
> +# Try regular extent size hint
> +echo extszinherit >> $seqres.full
> +_scratch_mkfs -d extszinherit=8388608 >> $seqres.full 2>&1
> +test $? -eq 1 || echo "should have failed extszinherit"

That won't fail on 64k block size filesystems....

> +# Try cow extent size hint if supported
> +$MKFS_XFS_PROG -N -f $SCRATCH_DEV > $tmp.mkfs
> +if grep -q 'reflink=' $tmp.mkfs ; then

That's testing mkfs for reflink support, yes? i.e.

_scratch_mkfs_xfs_supported -m crc=1,reflink=1 

[ need to explicitly say -m crc=1 here, because MKFS_OPTIONS might
have -m crc=0, right? ]

> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -515,6 +515,7 @@
>  731 dangerous_scrub dangerous_online_repair
>  736 auto quick unlink
>  737 auto quick unlink
> +738 auto quick

prealloc, too. And maybe mkfs?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: test mkfs extent size hint validation

[Darrick J. Wong]

On Mon, Feb 18, 2019 at 09:07:16AM +1100, Dave Chinner wrote:
> On Fri, Feb 15, 2019 at 04:41:19PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Make sure mkfs won't format filesystems that fail extent size hint
> > validation.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  tests/xfs/738     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/738.out |    2 ++
> >  tests/xfs/group   |    1 +
> >  3 files changed, 51 insertions(+)
> >  create mode 100755 tests/xfs/738
> >  create mode 100644 tests/xfs/738.out
> > 
> > diff --git a/tests/xfs/738 b/tests/xfs/738
> > new file mode 100755
> > index 00000000..ee0ffaae
> > --- /dev/null
> > +++ b/tests/xfs/738
> > @@ -0,0 +1,48 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 738
> > +#
> > +# Make sure we can't format a filesystem with insane extent hints.
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +testfile=$TEST_DIR/$seq.txt
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +
> > +# real QA test starts here
> > +_supported_fs xfs
> > +_supported_os Linux
> > +_require_scratch_nocheck
> > +
> > +rm -f $seqres.full
> > +
> > +# Try regular extent size hint
> > +echo extszinherit >> $seqres.full
> > +_scratch_mkfs -d extszinherit=8388608 >> $seqres.full 2>&1
> > +test $? -eq 1 || echo "should have failed extszinherit"
> 
> That won't fail on 64k block size filesystems....

The extzinherit parameter is copied directly into the root inode without
any unit conversion (dopts[D_EXTSZINHERIT].convert == false), which
means that it's specified in units of fs blocks, right?  So then the
only constraints are that the value < MAXEXTLEN and value < agsize/2,
which means that block size shouldn't matter.

> > +# Try cow extent size hint if supported
> > +$MKFS_XFS_PROG -N -f $SCRATCH_DEV > $tmp.mkfs
> > +if grep -q 'reflink=' $tmp.mkfs ; then
> 
> That's testing mkfs for reflink support, yes? i.e.
> 
> _scratch_mkfs_xfs_supported -m crc=1,reflink=1 

Ok.

> [ need to explicitly say -m crc=1 here, because MKFS_OPTIONS might
> have -m crc=0, right? ]
> 
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -515,6 +515,7 @@
> >  731 dangerous_scrub dangerous_online_repair
> >  736 auto quick unlink
> >  737 auto quick unlink
> > +738 auto quick
> 
> prealloc, too. And maybe mkfs?

Will fix the last two; thanks for the review.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: test mkfs extent size hint validation

[Dave Chinner]

On Mon, Feb 18, 2019 at 10:03:34AM -0800, Darrick J. Wong wrote:
> On Mon, Feb 18, 2019 at 09:07:16AM +1100, Dave Chinner wrote:
> > On Fri, Feb 15, 2019 at 04:41:19PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Make sure mkfs won't format filesystems that fail extent size hint
> > > validation.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  tests/xfs/738     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/xfs/738.out |    2 ++
> > >  tests/xfs/group   |    1 +
> > >  3 files changed, 51 insertions(+)
> > >  create mode 100755 tests/xfs/738
> > >  create mode 100644 tests/xfs/738.out
> > > 
> > > diff --git a/tests/xfs/738 b/tests/xfs/738
> > > new file mode 100755
> > > index 00000000..ee0ffaae
> > > --- /dev/null
> > > +++ b/tests/xfs/738
> > > @@ -0,0 +1,48 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 738
> > > +#
> > > +# Make sure we can't format a filesystem with insane extent hints.
> > > +#
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +tmp=/tmp/$$
> > > +status=1	# failure is the default!
> > > +testfile=$TEST_DIR/$seq.txt
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +
> > > +# real QA test starts here
> > > +_supported_fs xfs
> > > +_supported_os Linux
> > > +_require_scratch_nocheck
> > > +
> > > +rm -f $seqres.full
> > > +
> > > +# Try regular extent size hint
> > > +echo extszinherit >> $seqres.full
> > > +_scratch_mkfs -d extszinherit=8388608 >> $seqres.full 2>&1
> > > +test $? -eq 1 || echo "should have failed extszinherit"
> > 
> > That won't fail on 64k block size filesystems....
> 
> The extzinherit parameter is copied directly into the root inode without
> any unit conversion (dopts[D_EXTSZINHERIT].convert == false), which
> means that it's specified in units of fs blocks, right?  So then the
> only constraints are that the value < MAXEXTLEN and value < agsize/2,
> which means that block size shouldn't matter.

Ah, good point. I read it as 8GB, not 8M blocks.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com