fsverity cleanups

PATCH In review Eric Biggers 2 participants 8 messages Dec 14, 2022 – Jan 25, 2023 1 message not in the archive

Subsystems (5)

Btrfs file system ext4 file system F2FS file system fscrypt: file system level encryption support +1 more subsystems

lore

Share & tools

lore

Eric Biggers

author maintainer

2022-12-14 22:45 UTC #0 lore raw

cover fsverity cleanups

This series implements a few cleanups that have been suggested in the thread "[RFC PATCH 00/11] fs-verity support for XFS"
(https://lore.kernel.org/linux-fsdevel/[email protected]/T/#u).

This applies to mainline (commit 93761c93e9da).

Eric Biggers (4):
fsverity: optimize fsverity_file_open() on non-verity files
fsverity: optimize fsverity_prepare_setattr() on non-verity files
fsverity: optimize fsverity_cleanup_inode() on non-verity files
fsverity: pass pos and size to ->write_merkle_tree_block

 fs/btrfs/verity.c        | 19 ++++-------
 fs/ext4/verity.c         |  6 ++--
 fs/f2fs/verity.c         |  6 ++--
 fs/verity/enable.c       |  4 +--
 fs/verity/open.c         | 46 ++++---------------------
 include/linux/fsverity.h | 74 +++++++++++++++++++++++++++++++++-------
 6 files changed, 84 insertions(+), 71 deletions(-)

base-commit: 93761c93e9da28d8a020777cee2a84133082b477

Signature · 1 line

2.38.1

Eric Biggers

author maintainer

2022-12-14 22:45 UTC #1 lore raw

PATCH 1/4 fsverity: optimize fsverity_file_open() on non-verity files

From: Eric Biggers <[email protected]>

Make fsverity_file_open() an inline function that does the IS_VERITY() check, then (if needed) calls __fsverity_file_open() to do the real work. This reduces the overhead on non-verity files.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/verity/open.c         | 20 ++------------------
 include/linux/fsverity.h | 26 +++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 21 deletions(-)

fs/verity/open.c +2 ~~-18~~

index 81ff94442f7b..673d6db9abdf 100644
@@ -325,24 +325,8 @@ static int ensure_verity_info(struct inode *inode)
 	return err;
 }
 
-/**
- * fsverity_file_open() - prepare to open a verity file
- * @inode: the inode being opened
- * @filp: the struct file being set up
- *
- * When opening a verity file, deny the open if it is for writing.  Otherwise,
- * set up the inode's ->i_verity_info if not already done.
- *
- * When combined with fscrypt, this must be called after fscrypt_file_open().
- * Otherwise, we won't have the key set up to decrypt the verity metadata.
- *
- * Return: 0 on success, -errno on failure
- */
-int fsverity_file_open(struct inode *inode, struct file *filp)
+int __fsverity_file_open(struct inode *inode, struct file *filp)
 {
-	if (!IS_VERITY(inode))
-		return 0;
-
 	if (filp->f_mode & FMODE_WRITE) {
 		pr_debug("Denying opening verity file (ino %lu) for write\n",
 			 inode->i_ino);

@@ -351,7 +335,7 @@ int fsverity_file_open(struct inode *inode, struct file *filp)
 
 	return ensure_verity_info(inode);
 }
-EXPORT_SYMBOL_GPL(fsverity_file_open);
+EXPORT_SYMBOL_GPL(__fsverity_file_open);
 
 /**
  * fsverity_prepare_setattr() - prepare to change a verity inode's attributes

include/linux/fsverity.h +23 -3

index 40f14e5fed9d..326bf2e2b903 100644
@@ -148,7 +148,7 @@ int fsverity_get_digest(struct inode *inode,
 
 /* open.c */
 
-int fsverity_file_open(struct inode *inode, struct file *filp);
+int __fsverity_file_open(struct inode *inode, struct file *filp);
 int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
 void fsverity_cleanup_inode(struct inode *inode);

@@ -193,9 +193,9 @@ static inline int fsverity_get_digest(struct inode *inode,
 
 /* open.c */
 
-static inline int fsverity_file_open(struct inode *inode, struct file *filp)
+static inline int __fsverity_file_open(struct inode *inode, struct file *filp)
 {
-	return IS_VERITY(inode) ? -EOPNOTSUPP : 0;
+	return -EOPNOTSUPP;
 }
 
 static inline int fsverity_prepare_setattr(struct dentry *dentry,

@@ -254,4 +254,24 @@ static inline bool fsverity_active(const struct inode *inode)
 	return fsverity_get_info(inode) != NULL;
 }
 
+/**
+ * fsverity_file_open() - prepare to open a verity file
+ * @inode: the inode being opened
+ * @filp: the struct file being set up
+ *
+ * When opening a verity file, deny the open if it is for writing.  Otherwise,
+ * set up the inode's ->i_verity_info if not already done.
+ *
+ * When combined with fscrypt, this must be called after fscrypt_file_open().
+ * Otherwise, we won't have the key set up to decrypt the verity metadata.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static inline int fsverity_file_open(struct inode *inode, struct file *filp)
+{
+	if (IS_VERITY(inode))
+		return __fsverity_file_open(inode, filp);
+	return 0;
+}
+
 #endif	/* _LINUX_FSVERITY_H */

Signature · 1 line

2.38.1

Eric Biggers

author maintainer

2022-12-14 22:45 UTC #2 lore raw

PATCH 2/4 fsverity: optimize fsverity_prepare_setattr() on non-verity files

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Make fsverity_prepare_setattr() an inline function that does the IS_VERITY() check, then (if needed) calls __fsverity_prepare_setattr() to do the real work. This reduces the overhead on non-verity files.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/verity/open.c         | 16 +++-------------
 include/linux/fsverity.h | 26 ++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 17 deletions(-)

fs/verity/open.c +3 ~~-13~~

index 673d6db9abdf..e1e531d5e09a 100644
@@ -337,26 +337,16 @@ int __fsverity_file_open(struct inode *inode, struct file *filp)
 }
 EXPORT_SYMBOL_GPL(__fsverity_file_open);
 
-/**
- * fsverity_prepare_setattr() - prepare to change a verity inode's attributes
- * @dentry: dentry through which the inode is being changed
- * @attr: attributes to change
- *
- * Verity files are immutable, so deny truncates.  This isn't covered by the
- * open-time check because sys_truncate() takes a path, not a file descriptor.
- *
- * Return: 0 on success, -errno on failure
- */
-int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr)
+int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	if (IS_VERITY(d_inode(dentry)) && (attr->ia_valid & ATTR_SIZE)) {
+	if (attr->ia_valid & ATTR_SIZE) {
 		pr_debug("Denying truncate of verity file (ino %lu)\n",
 			 d_inode(dentry)->i_ino);
 		return -EPERM;
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(fsverity_prepare_setattr);
+EXPORT_SYMBOL_GPL(__fsverity_prepare_setattr);
 
 /**
  * fsverity_cleanup_inode() - free the inode's verity info, if present

include/linux/fsverity.h +22 -4

index 326bf2e2b903..84b498fff7ec 100644
@@ -149,7 +149,7 @@ int fsverity_get_digest(struct inode *inode,
 /* open.c */
 
 int __fsverity_file_open(struct inode *inode, struct file *filp);
-int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
+int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
 void fsverity_cleanup_inode(struct inode *inode);
 
 /* read_metadata.c */

@@ -198,10 +198,10 @@ static inline int __fsverity_file_open(struct inode *inode, struct file *filp)
 	return -EOPNOTSUPP;
 }
 
-static inline int fsverity_prepare_setattr(struct dentry *dentry,
-					   struct iattr *attr)
+static inline int __fsverity_prepare_setattr(struct dentry *dentry,
+					     struct iattr *attr)
 {
-	return IS_VERITY(d_inode(dentry)) ? -EOPNOTSUPP : 0;
+	return -EOPNOTSUPP;
 }
 
 static inline void fsverity_cleanup_inode(struct inode *inode)

@@ -274,4 +274,22 @@ static inline int fsverity_file_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/**
+ * fsverity_prepare_setattr() - prepare to change a verity inode's attributes
+ * @dentry: dentry through which the inode is being changed
+ * @attr: attributes to change
+ *
+ * Verity files are immutable, so deny truncates.  This isn't covered by the
+ * open-time check because sys_truncate() takes a path, not a file descriptor.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static inline int fsverity_prepare_setattr(struct dentry *dentry,
+					   struct iattr *attr)
+{
+	if (IS_VERITY(d_inode(dentry)))
+		return __fsverity_prepare_setattr(dentry, attr);
+	return 0;
+}
+
 #endif	/* _LINUX_FSVERITY_H */

Signature · 1 line

2.38.1

Eric Biggers

author maintainer

2022-12-14 22:45 UTC #3 lore raw

PATCH 3/4 fsverity: optimize fsverity_cleanup_inode() on non-verity files

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Make fsverity_cleanup_inode() an inline function that checks for non-NULL ->i_verity_info, then (if needed) calls __fsverity_cleanup_inode() to do the real work. This reduces the overhead on non-verity files.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/verity/open.c         | 10 ++--------
 include/linux/fsverity.h | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 9 deletions(-)

fs/verity/open.c +2 -8

index e1e531d5e09a..c723a62841db 100644
@@ -348,18 +348,12 @@ int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr)
 }
 EXPORT_SYMBOL_GPL(__fsverity_prepare_setattr);
 
-/**
- * fsverity_cleanup_inode() - free the inode's verity info, if present
- * @inode: an inode being evicted
- *
- * Filesystems must call this on inode eviction to free ->i_verity_info.
- */
-void fsverity_cleanup_inode(struct inode *inode)
+void __fsverity_cleanup_inode(struct inode *inode)
 {
 	fsverity_free_info(inode->i_verity_info);
 	inode->i_verity_info = NULL;
 }
-EXPORT_SYMBOL_GPL(fsverity_cleanup_inode);
+EXPORT_SYMBOL_GPL(__fsverity_cleanup_inode);
 
 int __init fsverity_init_info_cache(void)
 {

include/linux/fsverity.h +13 -1

index 84b498fff7ec..203f4962c54a 100644
@@ -150,7 +150,19 @@ int fsverity_get_digest(struct inode *inode,
 
 int __fsverity_file_open(struct inode *inode, struct file *filp);
 int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
-void fsverity_cleanup_inode(struct inode *inode);
+void __fsverity_cleanup_inode(struct inode *inode);
+
+/**
+ * fsverity_cleanup_inode() - free the inode's verity info, if present
+ * @inode: an inode being evicted
+ *
+ * Filesystems must call this on inode eviction to free ->i_verity_info.
+ */
+static inline void fsverity_cleanup_inode(struct inode *inode)
+{
+	if (inode->i_verity_info)
+		__fsverity_cleanup_inode(inode);
+}
 
 /* read_metadata.c */

Signature · 1 line

2.38.1

Eric Biggers

author maintainer

2022-12-14 22:45 UTC #4 lore raw

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

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

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'.

Signed-off-by Eric Biggers <[email protected]>

---
 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(-)

fs/btrfs/verity.c +7 ~~-12~~

index bf9eb693a6a7..c5ff16f9e9fa 100644
@@ -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 = {

fs/ext4/verity.c +3 -3

index 30e3b65798b5..e4da1704438e 100644
@@ -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 = {

fs/f2fs/verity.c +3 -3

index c352fff88a5e..f320ed8172ec 100644
@@ -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 = {

fs/verity/enable.c +2 -2

index df6b499bf6a1..a949ce817202 100644
@@ -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",

include/linux/fsverity.h +4 -4

index 203f4962c54a..f5ed7ecfd9ab 100644
@@ -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

Signature · 1 line

2.38.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

Eric Biggers

author maintainer

2023-01-25 18:12 UTC #6 lore raw

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

[Added back Cc's. Please use Reply-All instead of Reply!]

Quoted 11 lines · Andrey Albershteyn

Quoted 9 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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'.

Quoted 8 lines

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.

Quoted 4 lines

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

Dave Chinner

Acked-by on the v1 series

2022-12-14 23:10 UTC #7 lore raw

In reply to Eric Biggers

Quoted 12 lines · Eric Biggers

This series implements a few cleanups that have been suggested in the thread "[RFC PATCH 00/11] fs-verity support for XFS"
(https://lore.kernel.org/linux-fsdevel/[email protected]/T/#u).
This applies to mainline (commit 93761c93e9da).
Eric Biggers (4):
fsverity: optimize fsverity_file_open() on non-verity files
fsverity: optimize fsverity_prepare_setattr() on non-verity files
fsverity: optimize fsverity_cleanup_inode() on non-verity files
fsverity: pass pos and size to ->write_merkle_tree_block

Quoted 7 lines

 fs/btrfs/verity.c        | 19 ++++-------
 fs/ext4/verity.c         |  6 ++--
 fs/f2fs/verity.c         |  6 ++--
 fs/verity/enable.c       |  4 +--
 fs/verity/open.c         | 46 ++++---------------------
 include/linux/fsverity.h | 74 +++++++++++++++++++++++++++++++++-------
 6 files changed, 84 insertions(+), 71 deletions(-)

The whole series looks fairly sane to me.

Acked-by Dave Chinner <[email protected]>

Signature · 2 lines

Dave Chinner
[email protected]

Eric Biggers

author maintainer

2023-01-04 06:58 UTC #8 lore raw

In reply to Eric Biggers

Quoted 12 lines · Eric Biggers

This series implements a few cleanups that have been suggested in the thread "[RFC PATCH 00/11] fs-verity support for XFS"
(https://lore.kernel.org/linux-fsdevel/[email protected]/T/#u).
This applies to mainline (commit 93761c93e9da).
Eric Biggers (4):
fsverity: optimize fsverity_file_open() on non-verity files
fsverity: optimize fsverity_prepare_setattr() on non-verity files
fsverity: optimize fsverity_cleanup_inode() on non-verity files
fsverity: pass pos and size to ->write_merkle_tree_block

Quoted 9 lines

 fs/btrfs/verity.c        | 19 ++++-------
 fs/ext4/verity.c         |  6 ++--
 fs/f2fs/verity.c         |  6 ++--
 fs/verity/enable.c       |  4 +--
 fs/verity/open.c         | 46 ++++---------------------
 include/linux/fsverity.h | 74 +++++++++++++++++++++++++++++++++-------
 6 files changed, 84 insertions(+), 71 deletions(-)

Applied for 6.3. (To
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/?h=fsverity for now, but there might be a new git repo soon, as is being discussed elsewhere.)

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel