Re: [PATCH v7 2/5] btrfs: test btrfs specific fsverity corruption

[Boris Burkov <boris@bur.io>]

On Wed, Mar 16, 2022 at 05:26:59PM +0000, Eric Biggers wrote:
> On Tue, Mar 15, 2022 at 03:15:58PM -0700, Boris Burkov wrote:
> > There are some btrfs specific fsverity scenarios that don't map
> > neatly onto the tests in generic/574 like holes, inline extents,
> > and preallocated extents. Cover those in a btrfs specific test.
> > 
> > This test relies on the btrfs implementation of fsverity in the patch:
> > btrfs: initial fsverity support
> > 
> > and on btrfs-corrupt-block for corruption in the patches titled:
> > 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/btrfs        |   5 ++
> >  common/config       |   1 +
> >  common/verity       |  14 ++++
> >  tests/btrfs/290     | 168 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/290.out |  25 +++++++
> >  5 files changed, 213 insertions(+)
> >  create mode 100755 tests/btrfs/290
> >  create mode 100644 tests/btrfs/290.out
> > 
> > diff --git a/common/btrfs b/common/btrfs
> > index 670d9d1f..c3a7dc6e 100644
> > --- a/common/btrfs
> > +++ b/common/btrfs
> > @@ -511,3 +511,8 @@ _btrfs_metadump()
> >  	$BTRFS_IMAGE_PROG "$device" "$dumpfile"
> >  	[ -n "$DUMP_COMPRESSOR" ] && $DUMP_COMPRESSOR -f "$dumpfile" &> /dev/null
> >  }
> > +
> > +_require_btrfs_corrupt_block()
> > +{
> > +	_require_command "$BTRFS_CORRUPT_BLOCK_PROG" btrfs-corrupt-block
> > +}
> > diff --git a/common/config b/common/config
> > index 479e50d1..67bdf912 100644
> > --- a/common/config
> > +++ b/common/config
> > @@ -296,6 +296,7 @@ export BTRFS_UTIL_PROG=$(type -P btrfs)
> >  export BTRFS_SHOW_SUPER_PROG=$(type -P btrfs-show-super)
> >  export BTRFS_CONVERT_PROG=$(type -P btrfs-convert)
> >  export BTRFS_TUNE_PROG=$(type -P btrfstune)
> > +export BTRFS_CORRUPT_BLOCK_PROG=$(type -P btrfs-corrupt-block)
> >  export XFS_FSR_PROG=$(type -P xfs_fsr)
> >  export MKFS_NFS_PROG="false"
> >  export MKFS_CIFS_PROG="false"
> > diff --git a/common/verity b/common/verity
> > index 1afe4a82..77766fca 100644
> > --- a/common/verity
> > +++ b/common/verity
> > @@ -3,6 +3,8 @@
> >  #
> >  # Functions for setting up and testing fs-verity
> >  
> > +. common/btrfs
> > +
> >  _require_scratch_verity()
> >  {
> >  	_require_scratch
> > @@ -48,6 +50,15 @@ _require_scratch_verity()
> >  	FSV_BLOCK_SIZE=$(get_page_size)
> >  }
> >  
> > +# Check for userspace tools needed to corrupt verity data or metadata.
> > +_require_fsverity_corruption()
> > +{
> > +	_require_xfs_io_command "fiemap"
> > +	if [ $FSTYP == "btrfs" ]; then
> > +		_require_btrfs_corrupt_block
> > +	fi
> > +}
> 
> This is adding a second definition of _require_fsverity_corruption().
> Probably a rebase error.
> 
> Also, is this hunk in the right patch?  This patch is for adding btrfs/290;
> however, btrfs/290 doesn't use _require_fsverity_corruption() anymore.
> 
> > +
> >  # Check for CONFIG_FS_VERITY_BUILTIN_SIGNATURES=y, as well as the userspace
> >  # commands needed to generate certificates and add them to the kernel.
> >  _require_fsverity_builtin_signatures()
> > @@ -153,6 +164,9 @@ _scratch_mkfs_verity()
> >  	ext4|f2fs)
> >  		_scratch_mkfs -O verity
> >  		;;
> > +	btrfs)
> > +		_scratch_mkfs
> > +		;;
> 
> I think a good way to organize things would be to wire up the existing verity
> tests for btrfs first, then to add the btrfs-specific tests at thet end of the
> series.  That would mean the above hunk would go earlier in the series, not with
> btrfs/290.  It's a little hard to review as-is, as the different hunks needed to
> wire up the existing tests are mixed around in different patches.

I like that. I've definitely been struggling with getting all the hunks
in the right places.. I'll double check it all better before sending it
again. Thanks for the review,

Boris
> 
> - Eric