From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
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
From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
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;
From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
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;
On Fri, Mar 22, 2019 at 05:38:13PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <[email protected]>
>
> 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 <[email protected]>
Thanks, applied.
- Ted
On 3/22/19 7:37 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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;
>
>
On Sat, Mar 23, 2019 at 12:35 AM Darrick J. Wong
<[email protected]> wrote:
>
> From: Darrick J. Wong <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Filipe Manana <[email protected]>
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."
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
> <[email protected]> wrote:
> >
> > From: Darrick J. Wong <[email protected]>
> >
> > 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 <[email protected]>
>
> Reviewed-by: Filipe Manana <[email protected]>
>
> 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."
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
> > <[email protected]> wrote:
> > >
> > > From: Darrick J. Wong <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > Reviewed-by: Filipe Manana <[email protected]>
> >
> > 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."
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
> <[email protected]> wrote:
> >
> > From: Darrick J. Wong <[email protected]>
> >
> > 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 <[email protected]>
>
> Reviewed-by: Filipe Manana <[email protected]>
>
> Looks good, thanks for doing this. Just one question below.
Thanks for reviewing!
Eryu