Re: [RFC PATCH 06/11] xfs: initialize fs-verity on file open and cleanup on inode destruction

[Dave Chinner <david@fromorbit.com>]

On Tue, Dec 13, 2022 at 09:25:38PM -0800, Eric Biggers wrote:
> On Wed, Dec 14, 2022 at 12:35:24PM +1100, Dave Chinner wrote:
> > On Tue, Dec 13, 2022 at 06:29:30PM +0100, Andrey Albershteyn wrote:
> > > fs-verity will read and attach metadata (not the tree itself) from
> > > a disk for those inodes which already have fs-verity enabled.
> > > 
> > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > > ---
> > >  fs/xfs/xfs_file.c  | 8 ++++++++
> > >  fs/xfs/xfs_super.c | 2 ++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index 242165580e682..5eadd9a37c50e 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -32,6 +32,7 @@
> > >  #include <linux/mman.h>
> > >  #include <linux/fadvise.h>
> > >  #include <linux/mount.h>
> > > +#include <linux/fsverity.h>
> > >  
> > >  static const struct vm_operations_struct xfs_file_vm_ops;
> > >  
> > > @@ -1170,9 +1171,16 @@ xfs_file_open(
> > >  	struct inode	*inode,
> > >  	struct file	*file)
> > >  {
> > > +	int		error = 0;
> > > +
> > >  	if (xfs_is_shutdown(XFS_M(inode->i_sb)))
> > >  		return -EIO;
> > >  	file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC;
> > > +
> > > +	error = fsverity_file_open(inode, file);
> > > +	if (error)
> > > +		return error;
> > 
> > This is a hot path, so shouldn't we elide the function call
> > altogether if verity is not enabled on the inode? i.e:
> > 
> > 	if (IS_VERITY(inode)) {
> > 		error = fsverity_file_open(inode, file);
> > 		if (error)
> > 			return error;
> > 	}
> > 
> > It doesn't really matter for a single file open, but when you're
> > opening a few million inodes every second the function call overhead
> > only to immediately return because IS_VERITY() is false adds up...
> > 
> > >  	return generic_file_open(inode, file);
> > >  }
> > >  
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 8f1e9b9ed35d9..50c2c819ba940 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -45,6 +45,7 @@
> > >  #include <linux/magic.h>
> > >  #include <linux/fs_context.h>
> > >  #include <linux/fs_parser.h>
> > > +#include <linux/fsverity.h>
> > >  
> > >  static const struct super_operations xfs_super_operations;
> > >  
> > > @@ -647,6 +648,7 @@ xfs_fs_destroy_inode(
> > >  	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> > >  	XFS_STATS_INC(ip->i_mount, vn_rele);
> > >  	XFS_STATS_INC(ip->i_mount, vn_remove);
> > > +	fsverity_cleanup_inode(inode);
> > 
> > Similarly, shouldn't this be:
> > 
> > 	if (fsverity_active(inode))
> > 		fsverity_cleanup_inode(inode);
> > 
> 
> If you actually want to do that, then we should instead make these functions
> inline functions that do the "is anything needed?" check, then call a
> double-underscored version that does the actual work.  Some of the fscrypt
> functions are like that.  Then all filesystems would get the benefit.

Agreed, that's the right way to do it. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com