diff mbox series

[4/4] fsverity: pass pos and size to ->write_merkle_tree_block

Message ID 20221214224304.145712-5-ebiggers@kernel.org (mailing list archive)
State New, archived
Headers show
Series fsverity cleanups | expand

Commit Message

Eric Biggers Dec. 14, 2022, 10:43 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

fsverity_operations::write_merkle_tree_block is passed the index of the
block to write and the log base 2 of the block size.  However, all
implementations of it use these parameters only to calculate the
position and the size of the block, in bytes.

Therefore, make ->write_merkle_tree_block take 'pos' and 'size'
parameters instead of 'index' and 'log_blocksize'.

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/btrfs/verity.c        | 19 +++++++------------
 fs/ext4/verity.c         |  6 +++---
 fs/f2fs/verity.c         |  6 +++---
 fs/verity/enable.c       |  4 ++--
 include/linux/fsverity.h |  8 ++++----
 5 files changed, 19 insertions(+), 24 deletions(-)

Comments

Eric Biggers Jan. 25, 2023, 6:12 p.m. UTC | #1 
[Added back Cc's.  Please use Reply-All instead of Reply!]

On Wed, Jan 25, 2023 at 01:22:27PM +0100, Andrey Albershteyn wrote:
> On Wed, Dec 14, 2022 at 02:43:04PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > fsverity_operations::write_merkle_tree_block is passed the index of the
> > block to write and the log base 2 of the block size.  However, all
> > implementations of it use these parameters only to calculate the
> > position and the size of the block, in bytes.
> > 
> > Therefore, make ->write_merkle_tree_block take 'pos' and 'size'
> > parameters instead of 'index' and 'log_blocksize'.
> 
> Hi Eric,
> 
> Thanks for the quick responses with changes to fs-verity!
> 
> With this patch shouldn't the read_merkle_tree_block() also change
> to [pos, size] args? I see that ext4 uses index to read the page at
> that index + file size. But I suppose that when Merkle tree block
> size will vary (e.g. 8k) it will require position + size.

Not yet.  It's actually read_merkle_tree_page(), not read_merkle_tree_block().
The callees want a page index, and pages always have size PAGE_SIZE.  So the
current function prototype is appropriate for the current design.

> In XFS as we store the page under the xattr with "pos" as a name
> we also need a "pos" while reading the page. So, currently XFS can
> use index << log2(PAGE_SHIFT) or will need to get also log_blocksize
> from descriptor.

But you definitely need to think about what changes should be made to allow XFS
to do the Merkle tree caching the way the other XFS developers want it to do.
There will be significantly more to that than potentially changing a function
prototype.  There's been some discussion of this on the "fs-verity support for
XFS" thread, but there's not a detailed proposal yet.

Note: you should store Merkle tree blocks in the xattrs instead of "pages", so
that the on-disk format isn't tied to the page size.

- Eric
diff mbox series

Patch

diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
index bf9eb693a6a7..c5ff16f9e9fa 100644
--- a/fs/btrfs/verity.c
+++ b/fs/btrfs/verity.c
@@ -783,30 +783,25 @@  static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 /*
  * fsverity op that writes a Merkle tree block into the btree.
  *
- * @inode:          inode to write a Merkle tree block for
- * @buf:            Merkle tree data block to write
- * @index:          index of the block in the Merkle tree
- * @log_blocksize:  log base 2 of the Merkle tree block size
- *
- * Note that the block size could be different from the page size, so it is not
- * safe to assume that index is a page index.
+ * @inode:	inode to write a Merkle tree block for
+ * @buf:	Merkle tree block to write
+ * @pos:	the position of the block in the Merkle tree (in bytes)
+ * @size:	the Merkle tree block size (in bytes)
  *
  * Returns 0 on success or negative error code on failure
  */
 static int btrfs_write_merkle_tree_block(struct inode *inode, const void *buf,
-					u64 index, int log_blocksize)
+					 u64 pos, unsigned int size)
 {
-	u64 off = index << log_blocksize;
-	u64 len = 1ULL << log_blocksize;
 	loff_t merkle_pos = merkle_file_pos(inode);
 
 	if (merkle_pos < 0)
 		return merkle_pos;
-	if (merkle_pos > inode->i_sb->s_maxbytes - off - len)
+	if (merkle_pos > inode->i_sb->s_maxbytes - pos - size)
 		return -EFBIG;
 
 	return write_key_bytes(BTRFS_I(inode), BTRFS_VERITY_MERKLE_ITEM_KEY,
-			       off, buf, len);
+			       pos, buf, size);
 }
 
 const struct fsverity_operations btrfs_verityops = {
diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index 30e3b65798b5..e4da1704438e 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -381,11 +381,11 @@  static struct page *ext4_read_merkle_tree_page(struct inode *inode,
 }
 
 static int ext4_write_merkle_tree_block(struct inode *inode, const void *buf,
-					u64 index, int log_blocksize)
+					u64 pos, unsigned int size)
 {
-	loff_t pos = ext4_verity_metadata_pos(inode) + (index << log_blocksize);
+	pos += ext4_verity_metadata_pos(inode);
 
-	return pagecache_write(inode, buf, 1 << log_blocksize, pos);
+	return pagecache_write(inode, buf, size, pos);
 }
 
 const struct fsverity_operations ext4_verityops = {
diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
index c352fff88a5e..f320ed8172ec 100644
--- a/fs/f2fs/verity.c
+++ b/fs/f2fs/verity.c
@@ -276,11 +276,11 @@  static struct page *f2fs_read_merkle_tree_page(struct inode *inode,
 }
 
 static int f2fs_write_merkle_tree_block(struct inode *inode, const void *buf,
-					u64 index, int log_blocksize)
+					u64 pos, unsigned int size)
 {
-	loff_t pos = f2fs_verity_metadata_pos(inode) + (index << log_blocksize);
+	pos += f2fs_verity_metadata_pos(inode);
 
-	return pagecache_write(inode, buf, 1 << log_blocksize, pos);
+	return pagecache_write(inode, buf, size, pos);
 }
 
 const struct fsverity_operations f2fs_verityops = {
diff --git a/fs/verity/enable.c b/fs/verity/enable.c
index df6b499bf6a1..a949ce817202 100644
--- a/fs/verity/enable.c
+++ b/fs/verity/enable.c
@@ -120,8 +120,8 @@  static int build_merkle_tree_level(struct file *filp, unsigned int level,
 			       params->block_size - pending_size);
 			err = vops->write_merkle_tree_block(inode,
 					pending_hashes,
-					dst_block_num,
-					params->log_blocksize);
+					dst_block_num << params->log_blocksize,
+					params->block_size);
 			if (err) {
 				fsverity_err(inode,
 					     "Error %d writing Merkle tree block %llu",
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 203f4962c54a..f5ed7ecfd9ab 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -109,9 +109,9 @@  struct fsverity_operations {
 	 * Write a Merkle tree block to the given inode.
 	 *
 	 * @inode: the inode for which the Merkle tree is being built
-	 * @buf: block to write
-	 * @index: 0-based index of the block within the Merkle tree
-	 * @log_blocksize: log base 2 of the Merkle tree block size
+	 * @buf: the Merkle tree block to write
+	 * @pos: the position of the block in the Merkle tree (in bytes)
+	 * @size: the Merkle tree block size (in bytes)
 	 *
 	 * This is only called between ->begin_enable_verity() and
 	 * ->end_enable_verity().
@@ -119,7 +119,7 @@  struct fsverity_operations {
 	 * Return: 0 on success, -errno on failure
 	 */
 	int (*write_merkle_tree_block)(struct inode *inode, const void *buf,
-				       u64 index, int log_blocksize);
+				       u64 pos, unsigned int size);
 };
 
 #ifdef CONFIG_FS_VERITY