| Message ID | 20240612035334.134434-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [xfstests] generic/574: test corruption at more offsets | expand |
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>
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
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