| Message ID | 171175867998.1987804.8334701724660862039.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] fs: add FS_XFLAG_VERITY for verity files | expand |
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
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 >
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
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 >
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 --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