diff mbox series

[05/11] xfs: Check for extent overflow when adding/removing xattrs

Message ID 20201113112704.28798-6-chandanrlinux@gmail.com (mailing list archive)
State Superseded, archived
Headers show
Series xfs: Tests to check for inode fork extent count overflow detection | expand

Commit Message

Chandan Babu R Nov. 13, 2020, 11:26 a.m. UTC 
This test verifies that XFS does not cause inode fork's extent count to
overflow when adding/removing xattrs.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 tests/xfs/525     | 154 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/525.out |  16 +++++
 tests/xfs/group   |   1 +
 3 files changed, 171 insertions(+)
 create mode 100755 tests/xfs/525
 create mode 100644 tests/xfs/525.out

Comments

Darrick J. Wong Nov. 14, 2020, 12:34 a.m. UTC | #1 
On Fri, Nov 13, 2020 at 04:56:57PM +0530, Chandan Babu R wrote:
> This test verifies that XFS does not cause inode fork's extent count to
> overflow when adding/removing xattrs.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  tests/xfs/525     | 154 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/525.out |  16 +++++
>  tests/xfs/group   |   1 +
>  3 files changed, 171 insertions(+)
>  create mode 100755 tests/xfs/525
>  create mode 100644 tests/xfs/525.out
> 
> diff --git a/tests/xfs/525 b/tests/xfs/525
> new file mode 100755
> index 00000000..1d5d6e7c
> --- /dev/null
> +++ b/tests/xfs/525
> @@ -0,0 +1,154 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Chandan Babu R.  All Rights Reserved.
> +#
> +# FS QA Test 525
> +#
> +# Verify that XFS does not cause inode fork's extent count to overflow when
> +# Adding/removing xattrs.
> +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
> +. ./common/attr
> +. ./common/inject
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs xfs
> +_require_scratch
> +_require_attrs
> +_require_xfs_debug
> +_require_test_program "punch-alternating"
> +_require_xfs_io_error_injection "reduce_max_iextents"
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +
> +attr_set()
> +{
> +	echo "* Set xattrs"
> +
> +	echo "Format and mount fs"
> +	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
> +	_scratch_mount >> $seqres.full
> +
> +	bsize=$(_get_block_size $SCRATCH_MNT)
> +
> +	testfile=$SCRATCH_MNT/testfile
> +
> +	echo "Consume free space"
> +	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
> +	sync
> +
> +	echo "Create fragmented filesystem"
> +	$here/src/punch-alternating $testfile >> $seqres.full
> +	sync
> +
> +	echo "Inject reduce_max_iextents error tag"
> +	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
> +
> +	echo "Inject bmap_alloc_minlen_extent error tag"
> +	xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
> +
> +	echo "Create xattrs"
> +
> +	attr_len=$(uuidgen | wc -c)
> +	nr_attrs=$((bsize * 20 / attr_len))
> +	for i in $(seq 1 $nr_attrs); do
> +		$SETFATTR_PROG -n "trusted.""$(uuidgen)" $testfile \

Does this test require UUIDs in the attr names?  Why wouldn't
$(printf "%037d" $i) suffice for this purpose?

Though if you insist upon using uuids, please call $UUIDGEN_PROG per
fstest custom.

> +			 >> $seqres.full 2>&1
> +		[[ $? != 0 ]] && break
> +	done
> +
> +	testino=$(stat -c "%i" $testfile)
> +
> +	_scratch_unmount >> $seqres.full
> +
> +	echo "Verify uquota inode's extent count"

Huh?  I thought we were testing file attrs?

> +	nextents=$(_scratch_get_iext_count $testino attr || \
> +			_fail "Unable to obtain inode fork's extent count")
> +	if (( $nextents > 10 )); then
> +		echo "Extent count overflow check failed: nextents = $nextents"
> +		exit 1
> +	fi
> +}
> +
> +attr_remove()
> +{
> +	echo "* Remove xattrs"
> +
> +	echo "Format and mount fs"
> +	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
> +	_scratch_mount >> $seqres.full
> +
> +	bsize=$(_get_block_size $SCRATCH_MNT)
> +
> +	testfile=$SCRATCH_MNT/testfile
> +
> +	echo "Consume free space"
> +	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
> +	sync
> +
> +	echo "Create fragmented filesystem"
> +	$here/src/punch-alternating $testfile >> $seqres.full
> +	sync
> +
> +	testino=$(stat -c "%i" $testfile)
> +
> +	naextents=0
> +	last=""
> +
> +	attr_len=$(uuidgen | wc -c)
> +	nr_attrs=$((bsize / attr_len))
> +
> +	echo "Create initial xattr extents"
> +	while (( $naextents < 4 )); do
> +		xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
> +
> +		for i in $(seq 1 $nr_attrs); do
> +			last="trusted.""$(uuidgen)"
> +			$SETFATTR_PROG -n $last $testfile
> +		done
> +
> +		_scratch_unmount >> $seqres.full
> +
> +		naextents=$(_scratch_get_iext_count $testino attr || \
> +				_fail "Unable to obtain inode fork's extent count")
> +
> +		_scratch_mount >> $seqres.full
> +	done
> +
> +	echo "Inject reduce_max_iextents error tag"
> +	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
> +
> +	echo "Remove xattr to trigger -EFBIG"
> +	$SETFATTR_PROG -x "$last" $testfile >> $seqres.full 2>&1
> +	if [[ $? == 0 ]]; then
> +		echo "Xattr removal succeeded; Should have failed "

So at this point the user has a file for which he can't ever remove the
xattrs for fear of overflowing naextents.  The only way to clear this is
to delete the file, so shouldn't you be testing that this succeeds?

--D

> +		exit 1
> +	fi
> +}
> +
> +attr_set
> +attr_remove
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/525.out b/tests/xfs/525.out
> new file mode 100644
> index 00000000..cc40e6e2
> --- /dev/null
> +++ b/tests/xfs/525.out
> @@ -0,0 +1,16 @@
> +QA output created by 525
> +* Set xattrs
> +Format and mount fs
> +Consume free space
> +Create fragmented filesystem
> +Inject reduce_max_iextents error tag
> +Inject bmap_alloc_minlen_extent error tag
> +Create xattrs
> +Verify uquota inode's extent count
> +* Remove xattrs
> +Format and mount fs
> +Consume free space
> +Create fragmented filesystem
> +Create initial xattr extents
> +Inject reduce_max_iextents error tag
> +Remove xattr to trigger -EFBIG
> diff --git a/tests/xfs/group b/tests/xfs/group
> index 3fa38c36..bd38aff0 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -522,3 +522,4 @@
>  522 auto quick quota
>  523 auto quick realtime growfs
>  524 auto quick punch zero insert collapse
> +525 auto quick attr
> -- 
> 2.28.0
>
Chandan Babu R Nov. 17, 2020, 2:30 p.m. UTC | #2 
On Saturday 14 November 2020 6:04:40 AM IST Darrick J. Wong wrote:
> On Fri, Nov 13, 2020 at 04:56:57PM +0530, Chandan Babu R wrote:
> > This test verifies that XFS does not cause inode fork's extent count to
> > overflow when adding/removing xattrs.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  tests/xfs/525     | 154 ++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/525.out |  16 +++++
> >  tests/xfs/group   |   1 +
> >  3 files changed, 171 insertions(+)
> >  create mode 100755 tests/xfs/525
> >  create mode 100644 tests/xfs/525.out
> > 
> > diff --git a/tests/xfs/525 b/tests/xfs/525
> > new file mode 100755
> > index 00000000..1d5d6e7c
> > --- /dev/null
> > +++ b/tests/xfs/525
> > @@ -0,0 +1,154 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2020 Chandan Babu R.  All Rights Reserved.
> > +#
> > +# FS QA Test 525
> > +#
> > +# Verify that XFS does not cause inode fork's extent count to overflow when
> > +# Adding/removing xattrs.
> > +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
> > +. ./common/attr
> > +. ./common/inject
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +
> > +_supported_fs xfs
> > +_require_scratch
> > +_require_attrs
> > +_require_xfs_debug
> > +_require_test_program "punch-alternating"
> > +_require_xfs_io_error_injection "reduce_max_iextents"
> > +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> > +
> > +attr_set()
> > +{
> > +	echo "* Set xattrs"
> > +
> > +	echo "Format and mount fs"
> > +	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
> > +	_scratch_mount >> $seqres.full
> > +
> > +	bsize=$(_get_block_size $SCRATCH_MNT)
> > +
> > +	testfile=$SCRATCH_MNT/testfile
> > +
> > +	echo "Consume free space"
> > +	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
> > +	sync
> > +
> > +	echo "Create fragmented filesystem"
> > +	$here/src/punch-alternating $testfile >> $seqres.full
> > +	sync
> > +
> > +	echo "Inject reduce_max_iextents error tag"
> > +	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
> > +
> > +	echo "Inject bmap_alloc_minlen_extent error tag"
> > +	xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
> > +
> > +	echo "Create xattrs"
> > +
> > +	attr_len=$(uuidgen | wc -c)
> > +	nr_attrs=$((bsize * 20 / attr_len))
> > +	for i in $(seq 1 $nr_attrs); do
> > +		$SETFATTR_PROG -n "trusted.""$(uuidgen)" $testfile \
> 
> Does this test require UUIDs in the attr names?  Why wouldn't
> $(printf "%037d" $i) suffice for this purpose?

You are right. I can replace executing uuidgen with calls to printf.

> 
> Though if you insist upon using uuids, please call $UUIDGEN_PROG per
> fstest custom.
> 
> > +			 >> $seqres.full 2>&1
> > +		[[ $? != 0 ]] && break
> > +	done
> > +
> > +	testino=$(stat -c "%i" $testfile)
> > +
> > +	_scratch_unmount >> $seqres.full
> > +
> > +	echo "Verify uquota inode's extent count"
> 
> Huh?  I thought we were testing file attrs?

Sorry, I will fix that.

> 
> > +	nextents=$(_scratch_get_iext_count $testino attr || \
> > +			_fail "Unable to obtain inode fork's extent count")
> > +	if (( $nextents > 10 )); then
> > +		echo "Extent count overflow check failed: nextents = $nextents"
> > +		exit 1
> > +	fi
> > +}
> > +
> > +attr_remove()
> > +{
> > +	echo "* Remove xattrs"
> > +
> > +	echo "Format and mount fs"
> > +	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
> > +	_scratch_mount >> $seqres.full
> > +
> > +	bsize=$(_get_block_size $SCRATCH_MNT)
> > +
> > +	testfile=$SCRATCH_MNT/testfile
> > +
> > +	echo "Consume free space"
> > +	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
> > +	sync
> > +
> > +	echo "Create fragmented filesystem"
> > +	$here/src/punch-alternating $testfile >> $seqres.full
> > +	sync
> > +
> > +	testino=$(stat -c "%i" $testfile)
> > +
> > +	naextents=0
> > +	last=""
> > +
> > +	attr_len=$(uuidgen | wc -c)
> > +	nr_attrs=$((bsize / attr_len))
> > +
> > +	echo "Create initial xattr extents"
> > +	while (( $naextents < 4 )); do
> > +		xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
> > +
> > +		for i in $(seq 1 $nr_attrs); do
> > +			last="trusted.""$(uuidgen)"
> > +			$SETFATTR_PROG -n $last $testfile
> > +		done
> > +
> > +		_scratch_unmount >> $seqres.full
> > +
> > +		naextents=$(_scratch_get_iext_count $testino attr || \
> > +				_fail "Unable to obtain inode fork's extent count")
> > +
> > +		_scratch_mount >> $seqres.full
> > +	done
> > +
> > +	echo "Inject reduce_max_iextents error tag"
> > +	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
> > +
> > +	echo "Remove xattr to trigger -EFBIG"
> > +	$SETFATTR_PROG -x "$last" $testfile >> $seqres.full 2>&1
> > +	if [[ $? == 0 ]]; then
> > +		echo "Xattr removal succeeded; Should have failed "
> 
> So at this point the user has a file for which he can't ever remove the
> xattrs for fear of overflowing naextents.  The only way to clear this is
> to delete the file, so shouldn't you be testing that this succeeds?

Ok. I will add that. However there is another issue here. I think it would better
suited to discuss that w.r.t XFS_IEXT_DIR_MANIP_CNT tests (i.e. patch 6).

> 
> --D
> 
> > +		exit 1
> > +	fi
> > +}
> > +
> > +attr_set
> > +attr_remove
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/525.out b/tests/xfs/525.out
> > new file mode 100644
> > index 00000000..cc40e6e2
> > --- /dev/null
> > +++ b/tests/xfs/525.out
> > @@ -0,0 +1,16 @@
> > +QA output created by 525
> > +* Set xattrs
> > +Format and mount fs
> > +Consume free space
> > +Create fragmented filesystem
> > +Inject reduce_max_iextents error tag
> > +Inject bmap_alloc_minlen_extent error tag
> > +Create xattrs
> > +Verify uquota inode's extent count
> > +* Remove xattrs
> > +Format and mount fs
> > +Consume free space
> > +Create fragmented filesystem
> > +Create initial xattr extents
> > +Inject reduce_max_iextents error tag
> > +Remove xattr to trigger -EFBIG
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index 3fa38c36..bd38aff0 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -522,3 +522,4 @@
> >  522 auto quick quota
> >  523 auto quick realtime growfs
> >  524 auto quick punch zero insert collapse
> > +525 auto quick attr
>
diff mbox series

Patch

diff --git a/tests/xfs/525 b/tests/xfs/525
new file mode 100755
index 00000000..1d5d6e7c
--- /dev/null
+++ b/tests/xfs/525
@@ -0,0 +1,154 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 Chandan Babu R.  All Rights Reserved.
+#
+# FS QA Test 525
+#
+# Verify that XFS does not cause inode fork's extent count to overflow when
+# Adding/removing xattrs.
+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
+. ./common/attr
+. ./common/inject
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+_supported_fs xfs
+_require_scratch
+_require_attrs
+_require_xfs_debug
+_require_test_program "punch-alternating"
+_require_xfs_io_error_injection "reduce_max_iextents"
+_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
+
+attr_set()
+{
+	echo "* Set xattrs"
+
+	echo "Format and mount fs"
+	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
+	_scratch_mount >> $seqres.full
+
+	bsize=$(_get_block_size $SCRATCH_MNT)
+
+	testfile=$SCRATCH_MNT/testfile
+
+	echo "Consume free space"
+	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
+	sync
+
+	echo "Create fragmented filesystem"
+	$here/src/punch-alternating $testfile >> $seqres.full
+	sync
+
+	echo "Inject reduce_max_iextents error tag"
+	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
+
+	echo "Inject bmap_alloc_minlen_extent error tag"
+	xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
+
+	echo "Create xattrs"
+
+	attr_len=$(uuidgen | wc -c)
+	nr_attrs=$((bsize * 20 / attr_len))
+	for i in $(seq 1 $nr_attrs); do
+		$SETFATTR_PROG -n "trusted.""$(uuidgen)" $testfile \
+			 >> $seqres.full 2>&1
+		[[ $? != 0 ]] && break
+	done
+
+	testino=$(stat -c "%i" $testfile)
+
+	_scratch_unmount >> $seqres.full
+
+	echo "Verify uquota inode's extent count"
+
+	nextents=$(_scratch_get_iext_count $testino attr || \
+			_fail "Unable to obtain inode fork's extent count")
+	if (( $nextents > 10 )); then
+		echo "Extent count overflow check failed: nextents = $nextents"
+		exit 1
+	fi
+}
+
+attr_remove()
+{
+	echo "* Remove xattrs"
+
+	echo "Format and mount fs"
+	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
+	_scratch_mount >> $seqres.full
+
+	bsize=$(_get_block_size $SCRATCH_MNT)
+
+	testfile=$SCRATCH_MNT/testfile
+
+	echo "Consume free space"
+	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
+	sync
+
+	echo "Create fragmented filesystem"
+	$here/src/punch-alternating $testfile >> $seqres.full
+	sync
+
+	testino=$(stat -c "%i" $testfile)
+
+	naextents=0
+	last=""
+
+	attr_len=$(uuidgen | wc -c)
+	nr_attrs=$((bsize / attr_len))
+
+	echo "Create initial xattr extents"
+	while (( $naextents < 4 )); do
+		xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
+
+		for i in $(seq 1 $nr_attrs); do
+			last="trusted.""$(uuidgen)"
+			$SETFATTR_PROG -n $last $testfile
+		done
+
+		_scratch_unmount >> $seqres.full
+
+		naextents=$(_scratch_get_iext_count $testino attr || \
+				_fail "Unable to obtain inode fork's extent count")
+
+		_scratch_mount >> $seqres.full
+	done
+
+	echo "Inject reduce_max_iextents error tag"
+	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
+
+	echo "Remove xattr to trigger -EFBIG"
+	$SETFATTR_PROG -x "$last" $testfile >> $seqres.full 2>&1
+	if [[ $? == 0 ]]; then
+		echo "Xattr removal succeeded; Should have failed "
+		exit 1
+	fi
+}
+
+attr_set
+attr_remove
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/525.out b/tests/xfs/525.out
new file mode 100644
index 00000000..cc40e6e2
--- /dev/null
+++ b/tests/xfs/525.out
@@ -0,0 +1,16 @@ 
+QA output created by 525
+* Set xattrs
+Format and mount fs
+Consume free space
+Create fragmented filesystem
+Inject reduce_max_iextents error tag
+Inject bmap_alloc_minlen_extent error tag
+Create xattrs
+Verify uquota inode's extent count
+* Remove xattrs
+Format and mount fs
+Consume free space
+Create fragmented filesystem
+Create initial xattr extents
+Inject reduce_max_iextents error tag
+Remove xattr to trigger -EFBIG
diff --git a/tests/xfs/group b/tests/xfs/group
index 3fa38c36..bd38aff0 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -522,3 +522,4 @@ 
 522 auto quick quota
 523 auto quick realtime growfs
 524 auto quick punch zero insert collapse
+525 auto quick attr