diff mbox series

[08/13] fsverity: expose merkle tree geometry to callers

Message ID 171175867998.1987804.8334701724660862039.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series [01/13] fs: add FS_XFLAG_VERITY for verity files | expand

Commit Message

Darrick J. Wong March 30, 2024, 12:34 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Create a function that will return selected information about the
geometry of the merkle tree.  Online fsck for XFS will need this piece
to perform basic checks of the merkle tree.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/verity/open.c         |   26 ++++++++++++++++++++++++++
 include/linux/fsverity.h |    3 +++
 2 files changed, 29 insertions(+)

Comments

Eric Biggers April 5, 2024, 2:50 a.m. UTC | #1
On Fri, Mar 29, 2024 at 05:34:45PM -0700, Darrick J. Wong wrote:
> +/**
> + * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> + * @inode: the inode for which the Merkle tree is being built

This function is actually for inodes that already have fsverity enabled.  So the
above comment is misleading.

> +int fsverity_merkle_tree_geometry(struct inode *inode, unsigned int *block_size,
> +				  u64 *tree_size)
> +{
> +	struct fsverity_info *vi;
> +	int error;
> +
> +	if (!IS_VERITY(inode))
> +		return -EOPNOTSUPP;

Maybe use ENODATA, similar to fsverity_ioctl_measure() and
bpf_get_fsverity_digest().

> +
> +	error = ensure_verity_info(inode);
> +	if (error)
> +		return error;
> +
> +	vi = fsverity_get_info(inode);

This can just use 'vi = inode->i_verity_info', since ensure_verity_info() was
called.

It should also be documented that an open need not have been done on the file
yet, as this behavior differs from functions like fsverity_get_digest() that
require that an open was done first.

- Eric
Darrick J. Wong April 25, 2024, 12:45 a.m. UTC | #2
On Thu, Apr 04, 2024 at 10:50:45PM -0400, Eric Biggers wrote:
> On Fri, Mar 29, 2024 at 05:34:45PM -0700, Darrick J. Wong wrote:
> > +/**
> > + * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> > + * @inode: the inode for which the Merkle tree is being built
> 
> This function is actually for inodes that already have fsverity enabled.  So the
> above comment is misleading.

How about:

/**
 * fsverity_merkle_tree_geometry() - return Merkle tree geometry
 * @inode: the inode to query
 * @block_size: size of a merkle tree block, in bytes
 * @tree_size: size of the merkle tree, in bytes
 *
 * Callers are not required to have opened the file.
 */


> > +int fsverity_merkle_tree_geometry(struct inode *inode, unsigned int *block_size,
> > +				  u64 *tree_size)
> > +{
> > +	struct fsverity_info *vi;
> > +	int error;
> > +
> > +	if (!IS_VERITY(inode))
> > +		return -EOPNOTSUPP;
> 
> Maybe use ENODATA, similar to fsverity_ioctl_measure() and
> bpf_get_fsverity_digest().

Done.

> > +
> > +	error = ensure_verity_info(inode);
> > +	if (error)
> > +		return error;
> > +
> > +	vi = fsverity_get_info(inode);
> 
> This can just use 'vi = inode->i_verity_info', since ensure_verity_info() was
> called.

Changed.

> It should also be documented that an open need not have been done on the file
> yet, as this behavior differs from functions like fsverity_get_digest() that
> require that an open was done first.

Done.

--D

> - Eric
>
Eric Biggers April 25, 2024, 12:49 a.m. UTC | #3
On Wed, Apr 24, 2024 at 05:45:45PM -0700, Darrick J. Wong wrote:
> On Thu, Apr 04, 2024 at 10:50:45PM -0400, Eric Biggers wrote:
> > On Fri, Mar 29, 2024 at 05:34:45PM -0700, Darrick J. Wong wrote:
> > > +/**
> > > + * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> > > + * @inode: the inode for which the Merkle tree is being built
> > 
> > This function is actually for inodes that already have fsverity enabled.  So the
> > above comment is misleading.
> 
> How about:
> 
> /**
>  * fsverity_merkle_tree_geometry() - return Merkle tree geometry
>  * @inode: the inode to query
>  * @block_size: size of a merkle tree block, in bytes
>  * @tree_size: size of the merkle tree, in bytes
>  *
>  * Callers are not required to have opened the file.
>  */

Looks okay, but it would be helpful to document that the two output parameters
are outputs, and to document the return value.

- Eric
Darrick J. Wong April 25, 2024, 1:01 a.m. UTC | #4
On Thu, Apr 25, 2024 at 12:49:27AM +0000, Eric Biggers wrote:
> On Wed, Apr 24, 2024 at 05:45:45PM -0700, Darrick J. Wong wrote:
> > On Thu, Apr 04, 2024 at 10:50:45PM -0400, Eric Biggers wrote:
> > > On Fri, Mar 29, 2024 at 05:34:45PM -0700, Darrick J. Wong wrote:
> > > > +/**
> > > > + * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> > > > + * @inode: the inode for which the Merkle tree is being built
> > > 
> > > This function is actually for inodes that already have fsverity enabled.  So the
> > > above comment is misleading.
> > 
> > How about:
> > 
> > /**
> >  * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> >  * @inode: the inode to query
> >  * @block_size: size of a merkle tree block, in bytes
> >  * @tree_size: size of the merkle tree, in bytes
> >  *
> >  * Callers are not required to have opened the file.
> >  */
> 
> Looks okay, but it would be helpful to document that the two output parameters
> are outputs, and to document the return value.

How about:

 * Callers are not required to have opened the file.  Returns 0 for success,
 * -ENODATA if verity is not enabled, or any of the error codes that can result
 * from loading verity information while opening a file.

--D

> - Eric
>
Eric Biggers April 25, 2024, 1:04 a.m. UTC | #5
On Wed, Apr 24, 2024 at 06:01:37PM -0700, Darrick J. Wong wrote:
> On Thu, Apr 25, 2024 at 12:49:27AM +0000, Eric Biggers wrote:
> > On Wed, Apr 24, 2024 at 05:45:45PM -0700, Darrick J. Wong wrote:
> > > On Thu, Apr 04, 2024 at 10:50:45PM -0400, Eric Biggers wrote:
> > > > On Fri, Mar 29, 2024 at 05:34:45PM -0700, Darrick J. Wong wrote:
> > > > > +/**
> > > > > + * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> > > > > + * @inode: the inode for which the Merkle tree is being built
> > > > 
> > > > This function is actually for inodes that already have fsverity enabled.  So the
> > > > above comment is misleading.
> > > 
> > > How about:
> > > 
> > > /**
> > >  * fsverity_merkle_tree_geometry() - return Merkle tree geometry
> > >  * @inode: the inode to query
> > >  * @block_size: size of a merkle tree block, in bytes
> > >  * @tree_size: size of the merkle tree, in bytes
> > >  *
> > >  * Callers are not required to have opened the file.
> > >  */
> > 
> > Looks okay, but it would be helpful to document that the two output parameters
> > are outputs, and to document the return value.
> 
> How about:
> 
>  * Callers are not required to have opened the file.  Returns 0 for success,
>  * -ENODATA if verity is not enabled, or any of the error codes that can result
>  * from loading verity information while opening a file.
> 

The wording sounds good, but since this is a kerneldoc-style comment the
information about the return value should be in a "Return:" section.

- Eric
diff mbox series

Patch

diff --git a/fs/verity/open.c b/fs/verity/open.c
index 9603b3a404f74..7a86407732c41 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -412,6 +412,32 @@  void __fsverity_cleanup_inode(struct inode *inode)
 }
 EXPORT_SYMBOL_GPL(__fsverity_cleanup_inode);
 
+/**
+ * fsverity_merkle_tree_geometry() - return Merkle tree geometry
+ * @inode: the inode for which the Merkle tree is being built
+ * @block_size: size of a merkle tree block, in bytes
+ * @tree_size: size of the merkle tree, in bytes
+ */
+int fsverity_merkle_tree_geometry(struct inode *inode, unsigned int *block_size,
+				  u64 *tree_size)
+{
+	struct fsverity_info *vi;
+	int error;
+
+	if (!IS_VERITY(inode))
+		return -EOPNOTSUPP;
+
+	error = ensure_verity_info(inode);
+	if (error)
+		return error;
+
+	vi = fsverity_get_info(inode);
+	*block_size = vi->tree_params.block_size;
+	*tree_size = vi->tree_params.tree_size;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fsverity_merkle_tree_geometry);
+
 void __init fsverity_init_info_cache(void)
 {
 	fsverity_info_cachep = KMEM_CACHE_USERCOPY(
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 030d7094d80fc..5b1485a842983 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -245,6 +245,9 @@  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);
 
+int fsverity_merkle_tree_geometry(struct inode *inode, unsigned int *block_size,
+				  u64 *tree_size);
+
 /**
  * fsverity_cleanup_inode() - free the inode's verity info, if present
  * @inode: an inode being evicted