Re: [PATCH v3 07/28] fsverity: always use bitmap to track verified status

["Darrick J. Wong" <djwong@kernel.org>]

Hi Eric,

[Please excuse my ignorance, this is only the third time I've dived
into fsverity.]

On Thu, Oct 12, 2023 at 12:27:46AM -0700, Eric Biggers wrote:
> On Wed, Oct 11, 2023 at 03:03:55PM +0200, Andrey Albershteyn wrote:
> > > How complicated would it be to keep supporting using the page bit when
> > > merkle_tree_block_size == page_size and the filesystem supports it?  It's an
> > > efficient solution, so it would be a shame to lose it.  Also it doesn't have the
> > > max file size limit that the bitmap has.

How complex would it be to get rid of the bitmap entirely, and validate
all the verity tree blocks within a page all at once instead of
individual blocks within a page?

Assuming willy isn't grinding his axe to get rid of PGchecked,
obviously. ;)

> > Well, I think it's possible but my motivation was to step away from
> > page manipulation as much as possible with intent to not affect other
> > filesystems too much. I can probably add handling of this case to
> > fsverity_read_merkle_tree_block() but fs/verity still will create
> > bitmap and have a limit. The other way is basically revert changes
> > done in patch 09, then, it probably will be quite a mix of page/block
> > handling in fs/verity/verify.c
> 
> The page-based caching still has to be supported anyway, since that's what the
> other filesystems that support fsverity use, and it seems you don't plan to
> change that.

I frankly have been asking myself why /this/ patchset adds so much extra
code and flags and whatnot to XFS and fs/verity.  From what I can tell,
the xfs buffer cache has been extended to allocate double the memory so
that xattr contents can be shadowed.  getxattr for merkle tree contents
then pins the buffer, shadows the contents, and hands both back to the
caller (aka xfs_read_merkle_tree_block).   The shadow memory is then
handed to fs/verity to do its magic; following that, fsverity releases
the reference and we can eventually drop the xfs_buf reference.

But this seems way overcomplicated to me.  ->read_merkle_tree_page hands
us a pgoff_t and a suggestion for page readahead, and wants us to return
an uptodate locked page, right?

Why can't xfs allocate a page, walk the requested range to fill the page
with merkle tree blocks that were written to the xattr structure (or
zero the page contents if there is no xattr), and hand that page to
fsverity?  (It helps to provide the merkle tree block size to
xfs_read_merkle_tree_page, thanks for adding that).

Assuming fsverity also wants some caching, we could augment the
xfs_inode to point to a separate address_space for cached merkle tree
pages, and then xfs_read_merkle_tree_page can use __filemap_get_folio to
find uptodate cached pages, or instantiate one and make it uptodate.
Even better, we can pretty easily use multipage folios for this, though
AFAICT the fs/verity code isn't yet up to handling that.

The only thing I can't quite figure out is how to get memory reclaim to
scan the extra address_space when it wants to try to reclaim pages.
That part ext4 and f2fs got for free because they stuffed the merkle
tree in the posteof space.

But wouldn't that solve /all/ the plumbing problems without scattering
bits of new code and flags into the xfs buffer cache, the extended
attributes code, and elsewhere?  And then xfs would not need to burn up
vmap space to allocate 8K memory blocks just to provide 4k merkel tree
blocks to fs/verity.

That's just my 2 cents from spending a couple of hours hypothesizing how
I would fill out the fsverity_operations.

> The question is where the "block verified" flags should be stored.
> Currently there are two options: PG_checked and the separate bitmap.  I'm not
> yet convinced that removing the support for the PG_checked method is a good
> change.  PG_checked is a nice solution for the cases where it can be used; it
> requires no extra memory, no locking, and has no max file size.  Also, this
> change seems mostly orthogonal to what you're actually trying to accomplish.

That scheme sounds way better to me than the bitmap. ;)

--D

> > > > Also, this patch removes spinlock. The lock was used to reset bits
> > > > in bitmap belonging to one page. This patch works only with one
> > > > Merkle tree block and won't reset other blocks status.
> > > 
> > > The spinlock is needed when there are multiple Merkle tree blocks per page and
> > > the filesystem is using the page-based caching.  So I don't think you can remove
> > > it.  Can you elaborate on why you feel it can be removed?
> > 
> > With this patch is_hash_block_verified() doesn't reset bits for
> > blocks belonging to the same page. Even if page is re-instantiated
> > only one block is checked in this case. So, when other blocks are
> > checked they are reset.
> > 
> > 	if (block_cached)
> > 		return test_bit(hblock_idx, vi->hash_block_verified);
> 
> When part of the Merkle tree cache is evicted and re-instantiated, whether that
> part is a "page" or something else, the verified status of all the blocks
> contained in that part need to be invalidated so that they get re-verified.  The
> existing code does that.  I don't see how your proposed code does that.
> 
> - Eric