diff mbox series

[v2,2/3] generic/574: corrupt btrfs merkle tree data

Message ID ca320cd0c8427458828cc36d5d5168bbe6b6bab2.1617906318.git.boris@bur.io (mailing list archive)
State Superseded
Headers show
Series tests for btrfs fsverity | expand

Commit Message

Boris Burkov April 8, 2021, 6:30 p.m. UTC
generic/574 has tests for corrupting the merkle tree data stored by the
filesystem. Since btrfs uses a different scheme for storing this data,
the existing logic for corrupting it doesn't work out of the box. Adapt
it to properly corrupt btrfs merkle items.

Note that there is a bit of a kludge here: since btrfs_corrupt_block
doesn't handle streaming corruption bytes from stdin (I could change
that, but it feels like overkill for this purpose), I just read the
first corruption byte and duplicate it for the desired length. That is
how the test is using the interface in practice, anyway.

This test relies on the btrfs implementation of fsverity in:
btrfs: add compat_flags to btrfs_inode_item
btrfs: initial fsverity support
btrfs: check verity for reads of inline extents and holes
btrfs: fallback to buffered io for verity files

It also relies on the btrfs fiemap fix in:
btrfs: return whole extents in fiemap

and it relies on btrfs-corrupt-block for corruption, with the
following btrfs-progs patches:
btrfs-progs: corrupt generic item data with btrfs-corrupt-block
btrfs-progs: expand corrupt_file_extent in btrfs-corrupt-block

Signed-off-by: Boris Burkov <boris@bur.io>
---
 common/verity | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Eric Biggers April 8, 2021, 6:41 p.m. UTC | #1
On Thu, Apr 08, 2021 at 11:30:12AM -0700, Boris Burkov wrote:
> 
> Note that there is a bit of a kludge here: since btrfs_corrupt_block
> doesn't handle streaming corruption bytes from stdin (I could change
> that, but it feels like overkill for this purpose), I just read the
> first corruption byte and duplicate it for the desired length. That is
> how the test is using the interface in practice, anyway.

If that's the problem, couldn't you just write the data to a temporary file?

- Eric
Boris Burkov April 8, 2021, 6:49 p.m. UTC | #2
On Thu, Apr 08, 2021 at 11:41:42AM -0700, Eric Biggers wrote:
> On Thu, Apr 08, 2021 at 11:30:12AM -0700, Boris Burkov wrote:
> > 
> > Note that there is a bit of a kludge here: since btrfs_corrupt_block
> > doesn't handle streaming corruption bytes from stdin (I could change
> > that, but it feels like overkill for this purpose), I just read the
> > first corruption byte and duplicate it for the desired length. That is
> > how the test is using the interface in practice, anyway.
> 
> If that's the problem, couldn't you just write the data to a temporary file?

Sorry, I was a bit too vague. It doesn't have a file or stdin interface,
as far as I know.

btrfs-corrupt-block has your typical "kitchen sink of flags" interface and
doesn't currently read input from streams/files. I extended that
interface in the simplest way to support arbitrary corruption, which
didn't fit with the stream based corruption this test does.

my options seem to be:
shoehorn the "byte, length" interface into this test or
shoehorn the "stream corruption input in" interface into
btrfs-corrupt-block.

I have no problem with either, the former was just less work because I
already wrote it that way. If the junk I did here is a deal-breaker, I
don't mind modifying btrfs-corrupt-block.

> 
> - Eric

Thanks for the quick review,
Boris
Eric Biggers April 8, 2021, 11:16 p.m. UTC | #3
On Thu, Apr 08, 2021 at 11:49:37AM -0700, Boris Burkov wrote:
> On Thu, Apr 08, 2021 at 11:41:42AM -0700, Eric Biggers wrote:
> > On Thu, Apr 08, 2021 at 11:30:12AM -0700, Boris Burkov wrote:
> > > 
> > > Note that there is a bit of a kludge here: since btrfs_corrupt_block
> > > doesn't handle streaming corruption bytes from stdin (I could change
> > > that, but it feels like overkill for this purpose), I just read the
> > > first corruption byte and duplicate it for the desired length. That is
> > > how the test is using the interface in practice, anyway.
> > 
> > If that's the problem, couldn't you just write the data to a temporary file?
> 
> Sorry, I was a bit too vague. It doesn't have a file or stdin interface,
> as far as I know.
> 
> btrfs-corrupt-block has your typical "kitchen sink of flags" interface and
> doesn't currently read input from streams/files. I extended that
> interface in the simplest way to support arbitrary corruption, which
> didn't fit with the stream based corruption this test does.
> 
> my options seem to be:
> shoehorn the "byte, length" interface into this test or
> shoehorn the "stream corruption input in" interface into
> btrfs-corrupt-block.
> 
> I have no problem with either, the former was just less work because I
> already wrote it that way. If the junk I did here is a deal-breaker, I
> don't mind modifying btrfs-corrupt-block.
> 

If it's a lot of trouble to handle arbitrary data, then I think you should
change _fsv_scratch_corrupt_merkle_tree() to actually take a (byte, length) pair
instead of data on stdin.  Otherwise, _fsv_scratch_corrupt_merkle_tree() would
claim to do one thing but actually would do a different thing on one specific
filesystem.

- Eric
Boris Burkov April 9, 2021, 1:11 a.m. UTC | #4
On Thu, Apr 08, 2021 at 04:16:28PM -0700, Eric Biggers wrote:
> On Thu, Apr 08, 2021 at 11:49:37AM -0700, Boris Burkov wrote:
> > On Thu, Apr 08, 2021 at 11:41:42AM -0700, Eric Biggers wrote:
> > > On Thu, Apr 08, 2021 at 11:30:12AM -0700, Boris Burkov wrote:
> > > > 
> > > > Note that there is a bit of a kludge here: since btrfs_corrupt_block
> > > > doesn't handle streaming corruption bytes from stdin (I could change
> > > > that, but it feels like overkill for this purpose), I just read the
> > > > first corruption byte and duplicate it for the desired length. That is
> > > > how the test is using the interface in practice, anyway.
> > > 
> > > If that's the problem, couldn't you just write the data to a temporary file?
> > 
> > Sorry, I was a bit too vague. It doesn't have a file or stdin interface,
> > as far as I know.
> > 
> > btrfs-corrupt-block has your typical "kitchen sink of flags" interface and
> > doesn't currently read input from streams/files. I extended that
> > interface in the simplest way to support arbitrary corruption, which
> > didn't fit with the stream based corruption this test does.
> > 
> > my options seem to be:
> > shoehorn the "byte, length" interface into this test or
> > shoehorn the "stream corruption input in" interface into
> > btrfs-corrupt-block.
> > 
> > I have no problem with either, the former was just less work because I
> > already wrote it that way. If the junk I did here is a deal-breaker, I
> > don't mind modifying btrfs-corrupt-block.
> > 
> 
> If it's a lot of trouble to handle arbitrary data, then I think you should
> change _fsv_scratch_corrupt_merkle_tree() to actually take a (byte, length) pair
> instead of data on stdin.  Otherwise, _fsv_scratch_corrupt_merkle_tree() would
> claim to do one thing but actually would do a different thing on one specific
> filesystem.
> 
> - Eric

It's no trouble. I think reading in corruption bytes will be a better
interface for btrfs-corrupt-block so I'll go ahead and do it.
diff mbox series

Patch

diff --git a/common/verity b/common/verity
index c0b0c55d..333526ac 100644
--- a/common/verity
+++ b/common/verity
@@ -3,8 +3,7 @@ 
 #
 # Functions for setting up and testing fs-verity
 
-_require_scratch_verity()
-{
+_require_scratch_verity() {
 	_require_scratch
 	_require_command "$FSVERITY_PROG" fsverity
 
@@ -244,6 +243,18 @@  _fsv_scratch_corrupt_merkle_tree()
 		(( offset += ($(_get_filesize $file) + 65535) & ~65535 ))
 		_fsv_scratch_corrupt_bytes $file $offset
 		;;
+	btrfs)
+		ino=$(ls -i $file | awk '{print $1}')
+		sync
+		cat > $tmp.bytes
+		sz=$(_get_filesize $tmp.bytes)
+		read -n 1 byte < $tmp.bytes
+		ascii=$(printf "%d" "'$byte'")
+		_scratch_unmount
+		$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b $sz $SCRATCH_DEV
+		sync
+		_scratch_mount
+		;;
 	*)
 		_fail "_fsv_scratch_corrupt_merkle_tree() unimplemented on $FSTYP"
 		;;