Re: [PATCH 04/18] fsverity: support block-based Merkle tree caching

[Christoph Hellwig <hch@infradead.org>]

On Wed, May 01, 2024 at 03:35:19PM -0700, Darrick J. Wong wrote:
> Got a link?  This is the first I've heard of this, but TBH I've been
> ignoring a /lot/ of things trying to get online repair merged (thank
> you!) over the past months...

This was long before I got involved with repair :)

Below is what I found in my local tree.  It doesn't have a proper commit
log, so I probably only sent it out as a RFC in reply to a patch series
posting, most likely untested:

commit c11dcbe101a240c7a9e9bae7efaff2779d88b292
Author: Christoph Hellwig <hch@lst.de>
Date:   Mon Oct 16 14:14:11 2023 +0200

    fsverity block interface

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index af889512c6ac99..c616d530a89086 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -648,7 +648,7 @@ which verifies data that has been read into the pagecache of a verity
 inode.  The containing folio must still be locked and not Uptodate, so
 it's not yet readable by userspace.  As needed to do the verification,
 fsverity_verify_blocks() will call back into the filesystem to read
-hash blocks via fsverity_operations::read_merkle_tree_page().
+hash blocks via fsverity_operations::read_merkle_tree_block().
 
 fsverity_verify_blocks() returns false if verification failed; in this
 case, the filesystem must not set the folio Uptodate.  Following this,
diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
index 2b34796f68d349..4b6134923232e7 100644
--- a/fs/btrfs/verity.c
+++ b/fs/btrfs/verity.c
@@ -713,20 +713,20 @@ int btrfs_get_verity_descriptor(struct inode *inode, void *buf, size_t buf_size)
  *
  * Returns the page we read, or an ERR_PTR on error.
  */
-static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
-						pgoff_t index,
-						unsigned long num_ra_pages,
-						u8 log_blocksize)
+static int btrfs_read_merkle_tree_block(struct inode *inode,
+		unsigned int offset, struct fsverity_block *block,
+		unsigned long num_ra_pages)
 {
 	struct folio *folio;
+	pgoff_t index = offset >> PAGE_SHIFT;
 	u64 off = (u64)index << PAGE_SHIFT;
 	loff_t merkle_pos = merkle_file_pos(inode);
 	int ret;
 
 	if (merkle_pos < 0)
-		return ERR_PTR(merkle_pos);
+		return merkle_pos;
 	if (merkle_pos > inode->i_sb->s_maxbytes - off - PAGE_SIZE)
-		return ERR_PTR(-EFBIG);
+		return -EFBIG;
 	index += merkle_pos >> PAGE_SHIFT;
 again:
 	folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
@@ -739,7 +739,7 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 		if (!folio_test_uptodate(folio)) {
 			folio_unlock(folio);
 			folio_put(folio);
-			return ERR_PTR(-EIO);
+			return -EIO;
 		}
 		folio_unlock(folio);
 		goto out;
@@ -748,7 +748,7 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 	folio = filemap_alloc_folio(mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS),
 				    0);
 	if (!folio)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	ret = filemap_add_folio(inode->i_mapping, folio, index, GFP_NOFS);
 	if (ret) {
@@ -756,7 +756,7 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 		/* Did someone else insert a folio here? */
 		if (ret == -EEXIST)
 			goto again;
-		return ERR_PTR(ret);
+		return ret;
 	}
 
 	/*
@@ -769,7 +769,7 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 			     folio_address(folio), PAGE_SIZE, &folio->page);
 	if (ret < 0) {
 		folio_put(folio);
-		return ERR_PTR(ret);
+		return ret;
 	}
 	if (ret < PAGE_SIZE)
 		folio_zero_segment(folio, ret, PAGE_SIZE);
@@ -778,7 +778,8 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 	folio_unlock(folio);
 
 out:
-	return folio_file_page(folio, index);
+	return fsverity_set_block_page(block, folio_file_page(folio, index),
+				       offset);
 }
 
 /*
@@ -809,6 +810,7 @@ const struct fsverity_operations btrfs_verityops = {
 	.begin_enable_verity     = btrfs_begin_enable_verity,
 	.end_enable_verity       = btrfs_end_enable_verity,
 	.get_verity_descriptor   = btrfs_get_verity_descriptor,
-	.read_merkle_tree_page   = btrfs_read_merkle_tree_page,
+	.read_merkle_tree_block  = btrfs_read_merkle_tree_block,
 	.write_merkle_tree_block = btrfs_write_merkle_tree_block,
+	.drop_merkle_tree_block	 = fsverity_drop_page_merke_tree_block,
 };
diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index 4e2f01f048c09b..5623e2c1c302e8 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -358,15 +358,13 @@ static int ext4_get_verity_descriptor(struct inode *inode, void *buf,
 	return desc_size;
 }
 
-static struct page *ext4_read_merkle_tree_page(struct inode *inode,
-					       pgoff_t index,
-					       unsigned long num_ra_pages,
-					       u8 log_blocksize)
+static int ext4_read_merkle_tree_block(struct inode *inode, unsigned int offset,
+		struct fsverity_block *block, unsigned long num_ra_pages)
 {
 	struct folio *folio;
+	pgoff_t index;
 
-	index += ext4_verity_metadata_pos(inode) >> PAGE_SHIFT;
-
+	index = (ext4_verity_metadata_pos(inode) + offset) >> PAGE_SHIFT;
 	folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
 	if (IS_ERR(folio) || !folio_test_uptodate(folio)) {
 		DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
@@ -377,9 +375,10 @@ static struct page *ext4_read_merkle_tree_page(struct inode *inode,
 			page_cache_ra_unbounded(&ractl, num_ra_pages, 0);
 		folio = read_mapping_folio(inode->i_mapping, index, NULL);
 		if (IS_ERR(folio))
-			return ERR_CAST(folio);
+			return PTR_ERR(folio);
 	}
-	return folio_file_page(folio, index);
+	return fsverity_set_block_page(block, folio_file_page(folio, index),
+				       offset);
 }
 
 static int ext4_write_merkle_tree_block(struct inode *inode, const void *buf,
@@ -394,6 +393,7 @@ const struct fsverity_operations ext4_verityops = {
 	.begin_enable_verity	= ext4_begin_enable_verity,
 	.end_enable_verity	= ext4_end_enable_verity,
 	.get_verity_descriptor	= ext4_get_verity_descriptor,
-	.read_merkle_tree_page	= ext4_read_merkle_tree_page,
+	.read_merkle_tree_block	= ext4_read_merkle_tree_block,
 	.write_merkle_tree_block = ext4_write_merkle_tree_block,
+	.drop_merkle_tree_block	= fsverity_drop_page_merke_tree_block,
 };
diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
index 601ab9f0c02492..aac9281e9c4565 100644
--- a/fs/f2fs/verity.c
+++ b/fs/f2fs/verity.c
@@ -255,15 +255,13 @@ static int f2fs_get_verity_descriptor(struct inode *inode, void *buf,
 	return size;
 }
 
-static struct page *f2fs_read_merkle_tree_page(struct inode *inode,
-					       pgoff_t index,
-					       unsigned long num_ra_pages,
-					       u8 log_blocksize)
+static int f2fs_read_merkle_tree_block(struct inode *inode, unsigned int offset,
+		struct fsverity_block *block, unsigned long num_ra_pages)
 {
 	struct page *page;
+	pgoff_t index;
 
-	index += f2fs_verity_metadata_pos(inode) >> PAGE_SHIFT;
-
+	index = (f2fs_verity_metadata_pos(inode) + offset) >> PAGE_SHIFT;
 	page = find_get_page_flags(inode->i_mapping, index, FGP_ACCESSED);
 	if (!page || !PageUptodate(page)) {
 		DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
@@ -274,7 +272,7 @@ static struct page *f2fs_read_merkle_tree_page(struct inode *inode,
 			page_cache_ra_unbounded(&ractl, num_ra_pages, 0);
 		page = read_mapping_page(inode->i_mapping, index, NULL);
 	}
-	return page;
+	return fsverity_set_block_page(block, page, offset);
 }
 
 static int f2fs_write_merkle_tree_block(struct inode *inode, const void *buf,
@@ -289,6 +287,7 @@ const struct fsverity_operations f2fs_verityops = {
 	.begin_enable_verity	= f2fs_begin_enable_verity,
 	.end_enable_verity	= f2fs_end_enable_verity,
 	.get_verity_descriptor	= f2fs_get_verity_descriptor,
-	.read_merkle_tree_page	= f2fs_read_merkle_tree_page,
+	.read_merkle_tree_block	= f2fs_read_merkle_tree_block,
 	.write_merkle_tree_block = f2fs_write_merkle_tree_block,
+	.drop_merkle_tree_block	= fsverity_drop_page_merke_tree_block,
 };
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
index 182bddf5dec54c..5e362f8562bd5d 100644
--- a/fs/verity/read_metadata.c
+++ b/fs/verity/read_metadata.c
@@ -12,10 +12,33 @@
 #include <linux/sched/signal.h>
 #include <linux/uaccess.h>
 
+int fsverity_set_block_page(struct fsverity_block *block,
+		struct page *page, unsigned int index)
+{
+	if (IS_ERR(page))
+		return PTR_ERR(page);
+	block->kaddr = page_address(page) + (index % PAGE_SIZE);
+	block->cached = PageChecked(page);
+	block->context = page;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fsverity_set_block_page);
+
+void fsverity_drop_page_merke_tree_block(struct fsverity_block *block)
+{
+	struct page *page = block->context;
+
+	if (block->verified)
+		SetPageChecked(page);
+	put_page(page);
+}
+EXPORT_SYMBOL_GPL(fsverity_drop_page_merke_tree_block);
+
 static int fsverity_read_merkle_tree(struct inode *inode,
 				     const struct fsverity_info *vi,
 				     void __user *buf, u64 offset, int length)
 {
+	const struct fsverity_operations *vop = inode->i_sb->s_vop;
 	u64 end_offset;
 	unsigned int offs_in_block;
 	unsigned int block_size = vi->tree_params.block_size;
@@ -45,20 +68,19 @@ static int fsverity_read_merkle_tree(struct inode *inode,
 		struct fsverity_block block;
 
 		block.len = block_size;
-		if (fsverity_read_merkle_tree_block(inode,
-					index << vi->tree_params.log_blocksize,
-					&block, num_ra_pages)) {
-			fsverity_drop_block(inode, &block);
+		if (vop->read_merkle_tree_block(inode,
+				index << vi->tree_params.log_blocksize,
+				&block, num_ra_pages)) {
 			err = -EFAULT;
 			break;
 		}
 
 		if (copy_to_user(buf, block.kaddr + offs_in_block, bytes_to_copy)) {
-			fsverity_drop_block(inode, &block);
+			vop->drop_merkle_tree_block(&block);
 			err = -EFAULT;
 			break;
 		}
-		fsverity_drop_block(inode, &block);
+		vop->drop_merkle_tree_block(&block);
 		block.kaddr = NULL;
 
 		retval += bytes_to_copy;
diff --git a/fs/verity/verify.c b/fs/verity/verify.c
index dfe01f12184341..9b84262a6fa413 100644
--- a/fs/verity/verify.c
+++ b/fs/verity/verify.c
@@ -42,6 +42,7 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,
 		  const void *data, u64 data_pos, unsigned long max_ra_pages)
 {
 	const struct merkle_tree_params *params = &vi->tree_params;
+	const struct fsverity_operations *vop = inode->i_sb->s_vop;
 	const unsigned int hsize = params->digest_size;
 	int level;
 	int err;
@@ -115,9 +116,9 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,
 		block->len = params->block_size;
 		num_ra_pages = level == 0 ?
 			min(max_ra_pages, params->tree_pages - hpage_idx) : 0;
-		err = fsverity_read_merkle_tree_block(
-			inode, hblock_idx << params->log_blocksize, block,
-			num_ra_pages);
+		err = vop->read_merkle_tree_block(inode,
+				hblock_idx << params->log_blocksize, block,
+				num_ra_pages);
 		if (err) {
 			fsverity_err(inode,
 				     "Error %d reading Merkle tree block %lu",
@@ -127,7 +128,7 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,
 		if (is_hash_block_verified(vi, hblock_idx, block->cached)) {
 			memcpy(_want_hash, block->kaddr + hoffset, hsize);
 			want_hash = _want_hash;
-			fsverity_drop_block(inode, block);
+			vop->drop_merkle_tree_block(block);
 			goto descend;
 		}
 		hblocks[level].index = hblock_idx;
@@ -157,7 +158,7 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,
 		block->verified = true;
 		memcpy(_want_hash, haddr + hoffset, hsize);
 		want_hash = _want_hash;
-		fsverity_drop_block(inode, block);
+		vop->drop_merkle_tree_block(block);
 	}
 
 	/* Finally, verify the data block. */
@@ -174,9 +175,8 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,
 		     params->hash_alg->name, hsize, want_hash,
 		     params->hash_alg->name, hsize, real_hash);
 error:
-	for (; level > 0; level--) {
-		fsverity_drop_block(inode, &hblocks[level - 1].block);
-	}
+	for (; level > 0; level--)
+		vop->drop_merkle_tree_block(&hblocks[level - 1].block);
 	return false;
 }
 
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index ce37a430bc97f2..ae9ae7719af558 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -104,27 +104,6 @@ struct fsverity_operations {
 	int (*get_verity_descriptor)(struct inode *inode, void *buf,
 				     size_t bufsize);
 
-	/**
-	 * Read a Merkle tree page of the given inode.
-	 *
-	 * @inode: the inode
-	 * @index: 0-based index of the page within the Merkle tree
-	 * @num_ra_pages: The number of Merkle tree pages that should be
-	 *		  prefetched starting at @index if the page at @index
-	 *		  isn't already cached.  Implementations may ignore this
-	 *		  argument; it's only a performance optimization.
-	 *
-	 * This can be called at any time on an open verity file.  It may be
-	 * called by multiple processes concurrently, even with the same page.
-	 *
-	 * Note that this must retrieve a *page*, not necessarily a *block*.
-	 *
-	 * Return: the page on success, ERR_PTR() on failure
-	 */
-	struct page *(*read_merkle_tree_page)(struct inode *inode,
-					      pgoff_t index,
-					      unsigned long num_ra_pages,
-					      u8 log_blocksize);
 	/**
 	 * Read a Merkle tree block of the given inode.
 	 * @inode: the inode
@@ -162,13 +141,12 @@ struct fsverity_operations {
 
 	/**
 	 * Release the reference to a Merkle tree block
-	 *
-	 * @page: the block to release
+	 * @block: the block to release
 	 *
 	 * This is called when fs-verity is done with a block obtained with
 	 * ->read_merkle_tree_block().
 	 */
-	void (*drop_block)(struct fsverity_block *block);
+	void (*drop_merkle_tree_block)(struct fsverity_block *block);
 };
 
 #ifdef CONFIG_FS_VERITY
@@ -217,74 +195,16 @@ static inline void fsverity_cleanup_inode(struct inode *inode)
 
 int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg);
 
+int fsverity_set_block_page(struct fsverity_block *block,
+		struct page *page, unsigned int index);
+void fsverity_drop_page_merke_tree_block(struct fsverity_block *block);
+
 /* verify.c */
 
 bool fsverity_verify_blocks(struct folio *folio, size_t len, size_t offset);
 void fsverity_verify_bio(struct bio *bio);
 void fsverity_enqueue_verify_work(struct work_struct *work);
 
-/**
- * fsverity_drop_block() - drop block obtained with ->read_merkle_tree_block()
- * @inode: inode in use for verification or metadata reading
- * @block: block to be dropped
- *
- * Generic put_page() method. Calls out back to filesystem if ->drop_block() is
- * set, otherwise do nothing.
- *
- */
-static inline void fsverity_drop_block(struct inode *inode,
-		struct fsverity_block *block)
-{
-	if (inode->i_sb->s_vop->drop_block)
-		inode->i_sb->s_vop->drop_block(block);
-	else {
-		struct page *page = (struct page *)block->context;
-
-		if (block->verified)
-			SetPageChecked(page);
-
-		put_page(page);
-	}
-}
-
-/**
- * fsverity_read_block_from_page() - layer between fs using read page
- * and read block
- * @inode: inode in use for verification or metadata reading
- * @index: index of the block in the tree (offset into the tree)
- * @block: block to be read
- * @num_ra_pages: number of pages to readahead, may be ignored
- *
- * Depending on fs implementation use read_merkle_tree_block or
- * read_merkle_tree_page.
- */
-static inline int fsverity_read_merkle_tree_block(struct inode *inode,
-					unsigned int index,
-					struct fsverity_block *block,
-					unsigned long num_ra_pages)
-{
-	struct page *page;
-
-	if (inode->i_sb->s_vop->read_merkle_tree_block)
-		return inode->i_sb->s_vop->read_merkle_tree_block(
-			inode, index, block, num_ra_pages);
-
-	page = inode->i_sb->s_vop->read_merkle_tree_page(
-			inode, index >> PAGE_SHIFT, num_ra_pages,
-			block->len);
-
-	block->kaddr = page_address(page) + (index % PAGE_SIZE);
-	block->cached = PageChecked(page);
-	block->context = page;
-
-	if (IS_ERR(page))
-		return PTR_ERR(page);
-	else
-		return 0;
-}
-
-
-
 #else /* !CONFIG_FS_VERITY */
 
 static inline struct fsverity_info *fsverity_get_info(const struct inode *inode)
@@ -362,20 +282,6 @@ static inline void fsverity_enqueue_verify_work(struct work_struct *work)
 	WARN_ON_ONCE(1);
 }
 
-static inline void fsverity_drop_page(struct inode *inode, struct page *page)
-{
-	WARN_ON_ONCE(1);
-}
-
-static inline int fsverity_read_merkle_tree_block(struct inode *inode,
-					unsigned int index,
-					struct fsverity_block *block,
-					unsigned long num_ra_pages)
-{
-	WARN_ON_ONCE(1);
-	return -EOPNOTSUPP;
-}
-
 #endif	/* !CONFIG_FS_VERITY */
 
 static inline bool fsverity_verify_folio(struct folio *folio)