[xfstests PATCH] generic/574: test corruption at more offsets

[xfstests PATCH] generic/574: test corruption at more offsets

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Expand generic/574 to test for corruption in more different parts of the
file to try to exercise any hashing optimizations that might be used.

There is no existing bug that this finds.  This is just to prevent
future bugs, considering optimizations along the lines of
https://lore.kernel.org/fsverity/20240611034822.36603-7-ebiggers@kernel.org/

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 tests/generic/574 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tests/generic/574 b/tests/generic/574
index cb42baaa..72440d49 100755
--- a/tests/generic/574
+++ b/tests/generic/574
@@ -194,10 +194,25 @@ test_block_size()
 	corruption_test $block_size 131072 0 5
 	corruption_test $block_size 131072 4091 5
 	corruption_test $block_size 131072 65536 65536
 	corruption_test $block_size 131072 131067 5
 
+	# Test corrupting a block in files of length 1..4 blocks, and test
+	# corrupting each block of a 4-block file.  This ensures that all code
+	# paths that might exist due to multi-block hashing optimizations the
+	# fsverity implementation may use get covered, assuming no more than 4
+	# blocks are hashed at once.  E.g., consider an fsverity implementation
+	# that verifies sets of blocks but has a bug when given a single block,
+	# or that has a bug that makes it not verify all the blocks of each set.
+	local i
+	for i in $(seq 1 4); do
+		corruption_test $block_size $((i*block_size)) $((block_size/2)) 5
+	done
+	for i in $(seq 0 3); do
+		corruption_test $block_size $((4*block_size)) $((i*block_size)) 5
+	done
+
 	corrupt_eof_block_test $block_size 130999 72
 
 	# Merkle tree corruption.
 	corruption_test $block_size 200000 100 10 true
 

base-commit: e46fa3a7dae4a65fd80128bf381dba4fd5036ebb
-- 
2.45.2

Re: [xfstests PATCH] generic/574: test corruption at more offsets

[Andrey Albershteyn]

On 2024-06-11 20:53:34, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Expand generic/574 to test for corruption in more different parts of the
> file to try to exercise any hashing optimizations that might be used.
> 
> There is no existing bug that this finds.  This is just to prevent
> future bugs, considering optimizations along the lines of
> https://lore.kernel.org/fsverity/20240611034822.36603-7-ebiggers@kernel.org/
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  tests/generic/574 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tests/generic/574 b/tests/generic/574
> index cb42baaa..72440d49 100755
> --- a/tests/generic/574
> +++ b/tests/generic/574
> @@ -194,10 +194,25 @@ test_block_size()
>  	corruption_test $block_size 131072 0 5
>  	corruption_test $block_size 131072 4091 5
>  	corruption_test $block_size 131072 65536 65536
>  	corruption_test $block_size 131072 131067 5
>  
> +	# Test corrupting a block in files of length 1..4 blocks, and test
> +	# corrupting each block of a 4-block file.  This ensures that all code
> +	# paths that might exist due to multi-block hashing optimizations the
> +	# fsverity implementation may use get covered, assuming no more than 4
> +	# blocks are hashed at once.  E.g., consider an fsverity implementation
> +	# that verifies sets of blocks but has a bug when given a single block,
> +	# or that has a bug that makes it not verify all the blocks of each set.
> +	local i
> +	for i in $(seq 1 4); do
> +		corruption_test $block_size $((i*block_size)) $((block_size/2)) 5
> +	done
> +	for i in $(seq 0 3); do
> +		corruption_test $block_size $((4*block_size)) $((i*block_size)) 5
> +	done
> +
>  	corrupt_eof_block_test $block_size 130999 72
>  
>  	# Merkle tree corruption.
>  	corruption_test $block_size 200000 100 10 true
>  
> 
> base-commit: e46fa3a7dae4a65fd80128bf381dba4fd5036ebb
> -- 
> 2.45.2
> 
> 

The addition looks fine to me, why is it 4 blocks though, isn't 2
enough?

Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

-- 
- Andrey

Re: [xfstests PATCH] generic/574: test corruption at more offsets

[Eric Biggers]

On Wed, Jun 19, 2024 at 02:49:23PM +0200, Andrey Albershteyn wrote:
> On 2024-06-11 20:53:34, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Expand generic/574 to test for corruption in more different parts of the
> > file to try to exercise any hashing optimizations that might be used.
> > 
> > There is no existing bug that this finds.  This is just to prevent
> > future bugs, considering optimizations along the lines of
> > https://lore.kernel.org/fsverity/20240611034822.36603-7-ebiggers@kernel.org/
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  tests/generic/574 | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/tests/generic/574 b/tests/generic/574
> > index cb42baaa..72440d49 100755
> > --- a/tests/generic/574
> > +++ b/tests/generic/574
> > @@ -194,10 +194,25 @@ test_block_size()
> >  	corruption_test $block_size 131072 0 5
> >  	corruption_test $block_size 131072 4091 5
> >  	corruption_test $block_size 131072 65536 65536
> >  	corruption_test $block_size 131072 131067 5
> >  
> > +	# Test corrupting a block in files of length 1..4 blocks, and test
> > +	# corrupting each block of a 4-block file.  This ensures that all code
> > +	# paths that might exist due to multi-block hashing optimizations the
> > +	# fsverity implementation may use get covered, assuming no more than 4
> > +	# blocks are hashed at once.  E.g., consider an fsverity implementation
> > +	# that verifies sets of blocks but has a bug when given a single block,
> > +	# or that has a bug that makes it not verify all the blocks of each set.
> > +	local i
> > +	for i in $(seq 1 4); do
> > +		corruption_test $block_size $((i*block_size)) $((block_size/2)) 5
> > +	done
> > +	for i in $(seq 0 3); do
> > +		corruption_test $block_size $((4*block_size)) $((i*block_size)) 5
> > +	done
> > +
> >  	corrupt_eof_block_test $block_size 130999 72
> >  
> >  	# Merkle tree corruption.
> >  	corruption_test $block_size 200000 100 10 true
> >  
> > 
> > base-commit: e46fa3a7dae4a65fd80128bf381dba4fd5036ebb
> > -- 
> > 2.45.2
> > 
> > 
> 
> The addition looks fine to me, why is it 4 blocks though, isn't 2
> enough?
> 
> Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
> 

For the currently proposed fsverity patch, 2 is enough.  The same approach could
be extended to more blocks later, though.  So using something a bit higher in
the test from the beginning makes sense.

- Eric