[PATCH] generic: prohibit fstrim on journalled filesystems with norecovery

[PATCH] generic: prohibit fstrim on journalled filesystems with norecovery

[Darrick J. Wong]

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

This test makes sure that we can't use stale unrecovered fs metadata to
drive a DISCARD festival on a disk and thereby destroy user data by
accident.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/generic/714     |   61 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/714.out |    4 +++
 tests/generic/group   |    1 +
 3 files changed, 66 insertions(+)
 create mode 100755 tests/generic/714
 create mode 100644 tests/generic/714.out

diff --git a/tests/generic/714 b/tests/generic/714
new file mode 100755
index 00000000..1849a5e9
--- /dev/null
+++ b/tests/generic/714
@@ -0,0 +1,61 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019, Oracle and/or its affiliates.  All Rights Reserved.
+#
+# FS QA Test No. 714
+#
+# Ensure that we can't call fstrim on filesystems mounted norecovery, because
+# FSTRIM implementations use free space metadata to drive the discard requests
+# and we told the filesystem not to make sure the metadata are up to date.
+
+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 -rf $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_supported_os Linux
+_require_scratch
+_require_fstrim
+
+rm -f $seqres.full
+
+_scratch_mkfs > $seqres.full 2>&1
+_require_metadata_journaling $SCRATCH_DEV
+
+echo "fstrim on regular mount"
+_scratch_mount >> $seqres.full 2>&1
+$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 || \
+	_notrun "FSTRIM not supported"
+_scratch_unmount
+
+echo "fstrim on ro mount"
+_scratch_mount -o ro >> $seqres.full 2>&1
+$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1
+_scratch_unmount
+
+echo "fstrim on ro mount with no log replay"
+norecovery="norecovery"
+test $FSTYP = "btrfs" && norecovery=nologreplay
+_scratch_mount -o ro,$norecovery >> $seqres.full 2>&1
+$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 && \
+	echo "fstrim with unrecovered metadata just ate your filesystem"
+_scratch_unmount
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/714.out b/tests/generic/714.out
new file mode 100644
index 00000000..1158a2ff
--- /dev/null
+++ b/tests/generic/714.out
@@ -0,0 +1,4 @@
+QA output created by 714
+fstrim on regular mount
+fstrim on ro mount
+fstrim on ro mount with no log replay
diff --git a/tests/generic/group b/tests/generic/group
index 2e4341fb..c2046293 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -538,6 +538,7 @@
 533 auto quick attr
 534 auto quick log
 535 auto quick log
+714 auto trim
 940 auto quick clone punch
 941 auto quick clone punch
 942 auto quick clone punch

[PATCH] xfs: prohibit fstrim in norecovery mode

[Darrick J. Wong]

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

The xfs fstrim implementation uses the free space btrees to find free
space that can be discarded.  If we haven't recovered the log, the bnobt
will be stale and we absolutely *cannot* use stale metadata to zap the
underlying storage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_discard.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 93f07edafd81..9ee2a7d02e70 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -161,6 +161,14 @@ xfs_ioc_trim(
 		return -EPERM;
 	if (!blk_queue_discard(q))
 		return -EOPNOTSUPP;
+
+	/*
+	 * We haven't recovered the log, so we cannot use our bnobt-guided
+	 * storage zapping commands.
+	 */
+	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
+		return -EROFS;
+
 	if (copy_from_user(&range, urange, sizeof(range)))
 		return -EFAULT;
 

Re: [PATCH] xfs: prohibit fstrim in norecovery mode

[Eric Sandeen]

On 3/22/19 7:37 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The xfs fstrim implementation uses the free space btrees to find free
> space that can be discarded.  If we haven't recovered the log, the bnobt
> will be stale and we absolutely *cannot* use stale metadata to zap the
> underlying storage.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_discard.c |    8 ++++++++
>  1 file changed, 8 insertions(+)

Yikes...

Looks good to me (I briefly thought about a norecovery mount with a clean log,
but then decided I didn't care about that)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 93f07edafd81..9ee2a7d02e70 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -161,6 +161,14 @@ xfs_ioc_trim(
>  		return -EPERM;
>  	if (!blk_queue_discard(q))
>  		return -EOPNOTSUPP;
> +
> +	/*
> +	 * We haven't recovered the log, so we cannot use our bnobt-guided
> +	 * storage zapping commands.
> +	 */
> +	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
> +		return -EROFS;
> +
>  	if (copy_from_user(&range, urange, sizeof(range)))
>  		return -EFAULT;
>  
> 

[PATCH] ext4: prohibit fstrim in norecovery mode

[Darrick J. Wong]

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

The ext4 fstrim implementation uses the block bitmaps to find free space
that can be discarded.  If we haven't replayed the journal, the bitmaps
will be stale and we absolutely *cannot* use stale metadata to zap the
underlying storage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ext4/ioctl.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 3c4f8bb59f8a..bab3da4f1e0d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1000,6 +1000,13 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (!blk_queue_discard(q))
 			return -EOPNOTSUPP;
 
+		/*
+		 * We haven't replayed the journal, so we cannot use our
+		 * block-bitmap-guided storage zapping commands.
+		 */
+		if (test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb))
+			return -EROFS;
+
 		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
 		    sizeof(range)))
 			return -EFAULT;

Re: [PATCH] ext4: prohibit fstrim in norecovery mode

[Theodore Ts'o]

On Fri, Mar 22, 2019 at 05:38:13PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The ext4 fstrim implementation uses the block bitmaps to find free space
> that can be discarded.  If we haven't replayed the journal, the bitmaps
> will be stale and we absolutely *cannot* use stale metadata to zap the
> underlying storage.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

					- Ted

Re: [PATCH] generic: prohibit fstrim on journalled filesystems with norecovery

[Filipe David Manana]

On Sat, Mar 23, 2019 at 12:35 AM Darrick J. Wong
<darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> This test makes sure that we can't use stale unrecovered fs metadata to
> drive a DISCARD festival on a disk and thereby destroy user data by
> accident.

It would help to have listed the name of the patches that fix the
issues on xfs/ext4/btrfs, to make it faster.

>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks for doing this. Just one question below.

> ---
>  tests/generic/714     |   61 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/714.out |    4 +++
>  tests/generic/group   |    1 +
>  3 files changed, 66 insertions(+)
>  create mode 100755 tests/generic/714
>  create mode 100644 tests/generic/714.out
>
> diff --git a/tests/generic/714 b/tests/generic/714
> new file mode 100755
> index 00000000..1849a5e9
> --- /dev/null
> +++ b/tests/generic/714
> @@ -0,0 +1,61 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2019, Oracle and/or its affiliates.  All Rights Reserved.
> +#
> +# FS QA Test No. 714
> +#
> +# Ensure that we can't call fstrim on filesystems mounted norecovery, because
> +# FSTRIM implementations use free space metadata to drive the discard requests
> +# and we told the filesystem not to make sure the metadata are up to date.
> +
> +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 -rf $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_os Linux
> +_require_scratch
> +_require_fstrim
> +
> +rm -f $seqres.full
> +
> +_scratch_mkfs > $seqres.full 2>&1
> +_require_metadata_journaling $SCRATCH_DEV
> +
> +echo "fstrim on regular mount"
> +_scratch_mount >> $seqres.full 2>&1
> +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 || \
> +       _notrun "FSTRIM not supported"
> +_scratch_unmount
> +
> +echo "fstrim on ro mount"
> +_scratch_mount -o ro >> $seqres.full 2>&1
> +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1
> +_scratch_unmount
> +
> +echo "fstrim on ro mount with no log replay"
> +norecovery="norecovery"
> +test $FSTYP = "btrfs" && norecovery=nologreplay
> +_scratch_mount -o ro,$norecovery >> $seqres.full 2>&1
> +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 && \
> +       echo "fstrim with unrecovered metadata just ate your filesystem"
> +_scratch_unmount
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/714.out b/tests/generic/714.out
> new file mode 100644
> index 00000000..1158a2ff
> --- /dev/null
> +++ b/tests/generic/714.out
> @@ -0,0 +1,4 @@
> +QA output created by 714
> +fstrim on regular mount
> +fstrim on ro mount
> +fstrim on ro mount with no log replay
> diff --git a/tests/generic/group b/tests/generic/group
> index 2e4341fb..c2046293 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -538,6 +538,7 @@
>  533 auto quick attr
>  534 auto quick log
>  535 auto quick log
> +714 auto trim

Any reason to not add the 'quick' group as well? It runs in 1 second
for me on btrfs, xfs and ext4 with a debug kernel.

>  940 auto quick clone punch
>  941 auto quick clone punch
>  942 auto quick clone punch



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] generic: prohibit fstrim on journalled filesystems with norecovery

[Darrick J. Wong]

On Tue, Mar 26, 2019 at 12:21:20PM +0000, Filipe David Manana wrote:
> On Sat, Mar 23, 2019 at 12:35 AM Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > This test makes sure that we can't use stale unrecovered fs metadata to
> > drive a DISCARD festival on a disk and thereby destroy user data by
> > accident.
> 
> It would help to have listed the name of the patches that fix the
> issues on xfs/ext4/btrfs, to make it faster.
> 
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Looks good, thanks for doing this. Just one question below.
> 
> > ---
> >  tests/generic/714     |   61 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/714.out |    4 +++
> >  tests/generic/group   |    1 +
> >  3 files changed, 66 insertions(+)
> >  create mode 100755 tests/generic/714
> >  create mode 100644 tests/generic/714.out
> >
> > diff --git a/tests/generic/714 b/tests/generic/714
> > new file mode 100755
> > index 00000000..1849a5e9
> > --- /dev/null
> > +++ b/tests/generic/714
> > @@ -0,0 +1,61 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright (c) 2019, Oracle and/or its affiliates.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 714
> > +#
> > +# Ensure that we can't call fstrim on filesystems mounted norecovery, because
> > +# FSTRIM implementations use free space metadata to drive the discard requests
> > +# and we told the filesystem not to make sure the metadata are up to date.
> > +
> > +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 -rf $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_os Linux
> > +_require_scratch
> > +_require_fstrim
> > +
> > +rm -f $seqres.full
> > +
> > +_scratch_mkfs > $seqres.full 2>&1
> > +_require_metadata_journaling $SCRATCH_DEV
> > +
> > +echo "fstrim on regular mount"
> > +_scratch_mount >> $seqres.full 2>&1
> > +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 || \
> > +       _notrun "FSTRIM not supported"
> > +_scratch_unmount
> > +
> > +echo "fstrim on ro mount"
> > +_scratch_mount -o ro >> $seqres.full 2>&1
> > +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1
> > +_scratch_unmount
> > +
> > +echo "fstrim on ro mount with no log replay"
> > +norecovery="norecovery"
> > +test $FSTYP = "btrfs" && norecovery=nologreplay
> > +_scratch_mount -o ro,$norecovery >> $seqres.full 2>&1
> > +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 && \
> > +       echo "fstrim with unrecovered metadata just ate your filesystem"
> > +_scratch_unmount
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/714.out b/tests/generic/714.out
> > new file mode 100644
> > index 00000000..1158a2ff
> > --- /dev/null
> > +++ b/tests/generic/714.out
> > @@ -0,0 +1,4 @@
> > +QA output created by 714
> > +fstrim on regular mount
> > +fstrim on ro mount
> > +fstrim on ro mount with no log replay
> > diff --git a/tests/generic/group b/tests/generic/group
> > index 2e4341fb..c2046293 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -538,6 +538,7 @@
> >  533 auto quick attr
> >  534 auto quick log
> >  535 auto quick log
> > +714 auto trim
> 
> Any reason to not add the 'quick' group as well? It runs in 1 second
> for me on btrfs, xfs and ext4 with a debug kernel.

Probably just an oversight, I'll add it when I resubmit tomorrow.

Thanks for the review + btrfs fix. :)

--D

> >  940 auto quick clone punch
> >  941 auto quick clone punch
> >  942 auto quick clone punch
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."

Re: [PATCH] generic: prohibit fstrim on journalled filesystems with norecovery

[Eryu Guan]

On Tue, Mar 26, 2019 at 08:26:45PM -0700, Darrick J. Wong wrote:
> On Tue, Mar 26, 2019 at 12:21:20PM +0000, Filipe David Manana wrote:
> > On Sat, Mar 23, 2019 at 12:35 AM Darrick J. Wong
> > <darrick.wong@oracle.com> wrote:
> > >
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > This test makes sure that we can't use stale unrecovered fs metadata to
> > > drive a DISCARD festival on a disk and thereby destroy user data by
> > > accident.
> > 
> > It would help to have listed the name of the patches that fix the
> > issues on xfs/ext4/btrfs, to make it faster.
> > 
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> > 
> > Looks good, thanks for doing this. Just one question below.
> > 
> > > ---
> > >  tests/generic/714     |   61 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/714.out |    4 +++
> > >  tests/generic/group   |    1 +
> > >  3 files changed, 66 insertions(+)
> > >  create mode 100755 tests/generic/714
> > >  create mode 100644 tests/generic/714.out
> > >
> > > diff --git a/tests/generic/714 b/tests/generic/714
> > > new file mode 100755
> > > index 00000000..1849a5e9
> > > --- /dev/null
> > > +++ b/tests/generic/714
> > > @@ -0,0 +1,61 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +# Copyright (c) 2019, Oracle and/or its affiliates.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 714
> > > +#
> > > +# Ensure that we can't call fstrim on filesystems mounted norecovery, because
> > > +# FSTRIM implementations use free space metadata to drive the discard requests
> > > +# and we told the filesystem not to make sure the metadata are up to date.
> > > +
> > > +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 -rf $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +
> > > +# real QA test starts here
> > > +_supported_os Linux
> > > +_require_scratch
> > > +_require_fstrim
> > > +
> > > +rm -f $seqres.full
> > > +
> > > +_scratch_mkfs > $seqres.full 2>&1
> > > +_require_metadata_journaling $SCRATCH_DEV
> > > +
> > > +echo "fstrim on regular mount"
> > > +_scratch_mount >> $seqres.full 2>&1
> > > +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 || \
> > > +       _notrun "FSTRIM not supported"
> > > +_scratch_unmount
> > > +
> > > +echo "fstrim on ro mount"
> > > +_scratch_mount -o ro >> $seqres.full 2>&1
> > > +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1
> > > +_scratch_unmount
> > > +
> > > +echo "fstrim on ro mount with no log replay"
> > > +norecovery="norecovery"
> > > +test $FSTYP = "btrfs" && norecovery=nologreplay
> > > +_scratch_mount -o ro,$norecovery >> $seqres.full 2>&1
> > > +$FSTRIM_PROG -v $SCRATCH_MNT >> $seqres.full 2>&1 && \
> > > +       echo "fstrim with unrecovered metadata just ate your filesystem"
> > > +_scratch_unmount
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/714.out b/tests/generic/714.out
> > > new file mode 100644
> > > index 00000000..1158a2ff
> > > --- /dev/null
> > > +++ b/tests/generic/714.out
> > > @@ -0,0 +1,4 @@
> > > +QA output created by 714
> > > +fstrim on regular mount
> > > +fstrim on ro mount
> > > +fstrim on ro mount with no log replay
> > > diff --git a/tests/generic/group b/tests/generic/group
> > > index 2e4341fb..c2046293 100644
> > > --- a/tests/generic/group
> > > +++ b/tests/generic/group
> > > @@ -538,6 +538,7 @@
> > >  533 auto quick attr
> > >  534 auto quick log
> > >  535 auto quick log
> > > +714 auto trim
> > 
> > Any reason to not add the 'quick' group as well? It runs in 1 second
> > for me on btrfs, xfs and ext4 with a debug kernel.
> 
> Probably just an oversight, I'll add it when I resubmit tomorrow.

I'll just add it on commit.

> 
> Thanks for the review + btrfs fix. :)

Thanks again! :)

Eryu
> 
> --D
> 
> > >  940 auto quick clone punch
> > >  941 auto quick clone punch
> > >  942 auto quick clone punch
> > 
> > 
> > 
> > -- 
> > Filipe David Manana,
> > 
> > "Reasonable men adapt themselves to the world.
> >  Unreasonable men adapt the world to themselves.
> >  That's why all progress depends on unreasonable men."

Re: [PATCH] generic: prohibit fstrim on journalled filesystems with norecovery

[Eryu Guan]

On Tue, Mar 26, 2019 at 12:21:20PM +0000, Filipe David Manana wrote:
> On Sat, Mar 23, 2019 at 12:35 AM Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > This test makes sure that we can't use stale unrecovered fs metadata to
> > drive a DISCARD festival on a disk and thereby destroy user data by
> > accident.
> 
> It would help to have listed the name of the patches that fix the
> issues on xfs/ext4/btrfs, to make it faster.

Yeah, I'm about to do that :)

> 
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Looks good, thanks for doing this. Just one question below.

Thanks for reviewing!

Eryu