diff mbox series

[RFC] fstests: add mmap page boundary tests

Message ID 20240415081054.1782715-1-mcgrof@kernel.org (mailing list archive)
State Superseded, archived
Headers show
Series [RFC] fstests: add mmap page boundary tests | expand

Commit Message

Luis Chamberlain April 15, 2024, 8:10 a.m. UTC 
mmap() POSIX compliance says we should zero fill data beyond a file
size up to page boundary, and issue a SIGBUS if we go beyond. While fsx
helps us test zero-fill sometimes, fsstress also let's us sometimes test
for SIGBUS however that is based on a random value and its not likley we
always test it. Dedicate a specic test for this to make testing for
this specific situation and to easily expand on other corner cases.

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

Does enough to get us to use to test this, however I'm aware of a bit
more polishing up to do:

  * maybe moving mread to common as generic/574 did it first
  * sharing round_up_to_page_boundary() as well

generic/574 is special, it was just testing for correctness of
integrity if we muck with mmap() however if you don't have verity
stuff available obviously you won't end up testing it.

This generalizes mmap() zero-fill and SIGBUS corner case tests.

I've tested so far only 4k and it works well there. For 16k bs on LBS
just the SIGBUS issue exists, I'll test smaller block sizes later like
512, 1k, 2k as well. We'll fix triggering the SIBGUS when LBS is used,
we'll address that in the next iteration.

Is this a worthy test as a generic test?

 common/filter         |   6 ++
 tests/generic/740     | 231 ++++++++++++++++++++++++++++++++++++++++++
 tests/generic/740.out |   2 +
 3 files changed, 239 insertions(+)
 create mode 100755 tests/generic/740
 create mode 100644 tests/generic/740.out

Comments

Darrick J. Wong April 15, 2024, 3:58 p.m. UTC | #1 
On Mon, Apr 15, 2024 at 01:10:54AM -0700, Luis Chamberlain wrote:
> mmap() POSIX compliance says we should zero fill data beyond a file
> size up to page boundary, and issue a SIGBUS if we go beyond. While fsx
> helps us test zero-fill sometimes, fsstress also let's us sometimes test
> for SIGBUS however that is based on a random value and its not likley we
> always test it. Dedicate a specic test for this to make testing for
> this specific situation and to easily expand on other corner cases.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> 
> Does enough to get us to use to test this, however I'm aware of a bit
> more polishing up to do:
> 
>   * maybe moving mread to common as generic/574 did it first
>   * sharing round_up_to_page_boundary() as well
> 
> generic/574 is special, it was just testing for correctness of
> integrity if we muck with mmap() however if you don't have verity
> stuff available obviously you won't end up testing it.
> 
> This generalizes mmap() zero-fill and SIGBUS corner case tests.
> 
> I've tested so far only 4k and it works well there. For 16k bs on LBS
> just the SIGBUS issue exists, I'll test smaller block sizes later like
> 512, 1k, 2k as well. We'll fix triggering the SIBGUS when LBS is used,
> we'll address that in the next iteration.
> 
> Is this a worthy test as a generic test?
> 
>  common/filter         |   6 ++
>  tests/generic/740     | 231 ++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/740.out |   2 +
>  3 files changed, 239 insertions(+)
>  create mode 100755 tests/generic/740
>  create mode 100644 tests/generic/740.out
> 
> diff --git a/common/filter b/common/filter
> index 36d51bd957dd..d7add06f3be7 100644
> --- a/common/filter
> +++ b/common/filter
> @@ -194,6 +194,12 @@ _filter_xfs_io_unique()
>      common_line_filter | _filter_xfs_io
>  }
>  
> +_filter_xfs_io_data_unique()
> +{
> +    _filter_xfs_io_offset | sed -e 's| |\n|g' | egrep -v "\.|XX|\*" | \
> +	sort | uniq | tr -d '\n'
> +}
> +
>  _filter_xfs_io_units_modified()
>  {
>  	UNIT=$1
> diff --git a/tests/generic/740 b/tests/generic/740
> new file mode 100755
> index 000000000000..cbb823014600
> --- /dev/null
> +++ b/tests/generic/740
> @@ -0,0 +1,231 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) Luis Chamberlain. All Rights Reserved.
> +#
> +# FS QA Test 740
> +#
> +# As per POSIX NOTES mmap(2) maps multiples of the system page size, but if the
> +# data mapped is not multiples of the page size the remaining bytes are zeroed
> +# out when mapped and modifications to that region are not written to the file.
> +# On Linux when you write data to such partial page after the end of the
> +# object, the data stays in the page cache even after the file is closed and
> +# unmapped and  even  though  the data  is never written to the file itself,
> +# subsequent mappings may see the modified content. If you go *beyond* this
> +# page, you should get a SIGBUS. This test verifies we zero-fill to page
> +# boundary and ensures we get a SIGBUS if we write to data beyond the system
> +# page size even if the block size is greater than the system page size.
> +. ./common/preamble
> +. ./common/rc
> +_begin_fstest auto quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_scratch_nocheck
> +_require_test
> +
> +setup_zeroed_file()
> +{
> +	local file_len=$1
> +	local sparse=$2
> +
> +	if $sparse; then
> +		$XFS_IO_PROG -f -c "truncate $file_len" $test_file
> +	else
> +		$XFS_IO_PROG -f -c "falloc 0 $file_len" $test_file
> +	fi
> +}
> +
> +round_up_to_page_boundary()
> +{
> +	local n=$1
> +	local page_size=$(_get_page_size)
> +
> +	echo $(( (n + page_size - 1) & ~(page_size - 1) ))

Does iomap put a large folio into the pagecache that crosses EOF, or
does it back down to base page size?

> +}
> +
> +mread()
> +{
> +	local file=$1
> +	local map_len=$2
> +	local offset=$3
> +	local length=$4
> +
> +	# Some callers expect xfs_io to crash with SIGBUS due to the mread,
> +	# causing the shell to print "Bus error" to stderr.  To allow this
> +	# message to be redirected, execute xfs_io in a new shell instance.
> +	# However, for this to work reliably, we also need to prevent the new
> +	# shell instance from optimizing out the fork and directly exec'ing
> +	# xfs_io.  The easiest way to do that is to append 'true' to the
> +	# commands, so that xfs_io is no longer the last command the shell sees.
> +	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $file \
> +		-c 'mmap -r 0 $map_len' \
> +		-c 'mread $offset $length'; true"

Please hoist the mread() function with generic/574 to common; your copy
is out of date with the original.

> +}
> +
> +do_mmap_tests()
> +{
> +	local block_size=$1
> +	local file_len=$2
> +	local offset=$3
> +	local len=$4
> +	local use_sparse_file=${5:-false}
> +	local new_filelen=0
> +	local map_len=0
> +	local csum=0
> +	local fs_block_size=$(_get_block_size $SCRATCH_MNT)
> +
> +	echo -en "\n\n==> Testing blocksize $block_size " >> $seqres.full
> +	echo -en "file_len: $file_len offset: $offset " >> $seqres.full
> +	echo -e "len: $len sparse: $use_sparse_file" >> $seqres.full
> +
> +	if ((fs_block_size != block_size)); then
> +		echo "Block size created ($block_size) doesn't match _get_block_size on mount ($fs_block_size)"
> +		_fail

_fail "Block size created..."

> +	fi
> +
> +	rm -rf "${SCRATCH_MNT:?}"/*
> +
> +	# This let's us also test against sparse files
> +	setup_zeroed_file $file_len $use_sparse_file
> +
> +	# This will overwrite the old data, the file size is the
> +	# delta between offset and len now.
> +	$XFS_IO_PROG -f -c "pwrite -S 0xaa -b 512 $offset $len" \
> +		$test_file >> $seqres.full
> +
> +	sync
> +	new_filelen=$(_get_filesize $test_file)
> +	map_len=$(round_up_to_page_boundary $new_filelen)
> +	csum_orig="$(_md5_checksum $test_file)"
> +
> +	# A couple of mmap() tests:
> +	#
> +	# We are allowed to mmap() up to the boundary of the page size of a
> +	# data object, but there a few rules to follow we must check for:
> +	#
> +	# a) zero-fill test for the data: POSIX says we should zero fill any
> +	#    partial page after the end of the object. Verify zero-fill.
> +	# b) do not write this bogus data to disk: on Linux, if we write data
> +	#    to a partially filled page, it will stay in the page cache even
> +	#    after the file is closed and unmapped even if it never reaches the
> +	#    file. Subsequent mappings *may* see the modified content, but it
> +	#    also can get other data. Since the data read after the actual

What does "other data" mean?

> +	#    object data can vary we just verify the filesize does not change.
> +	#    This is not true for tmpfs.

Er... is this file size change a bug?

> +	if [[ $map_len -gt $new_filelen ]]; then
> +		zero_filled_data_len=$((map_len - new_filelen))
> +		_scratch_cycle_mount
> +		expected_zero_data="00"
> +		zero_filled_data=$($XFS_IO_PROG -r $test_file \
> +			-c "mmap -r 0 $map_len" \
> +			-c "mread -v $new_filelen $zero_filled_data_len" \
> +			-c "munmap" | \
> +			_filter_xfs_io_data_unique)
> +		if [[ "$zero_filled_data" != "$expected_zero_data" ]]; then
> +			echo "Expected data: $expected_zero_data"
> +			echo "  Actual data: $zero_filled_data"
> +			echo "Zero-fill broken see mmap() requirements"
> +			_fail
> +		fi
> +
> +		if [[ "$FSTYP" != "tmpfs" ]]; then
> +			_scratch_cycle_mount
> +			$XFS_IO_PROG $test_file \
> +				-c "mmap -w 0 $map_len" \
> +				-c "mwrite $new_filelen $zero_filled_data_len" \
> +				-c "munmap"
> +			sync
> +			csum_post="$(_md5_checksum $test_file)"
> +			if [[ "$csum_orig" != "$csum_post" ]]; then
> +				echo "Expected csum: $csum_orig"
> +				echo " Actual  csum: $csum_post"
> +				_fail
> +			fi
> +
> +			local filelen_test=$(_get_filesize $test_file)
> +			if [[ "$filelen_test" != "$new_filelen" ]]; then
> +				echo "Expected file length: $new_filelen"
> +				echo " Actual  file length: $filelen_test"
> +				_fail
> +			fi
> +		fi
> +	fi
> +
> +	# Now lets ensure we get SIGBUS when we go beyond the page boundary
> +	if [[ "$FSTYP" != "tmpfs" ]]; then
> +		_scratch_cycle_mount
> +		new_filelen=$(_get_filesize $test_file)
> +		map_len=$(round_up_to_page_boundary $new_filelen)
> +		csum_orig="$(_md5_checksum $test_file)"
> +		mread $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
> +		if grep -q 'Bus error' $tmp.err; then
> +			echo "Not expecting SIGBUS when reading up to page boundary"
> +			cat $tmp.err
> +			_fail
> +		fi
> +
> +		# This should just work
> +		mread $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
> +		if [[ $? -ne 0 ]]; then
> +			_fail
> +		fi
> +
> +		# If we mmap() on the boundary but try to read beyond it just
> +		# fails, we don't get a SIGBUS
> +		$XFS_IO_PROG -r $test_file \
> +			-c "mmap -r 0 $map_len" \
> +			-c "mread 0 $((map_len + 10))" >> $seqres.full  2>$tmp.err
> +		local mread_err=$?
> +		if [[ $mread_err -eq 0 ]]; then
> +			echo "mmap() to page boundary works as expected but reading beyond should fail"
> +			echo "err: $?"
> +			_fail
> +		fi
> +
> +		# Now let's go beyond the allowed mmap() page boundary
> +		mread $test_file $((map_len + 10)) 0 $((map_len + 10)) >> $seqres.full  2>$tmp.err
> +		if ! grep -q 'Bus error' $tmp.err; then
> +			echo "Expected SIGBUS when mmap() reading beyond page boundary"
> +			_fail
> +		fi
> +		local filelen_test=$(_get_filesize $test_file)
> +		if [[ "$filelen_test" != "$new_filelen" ]]; then
> +			echo "Expected file length: $new_filelen"
> +			echo " Actual  file length: $filelen_test"
> +			_fail
> +		fi
> +	fi
> +}
> +
> +test_block_size()
> +{
> +	local block_size=$1
> +
> +	do_mmap_tests $block_size 512 3 5
> +	do_mmap_tests $block_size 16k 0 $((16384+3))
> +	do_mmap_tests $block_size 16k $((16384-10)) $((16384+20))
> +	do_mmap_tests $block_size 64k 0 $((65536+3))
> +	do_mmap_tests $block_size 4k 4090 30 true
> +}
> +
> +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> +_scratch_mount
> +test_file=$SCRATCH_MNT/file
> +block_size=$(_get_block_size "$SCRATCH_MNT")

_get_file_block_size ?

> +test_block_size $block_size
> +_scratch_unmount
> +_check_scratch_fs

This work is done for you automatically if you simply exit the test
with the scratch fs still mounted.

--D

> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/740.out b/tests/generic/740.out
> new file mode 100644
> index 000000000000..3f841e600ed3
> --- /dev/null
> +++ b/tests/generic/740.out
> @@ -0,0 +1,2 @@
> +QA output created by 740
> +Silence is golden
> -- 
> 2.43.0
> 
>
Zorro Lang April 15, 2024, 8:05 p.m. UTC | #2 
On Mon, Apr 15, 2024 at 01:10:54AM -0700, Luis Chamberlain wrote:
> mmap() POSIX compliance says we should zero fill data beyond a file
> size up to page boundary, and issue a SIGBUS if we go beyond. While fsx
> helps us test zero-fill sometimes, fsstress also let's us sometimes test
> for SIGBUS however that is based on a random value and its not likley we
> always test it. Dedicate a specic test for this to make testing for
> this specific situation and to easily expand on other corner cases.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> 
> Does enough to get us to use to test this, however I'm aware of a bit
> more polishing up to do:
> 
>   * maybe moving mread to common as generic/574 did it first
>   * sharing round_up_to_page_boundary() as well
> 
> generic/574 is special, it was just testing for correctness of
> integrity if we muck with mmap() however if you don't have verity
> stuff available obviously you won't end up testing it.
> 
> This generalizes mmap() zero-fill and SIGBUS corner case tests.
> 
> I've tested so far only 4k and it works well there. For 16k bs on LBS
> just the SIGBUS issue exists, I'll test smaller block sizes later like
> 512, 1k, 2k as well. We'll fix triggering the SIBGUS when LBS is used,
> we'll address that in the next iteration.
> 
> Is this a worthy test as a generic test?
> 
>  common/filter         |   6 ++
>  tests/generic/740     | 231 ++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/740.out |   2 +
>  3 files changed, 239 insertions(+)
>  create mode 100755 tests/generic/740
>  create mode 100644 tests/generic/740.out
> 
> diff --git a/common/filter b/common/filter
> index 36d51bd957dd..d7add06f3be7 100644
> --- a/common/filter
> +++ b/common/filter
> @@ -194,6 +194,12 @@ _filter_xfs_io_unique()
>      common_line_filter | _filter_xfs_io
>  }
>  
> +_filter_xfs_io_data_unique()
> +{
> +    _filter_xfs_io_offset | sed -e 's| |\n|g' | egrep -v "\.|XX|\*" | \

The egrep is deprecated, please use 'grep -E'.

> +	sort | uniq | tr -d '\n'

Isn't "sort | uniq" equal to "sort -u" ?

> +}

Do we need this filter to be a common helper? Will it be used widely? If not,
this can be a local function of below single test case.

> +
>  _filter_xfs_io_units_modified()
>  {
>  	UNIT=$1
> diff --git a/tests/generic/740 b/tests/generic/740
> new file mode 100755
> index 000000000000..cbb823014600
> --- /dev/null
> +++ b/tests/generic/740
> @@ -0,0 +1,231 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) Luis Chamberlain. All Rights Reserved.
> +#
> +# FS QA Test 740
> +#
> +# As per POSIX NOTES mmap(2) maps multiples of the system page size, but if the
> +# data mapped is not multiples of the page size the remaining bytes are zeroed
> +# out when mapped and modifications to that region are not written to the file.
> +# On Linux when you write data to such partial page after the end of the
> +# object, the data stays in the page cache even after the file is closed and
> +# unmapped and  even  though  the data  is never written to the file itself,
> +# subsequent mappings may see the modified content. If you go *beyond* this
> +# page, you should get a SIGBUS. This test verifies we zero-fill to page
> +# boundary and ensures we get a SIGBUS if we write to data beyond the system
> +# page size even if the block size is greater than the system page size.
> +. ./common/preamble
> +. ./common/rc
> +_begin_fstest auto quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.*
> +}

This _cleanup is same with the default one, don't need this override.

> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_scratch_nocheck
> +_require_test
> +
> +setup_zeroed_file()
> +{
> +	local file_len=$1
> +	local sparse=$2
> +
> +	if $sparse; then
> +		$XFS_IO_PROG -f -c "truncate $file_len" $test_file

_require_xfs_io_command "truncate"

> +	else
> +		$XFS_IO_PROG -f -c "falloc 0 $file_len" $test_file

_require_xfs_io_command "falloc"

And with this, better to add this case into "prealloc" group

> +	fi
> +}
> +
> +round_up_to_page_boundary()
> +{
> +	local n=$1
> +	local page_size=$(_get_page_size)
> +
> +	echo $(( (n + page_size - 1) & ~(page_size - 1) ))
> +}
> +
> +mread()
> +{
> +	local file=$1
> +	local map_len=$2
> +	local offset=$3
> +	local length=$4
> +
> +	# Some callers expect xfs_io to crash with SIGBUS due to the mread,
> +	# causing the shell to print "Bus error" to stderr.  To allow this
> +	# message to be redirected, execute xfs_io in a new shell instance.
> +	# However, for this to work reliably, we also need to prevent the new
> +	# shell instance from optimizing out the fork and directly exec'ing
> +	# xfs_io.  The easiest way to do that is to append 'true' to the
> +	# commands, so that xfs_io is no longer the last command the shell sees.
> +	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $file \
> +		-c 'mmap -r 0 $map_len' \
> +		-c 'mread $offset $length'; true"
> +}
> +
> +do_mmap_tests()
> +{
> +	local block_size=$1
> +	local file_len=$2
> +	local offset=$3
> +	local len=$4
> +	local use_sparse_file=${5:-false}
> +	local new_filelen=0
> +	local map_len=0
> +	local csum=0
> +	local fs_block_size=$(_get_block_size $SCRATCH_MNT)

Generally the _get_file_block_size is recommended, rather than _get_block_size.

> +
> +	echo -en "\n\n==> Testing blocksize $block_size " >> $seqres.full
> +	echo -en "file_len: $file_len offset: $offset " >> $seqres.full
> +	echo -e "len: $len sparse: $use_sparse_file" >> $seqres.full
> +
> +	if ((fs_block_size != block_size)); then
> +		echo "Block size created ($block_size) doesn't match _get_block_size on mount ($fs_block_size)"
> +		_fail


_fail "Block size created ($block_size) doesn't match _get_block_size on mount ($fs_block_size)" ?

Generally we don't use an empty _fail, same as all "_fail" call in this patch.

> +	fi
> +
> +	rm -rf "${SCRATCH_MNT:?}"/*
> +
> +	# This let's us also test against sparse files
> +	setup_zeroed_file $file_len $use_sparse_file
> +
> +	# This will overwrite the old data, the file size is the
> +	# delta between offset and len now.
> +	$XFS_IO_PROG -f -c "pwrite -S 0xaa -b 512 $offset $len" \
> +		$test_file >> $seqres.full
> +
> +	sync
> +	new_filelen=$(_get_filesize $test_file)
> +	map_len=$(round_up_to_page_boundary $new_filelen)
> +	csum_orig="$(_md5_checksum $test_file)"
> +
> +	# A couple of mmap() tests:
> +	#
> +	# We are allowed to mmap() up to the boundary of the page size of a
> +	# data object, but there a few rules to follow we must check for:
> +	#
> +	# a) zero-fill test for the data: POSIX says we should zero fill any
> +	#    partial page after the end of the object. Verify zero-fill.
> +	# b) do not write this bogus data to disk: on Linux, if we write data
> +	#    to a partially filled page, it will stay in the page cache even
> +	#    after the file is closed and unmapped even if it never reaches the
> +	#    file. Subsequent mappings *may* see the modified content, but it
> +	#    also can get other data. Since the data read after the actual
> +	#    object data can vary we just verify the filesize does not change.
> +	#    This is not true for tmpfs.
> +	if [[ $map_len -gt $new_filelen ]]; then
> +		zero_filled_data_len=$((map_len - new_filelen))
> +		_scratch_cycle_mount
> +		expected_zero_data="00"
> +		zero_filled_data=$($XFS_IO_PROG -r $test_file \
> +			-c "mmap -r 0 $map_len" \
> +			-c "mread -v $new_filelen $zero_filled_data_len" \
> +			-c "munmap" | \
> +			_filter_xfs_io_data_unique)
> +		if [[ "$zero_filled_data" != "$expected_zero_data" ]]; then
> +			echo "Expected data: $expected_zero_data"
> +			echo "  Actual data: $zero_filled_data"
> +			echo "Zero-fill broken see mmap() requirements"
> +			_fail
> +		fi
> +
> +		if [[ "$FSTYP" != "tmpfs" ]]; then
> +			_scratch_cycle_mount
> +			$XFS_IO_PROG $test_file \
> +				-c "mmap -w 0 $map_len" \
> +				-c "mwrite $new_filelen $zero_filled_data_len" \
> +				-c "munmap"
> +			sync
> +			csum_post="$(_md5_checksum $test_file)"
> +			if [[ "$csum_orig" != "$csum_post" ]]; then
> +				echo "Expected csum: $csum_orig"
> +				echo " Actual  csum: $csum_post"
> +				_fail
> +			fi
> +
> +			local filelen_test=$(_get_filesize $test_file)
> +			if [[ "$filelen_test" != "$new_filelen" ]]; then
> +				echo "Expected file length: $new_filelen"
> +				echo " Actual  file length: $filelen_test"
> +				_fail
> +			fi
> +		fi
> +	fi
> +
> +	# Now lets ensure we get SIGBUS when we go beyond the page boundary
> +	if [[ "$FSTYP" != "tmpfs" ]]; then
> +		_scratch_cycle_mount
> +		new_filelen=$(_get_filesize $test_file)
> +		map_len=$(round_up_to_page_boundary $new_filelen)
> +		csum_orig="$(_md5_checksum $test_file)"
> +		mread $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
> +		if grep -q 'Bus error' $tmp.err; then
> +			echo "Not expecting SIGBUS when reading up to page boundary"
> +			cat $tmp.err
> +			_fail
> +		fi
> +
> +		# This should just work
> +		mread $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
> +		if [[ $? -ne 0 ]]; then
> +			_fail
> +		fi
> +
> +		# If we mmap() on the boundary but try to read beyond it just
> +		# fails, we don't get a SIGBUS
> +		$XFS_IO_PROG -r $test_file \
> +			-c "mmap -r 0 $map_len" \
> +			-c "mread 0 $((map_len + 10))" >> $seqres.full  2>$tmp.err
> +		local mread_err=$?
> +		if [[ $mread_err -eq 0 ]]; then
> +			echo "mmap() to page boundary works as expected but reading beyond should fail"
> +			echo "err: $?"
> +			_fail
> +		fi
> +
> +		# Now let's go beyond the allowed mmap() page boundary
> +		mread $test_file $((map_len + 10)) 0 $((map_len + 10)) >> $seqres.full  2>$tmp.err
> +		if ! grep -q 'Bus error' $tmp.err; then
> +			echo "Expected SIGBUS when mmap() reading beyond page boundary"
> +			_fail
> +		fi
> +		local filelen_test=$(_get_filesize $test_file)
> +		if [[ "$filelen_test" != "$new_filelen" ]]; then
> +			echo "Expected file length: $new_filelen"
> +			echo " Actual  file length: $filelen_test"
> +			_fail
> +		fi
> +	fi
> +}
> +
> +test_block_size()
> +{
> +	local block_size=$1
> +
> +	do_mmap_tests $block_size 512 3 5
> +	do_mmap_tests $block_size 16k 0 $((16384+3))
> +	do_mmap_tests $block_size 16k $((16384-10)) $((16384+20))
> +	do_mmap_tests $block_size 64k 0 $((65536+3))
> +	do_mmap_tests $block_size 4k 4090 30 true
> +}
> +
> +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> +_scratch_mount
> +test_file=$SCRATCH_MNT/file
> +block_size=$(_get_block_size "$SCRATCH_MNT")
> +test_block_size $block_size
> +_scratch_unmount
> +_check_scratch_fs

The _scratch_unmount && _check_scratch_fs aren't needed, they'll be done
after this test done.

> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/740.out b/tests/generic/740.out
> new file mode 100644
> index 000000000000..3f841e600ed3
> --- /dev/null
> +++ b/tests/generic/740.out
> @@ -0,0 +1,2 @@
> +QA output created by 740
> +Silence is golden
> -- 
> 2.43.0
> 
>
Pankaj Raghav (Samsung) April 22, 2024, 2:09 p.m. UTC | #3 
> > +setup_zeroed_file()
> > +{
> > +	local file_len=$1
> > +	local sparse=$2
> > +
> > +	if $sparse; then
> > +		$XFS_IO_PROG -f -c "truncate $file_len" $test_file
> > +	else
> > +		$XFS_IO_PROG -f -c "falloc 0 $file_len" $test_file
> > +	fi
> > +}
> > +
> > +round_up_to_page_boundary()
> > +{
> > +	local n=$1
> > +	local page_size=$(_get_page_size)
> > +
> > +	echo $(( (n + page_size - 1) & ~(page_size - 1) ))
> 
> Does iomap put a large folio into the pagecache that crosses EOF, or
> does it back down to base page size?

No, we back it down to the base page size if the higher order folio
crosses EOF. But this changes when we have the LBS support as we need to
guarantee the base folio order that will be based on the filesystem BS.
Pankaj Raghav (Samsung) April 22, 2024, 2:12 p.m. UTC | #4 
On Mon, Apr 15, 2024 at 01:10:54AM -0700, Luis Chamberlain wrote:
> mmap() POSIX compliance says we should zero fill data beyond a file
> size up to page boundary, and issue a SIGBUS if we go beyond. While fsx
> helps us test zero-fill sometimes, fsstress also let's us sometimes test
> for SIGBUS however that is based on a random value and its not likley we
> always test it. Dedicate a specic test for this to make testing for
> this specific situation and to easily expand on other corner cases.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> 
> Does enough to get us to use to test this, however I'm aware of a bit
> more polishing up to do:
> 
>   * maybe moving mread to common as generic/574 did it first
>   * sharing round_up_to_page_boundary() as well
> 
> generic/574 is special, it was just testing for correctness of
> integrity if we muck with mmap() however if you don't have verity
> stuff available obviously you won't end up testing it.
> 
> This generalizes mmap() zero-fill and SIGBUS corner case tests.
> 
> I've tested so far only 4k and it works well there. For 16k bs on LBS
> just the SIGBUS issue exists, I'll test smaller block sizes later like
> 512, 1k, 2k as well. We'll fix triggering the SIBGUS when LBS is used,
> we'll address that in the next iteration.
> 
> Is this a worthy test as a generic test?
> 
>  common/filter         |   6 ++
>  tests/generic/740     | 231 ++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/740.out |   2 +
>  3 files changed, 239 insertions(+)
>  create mode 100755 tests/generic/740
>  create mode 100644 tests/generic/740.out
> 
We could also extend the test to include mmap writes. 

I took the lazy approach here but I think we can make the code look 
prettier by adding some more functions that can do both mmap read and writes.

diff --git a/tests/generic/740 b/tests/generic/740
index cbb82301..93663964 100755
--- a/tests/generic/740
+++ b/tests/generic/740
@@ -53,6 +53,25 @@ round_up_to_page_boundary()
        echo $(( (n + page_size - 1) & ~(page_size - 1) ))
 }
 
+mwrite()
+{
+       local file=$1
+       local map_len=$2
+       local offset=$3
+       local length=$4
+
+       # Some callers expect xfs_io to crash with SIGBUS due to the mread,
+       # causing the shell to print "Bus error" to stderr.  To allow this
+       # message to be redirected, execute xfs_io in a new shell instance.
+       # However, for this to work reliably, we also need to prevent the new
+       # shell instance from optimizing out the fork and directly exec'ing
+       # xfs_io.  The easiest way to do that is to append 'true' to the
+       # commands, so that xfs_io is no longer the last command the shell sees.
+       bash -c "trap '' SIGBUS; $XFS_IO_PROG $file \
+               -c 'mmap -w 0 $map_len' \
+               -c 'mwrite $offset $length'; true"
+}
+
 mread()
 {
        local file=$1
@@ -180,6 +199,12 @@ do_mmap_tests()
                        _fail
                fi
 
+               # This should just work
+               mwrite $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
+               if [[ $? -ne 0 ]]; then
+                       _fail
+               fi
+
                # If we mmap() on the boundary but try to read beyond it just
                # fails, we don't get a SIGBUS
                $XFS_IO_PROG -r $test_file \
@@ -192,12 +217,29 @@ do_mmap_tests()
                        _fail
                fi
 
+               $XFS_IO_PROG -w $test_file \
+                       -c "mmap -w 0 $map_len" \
+                       -c "mwrite 0 $((map_len + 10))" >> $seqres.full  2>$tmp.err
+               local mwrite_err=$?
+               if [[ $mwrite_err -eq 0 ]]; then
+                       echo "mmap() to page boundary works as expected but writing beyond should fail"
+                       echo "err: $?"
+                       _fail
+               fi
+
                # Now let's go beyond the allowed mmap() page boundary
                mread $test_file $((map_len + 10)) 0 $((map_len + 10)) >> $seqres.full  2>$tmp.err
                if ! grep -q 'Bus error' $tmp.err; then
                        echo "Expected SIGBUS when mmap() reading beyond page boundary"
                        _fail
                fi
+
+               mwrite $test_file $((map_len + 10)) 0 $((map_len + 10)) >> $seqres.full  2>$tmp.err
+               if ! grep -q 'Bus error' $tmp.err; then
+                       echo "Expected SIGBUS when mmap() writing beyond page boundary"
+                       _fail
+               fi
+
                local filelen_test=$(_get_filesize $test_file)
                if [[ "$filelen_test" != "$new_filelen" ]]; then
                        echo "Expected file length: $new_filelen"


--
Pankaj
Luis Chamberlain May 28, 2024, 10:18 p.m. UTC | #5 
On Mon, Apr 15, 2024 at 08:58:25AM -0700, Darrick J. Wong wrote:
> On Mon, Apr 15, 2024 at 01:10:54AM -0700, Luis Chamberlain wrote:
> > +round_up_to_page_boundary()
> > +{
> > +	local n=$1
> > +	local page_size=$(_get_page_size)
> > +
> > +	echo $(( (n + page_size - 1) & ~(page_size - 1) ))
> 
> Does iomap put a large folio into the pagecache that crosses EOF

When minorder is used care had to be taken to ensure the EOF is properly
respected.  Refer to the patch titled "filemap: cap PTE range to be
created to allowed zero fill in folio_map_range()", with that, we fix
that case to respect the EOF. But since minorder is used the folio is
there.

> > +mread()
> > +{
> > +	local file=$1
> > +	local map_len=$2
> > +	local offset=$3
> > +	local length=$4
> > +
> > +	# Some callers expect xfs_io to crash with SIGBUS due to the mread,
> > +	# causing the shell to print "Bus error" to stderr.  To allow this
> > +	# message to be redirected, execute xfs_io in a new shell instance.
> > +	# However, for this to work reliably, we also need to prevent the new
> > +	# shell instance from optimizing out the fork and directly exec'ing
> > +	# xfs_io.  The easiest way to do that is to append 'true' to the
> > +	# commands, so that xfs_io is no longer the last command the shell sees.
> > +	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $file \
> > +		-c 'mmap -r 0 $map_len' \
> > +		-c 'mread $offset $length'; true"
> 
> Please hoist the mread() function with generic/574 to common; your copy
> is out of date with the original.

Sure thing! Shall we add _mwrite to common/rc too while at it or do we
wait to get a user for that?

> > +	# A couple of mmap() tests:
> > +	#
> > +	# We are allowed to mmap() up to the boundary of the page size of a
> > +	# data object, but there a few rules to follow we must check for:
> > +	#
> > +	# a) zero-fill test for the data: POSIX says we should zero fill any
> > +	#    partial page after the end of the object. Verify zero-fill.
> > +	# b) do not write this bogus data to disk: on Linux, if we write data
> > +	#    to a partially filled page, it will stay in the page cache even
> > +	#    after the file is closed and unmapped even if it never reaches the
> > +	#    file. Subsequent mappings *may* see the modified content, but it
> > +	#    also can get other data. Since the data read after the actual
> 
> What does "other data" mean?

That depends on the filesystem implementation, it just means we don't
provide a consistent behaviour or enforce a strategy for all filesystems.

> > +	#    object data can vary we just verify the filesize does not change.
> > +	#    This is not true for tmpfs.
> 
> Er... is this file size change a bug?

There is no filesize bug, the comment about tmpfs always ensuring seeing
the actual data since, well, there its kind of write-through. Since we
share the same filemap_map_pages() I'd expect the rest should behave the
same with tmpfs, but since I didn't test that the test skips it for now.

We'll test it, with all the patch "filemap: cap PTE range to be             
created to allowed zero fill in folio_map_range()" on tmpfs, and see if
we can just enable this test there too. Might as well as we're driving
by and sprinkling large folios there too.

  Luis
Luis Chamberlain May 28, 2024, 10:23 p.m. UTC | #6 
On Tue, Apr 16, 2024 at 04:05:42AM +0800, Zorro Lang wrote:
> On Mon, Apr 15, 2024 at 01:10:54AM -0700, Luis Chamberlain wrote:
> > diff --git a/common/filter b/common/filter
> > index 36d51bd957dd..d7add06f3be7 100644
> > --- a/common/filter
> > +++ b/common/filter
> > @@ -194,6 +194,12 @@ _filter_xfs_io_unique()
> >      common_line_filter | _filter_xfs_io
> >  }
> >  
> > +_filter_xfs_io_data_unique()
> > +{
> > +    _filter_xfs_io_offset | sed -e 's| |\n|g' | egrep -v "\.|XX|\*" | \
> 
> The egrep is deprecated, please use 'grep -E'.
> 
> > +	sort | uniq | tr -d '\n'
> 
> Isn't "sort | uniq" equal to "sort -u" ?

I'll try both suggestions.

> Do we need this filter to be a common helper? Will it be used widely? If not,
> this can be a local function of below single test case.

I suspect it will be but its OK I can stuff it alone, same as for
_mwrite() too then.

  Luis
Luis Chamberlain June 5, 2024, 4:40 a.m. UTC | #7 
On Tue, May 28, 2024 at 03:18:03PM -0700, Luis Chamberlain wrote:
> On Mon, Apr 15, 2024 at 08:58:25AM -0700, Darrick J. Wong wrote:
> > On Mon, Apr 15, 2024 at 01:10:54AM -0700, Luis Chamberlain wrote:
> > > +	#    This is not true for tmpfs.
> > 
> > Er... is this file size change a bug?
> 
> There is no filesize bug, the comment about tmpfs always ensuring seeing
> the actual data since, well, there its kind of write-through. Since we
> share the same filemap_map_pages() I'd expect the rest should behave the
> same with tmpfs, but since I didn't test that the test skips it for now.
> 
> We'll test it, with all the patch "filemap: cap PTE range to be             
> created to allowed zero fill in folio_map_range()" on tmpfs, and see if
> we can just enable this test there too. Might as well as we're driving
> by and sprinkling large folios there too.

Turns out our patch "filemap: cap PTE range to be created to allowed
zero fill in folio_map_range()" ends up fixing this on tmpfs for huge
pages. So consider this now tested on tmpfs as well, I'll adjust the
test for that.

  Luis
diff mbox series

Patch

diff --git a/common/filter b/common/filter
index 36d51bd957dd..d7add06f3be7 100644
--- a/common/filter
+++ b/common/filter
@@ -194,6 +194,12 @@  _filter_xfs_io_unique()
     common_line_filter | _filter_xfs_io
 }
 
+_filter_xfs_io_data_unique()
+{
+    _filter_xfs_io_offset | sed -e 's| |\n|g' | egrep -v "\.|XX|\*" | \
+	sort | uniq | tr -d '\n'
+}
+
 _filter_xfs_io_units_modified()
 {
 	UNIT=$1
diff --git a/tests/generic/740 b/tests/generic/740
new file mode 100755
index 000000000000..cbb823014600
--- /dev/null
+++ b/tests/generic/740
@@ -0,0 +1,231 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) Luis Chamberlain. All Rights Reserved.
+#
+# FS QA Test 740
+#
+# As per POSIX NOTES mmap(2) maps multiples of the system page size, but if the
+# data mapped is not multiples of the page size the remaining bytes are zeroed
+# out when mapped and modifications to that region are not written to the file.
+# On Linux when you write data to such partial page after the end of the
+# object, the data stays in the page cache even after the file is closed and
+# unmapped and  even  though  the data  is never written to the file itself,
+# subsequent mappings may see the modified content. If you go *beyond* this
+# page, you should get a SIGBUS. This test verifies we zero-fill to page
+# boundary and ensures we get a SIGBUS if we write to data beyond the system
+# page size even if the block size is greater than the system page size.
+. ./common/preamble
+. ./common/rc
+_begin_fstest auto quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.*
+}
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_require_scratch_nocheck
+_require_test
+
+setup_zeroed_file()
+{
+	local file_len=$1
+	local sparse=$2
+
+	if $sparse; then
+		$XFS_IO_PROG -f -c "truncate $file_len" $test_file
+	else
+		$XFS_IO_PROG -f -c "falloc 0 $file_len" $test_file
+	fi
+}
+
+round_up_to_page_boundary()
+{
+	local n=$1
+	local page_size=$(_get_page_size)
+
+	echo $(( (n + page_size - 1) & ~(page_size - 1) ))
+}
+
+mread()
+{
+	local file=$1
+	local map_len=$2
+	local offset=$3
+	local length=$4
+
+	# Some callers expect xfs_io to crash with SIGBUS due to the mread,
+	# causing the shell to print "Bus error" to stderr.  To allow this
+	# message to be redirected, execute xfs_io in a new shell instance.
+	# However, for this to work reliably, we also need to prevent the new
+	# shell instance from optimizing out the fork and directly exec'ing
+	# xfs_io.  The easiest way to do that is to append 'true' to the
+	# commands, so that xfs_io is no longer the last command the shell sees.
+	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $file \
+		-c 'mmap -r 0 $map_len' \
+		-c 'mread $offset $length'; true"
+}
+
+do_mmap_tests()
+{
+	local block_size=$1
+	local file_len=$2
+	local offset=$3
+	local len=$4
+	local use_sparse_file=${5:-false}
+	local new_filelen=0
+	local map_len=0
+	local csum=0
+	local fs_block_size=$(_get_block_size $SCRATCH_MNT)
+
+	echo -en "\n\n==> Testing blocksize $block_size " >> $seqres.full
+	echo -en "file_len: $file_len offset: $offset " >> $seqres.full
+	echo -e "len: $len sparse: $use_sparse_file" >> $seqres.full
+
+	if ((fs_block_size != block_size)); then
+		echo "Block size created ($block_size) doesn't match _get_block_size on mount ($fs_block_size)"
+		_fail
+	fi
+
+	rm -rf "${SCRATCH_MNT:?}"/*
+
+	# This let's us also test against sparse files
+	setup_zeroed_file $file_len $use_sparse_file
+
+	# This will overwrite the old data, the file size is the
+	# delta between offset and len now.
+	$XFS_IO_PROG -f -c "pwrite -S 0xaa -b 512 $offset $len" \
+		$test_file >> $seqres.full
+
+	sync
+	new_filelen=$(_get_filesize $test_file)
+	map_len=$(round_up_to_page_boundary $new_filelen)
+	csum_orig="$(_md5_checksum $test_file)"
+
+	# A couple of mmap() tests:
+	#
+	# We are allowed to mmap() up to the boundary of the page size of a
+	# data object, but there a few rules to follow we must check for:
+	#
+	# a) zero-fill test for the data: POSIX says we should zero fill any
+	#    partial page after the end of the object. Verify zero-fill.
+	# b) do not write this bogus data to disk: on Linux, if we write data
+	#    to a partially filled page, it will stay in the page cache even
+	#    after the file is closed and unmapped even if it never reaches the
+	#    file. Subsequent mappings *may* see the modified content, but it
+	#    also can get other data. Since the data read after the actual
+	#    object data can vary we just verify the filesize does not change.
+	#    This is not true for tmpfs.
+	if [[ $map_len -gt $new_filelen ]]; then
+		zero_filled_data_len=$((map_len - new_filelen))
+		_scratch_cycle_mount
+		expected_zero_data="00"
+		zero_filled_data=$($XFS_IO_PROG -r $test_file \
+			-c "mmap -r 0 $map_len" \
+			-c "mread -v $new_filelen $zero_filled_data_len" \
+			-c "munmap" | \
+			_filter_xfs_io_data_unique)
+		if [[ "$zero_filled_data" != "$expected_zero_data" ]]; then
+			echo "Expected data: $expected_zero_data"
+			echo "  Actual data: $zero_filled_data"
+			echo "Zero-fill broken see mmap() requirements"
+			_fail
+		fi
+
+		if [[ "$FSTYP" != "tmpfs" ]]; then
+			_scratch_cycle_mount
+			$XFS_IO_PROG $test_file \
+				-c "mmap -w 0 $map_len" \
+				-c "mwrite $new_filelen $zero_filled_data_len" \
+				-c "munmap"
+			sync
+			csum_post="$(_md5_checksum $test_file)"
+			if [[ "$csum_orig" != "$csum_post" ]]; then
+				echo "Expected csum: $csum_orig"
+				echo " Actual  csum: $csum_post"
+				_fail
+			fi
+
+			local filelen_test=$(_get_filesize $test_file)
+			if [[ "$filelen_test" != "$new_filelen" ]]; then
+				echo "Expected file length: $new_filelen"
+				echo " Actual  file length: $filelen_test"
+				_fail
+			fi
+		fi
+	fi
+
+	# Now lets ensure we get SIGBUS when we go beyond the page boundary
+	if [[ "$FSTYP" != "tmpfs" ]]; then
+		_scratch_cycle_mount
+		new_filelen=$(_get_filesize $test_file)
+		map_len=$(round_up_to_page_boundary $new_filelen)
+		csum_orig="$(_md5_checksum $test_file)"
+		mread $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
+		if grep -q 'Bus error' $tmp.err; then
+			echo "Not expecting SIGBUS when reading up to page boundary"
+			cat $tmp.err
+			_fail
+		fi
+
+		# This should just work
+		mread $test_file $map_len 0 $map_len >> $seqres.full  2>$tmp.err
+		if [[ $? -ne 0 ]]; then
+			_fail
+		fi
+
+		# If we mmap() on the boundary but try to read beyond it just
+		# fails, we don't get a SIGBUS
+		$XFS_IO_PROG -r $test_file \
+			-c "mmap -r 0 $map_len" \
+			-c "mread 0 $((map_len + 10))" >> $seqres.full  2>$tmp.err
+		local mread_err=$?
+		if [[ $mread_err -eq 0 ]]; then
+			echo "mmap() to page boundary works as expected but reading beyond should fail"
+			echo "err: $?"
+			_fail
+		fi
+
+		# Now let's go beyond the allowed mmap() page boundary
+		mread $test_file $((map_len + 10)) 0 $((map_len + 10)) >> $seqres.full  2>$tmp.err
+		if ! grep -q 'Bus error' $tmp.err; then
+			echo "Expected SIGBUS when mmap() reading beyond page boundary"
+			_fail
+		fi
+		local filelen_test=$(_get_filesize $test_file)
+		if [[ "$filelen_test" != "$new_filelen" ]]; then
+			echo "Expected file length: $new_filelen"
+			echo " Actual  file length: $filelen_test"
+			_fail
+		fi
+	fi
+}
+
+test_block_size()
+{
+	local block_size=$1
+
+	do_mmap_tests $block_size 512 3 5
+	do_mmap_tests $block_size 16k 0 $((16384+3))
+	do_mmap_tests $block_size 16k $((16384-10)) $((16384+20))
+	do_mmap_tests $block_size 64k 0 $((65536+3))
+	do_mmap_tests $block_size 4k 4090 30 true
+}
+
+_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
+_scratch_mount
+test_file=$SCRATCH_MNT/file
+block_size=$(_get_block_size "$SCRATCH_MNT")
+test_block_size $block_size
+_scratch_unmount
+_check_scratch_fs
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/740.out b/tests/generic/740.out
new file mode 100644
index 000000000000..3f841e600ed3
--- /dev/null
+++ b/tests/generic/740.out
@@ -0,0 +1,2 @@ 
+QA output created by 740
+Silence is golden