Re: [PATCH v8 2/5] common/verity: support btrfs in generic fsverity tests

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Apr 22, 2022 at 04:27:29PM -0700, Boris Burkov wrote:
> On Thu, Mar 17, 2022 at 06:16:45PM +0000, Eric Biggers wrote:
> > On Wed, Mar 16, 2022 at 01:25:12PM -0700, Boris Burkov wrote:
> > > generic/572-579 have tests for fsverity. Now that btrfs supports
> > > fsverity, make these tests function as well. For a majority of the tests
> > > that pass, simply adding the case to mkfs a btrfs filesystem with no
> > > extra options is sufficient.
> > > 
> > > However, generic/574 has tests for corrupting the merkle tree itself.
> > > Since btrfs uses a different scheme from ext4 and f2fs 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.
> > > 
> > > 576 does not run because btrfs does not support transparent encryption.
> > > 
> > > 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 | 23 +++++++++++++++++++++++
> > >  3 files changed, 29 insertions(+)
> > 
> > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > 
> > - Eric
> 
> Eric,
> 
> Unfortunately, I think I found a more serious problem with the
> compatibility of generic/574 and btrfs while working on testing the
> enable/disable sysctls.
> 
> I realized that I had forgotten to customize the mount options for btrfs
> to use "nodatasum" and as a result, this test was passing for btrfs
> inappropriately, since we were getting EIOs for failing data checksums,
> not verity checks.
> 
> I fixed the mount option issue only to realize that some of the test
> cases make assumptions that don't apply to btrfs. For example:
> "corruption_test 130999 131000 72"
> 
> Btrfs zeros pages past EOF in readpage before they make it to the user
> via read or mmap, and the fsverity check is done at that point, so
> corrupting the disk past EOF does not cause a verity failure (or get
> leaked to the user) but it does cause csum failures since those are done
> on bios, like verity checks in ext4. I verified that removing that
> zeroing in readpage makes the test case pass.
> 
> Do you have a preference for how I might fix this? My first thought is
> to try to factor out any such test cases into a new test with a new
> requires clause that btrfs fails but ext4/f2fs pass, kind of like what
> we did for the EFBIG test for future FSes that might not logically
> address the Merkle tree in the past-EOF space.

The reason that an error is expected for corruption past EOF in the block
containing EOF, is to ensure that the bad data isn't visible via mmap reads.  If
that's not a problem for btrfs due to it always zeroing the part past EOF, I
think it would be fine to change that test case (specifically the one with
parameters "corruption_test 130999 131000 72") to just try a mmap read of the
whole EOF block, and check that it either fails with an error *or* returns
zeroes.  I don't think that an entirely new test script would be warranted.

- Eric