Re: [RFC PATCH 07/11] xfs: disable direct read path for fs-verity sealed files

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Dec 14, 2022 at 01:07:15PM +1100, Dave Chinner wrote:
> On Tue, Dec 13, 2022 at 06:29:31PM +0100, Andrey Albershteyn wrote:
> > The direct path is not supported on verity files. Attempts to use direct
> > I/O path on such files should fall back to buffered I/O path.
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> >  fs/xfs/xfs_file.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 5eadd9a37c50e..fb4181e38a19d 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -245,7 +245,8 @@ xfs_file_dax_read(
> >  	struct kiocb		*iocb,
> >  	struct iov_iter		*to)
> >  {
> > -	struct xfs_inode	*ip = XFS_I(iocb->ki_filp->f_mapping->host);
> > +	struct inode		*inode = iocb->ki_filp->f_mapping->host;
> > +	struct xfs_inode	*ip = XFS_I(inode);
> >  	ssize_t			ret = 0;
> >  
> >  	trace_xfs_file_dax_read(iocb, to);
> > @@ -298,10 +299,17 @@ xfs_file_read_iter(
> >  
> >  	if (IS_DAX(inode))
> >  		ret = xfs_file_dax_read(iocb, to);
> 
> fsverity is supported on DAX?
> 
> Eric, I was under the impression that the DAX io path does not
> support fsverity, but I can't see anything that prevents ext4 from
> using fsverity on dax enabled filesystems. Does this work (is it
> tested regularly?), or is the lack of checking simply an oversight
> in that nobody thought to check DAX status when fsverity is enabled?

DAX and fsverity are mutually exclusive.  ext4_set_inode_flags() doesn't set the
DAX flag if the inode already has the verity flag, and
ext4_begin_enable_verity() doesn't allow setting the verity flag if the inode
already has the DAX flag.

> 
> > -	else if (iocb->ki_flags & IOCB_DIRECT)
> > +	else if (iocb->ki_flags & IOCB_DIRECT && !fsverity_active(inode))
> >  		ret = xfs_file_dio_read(iocb, to);
> > -	else
> > +	else {
> > +		/*
> > +		 * In case fs-verity is enabled, we also fallback to the
> > +		 * buffered read from the direct read path. Therefore,
> > +		 * IOCB_DIRECT is set and need to be cleared
> > +		 */
> > +		iocb->ki_flags &= ~IOCB_DIRECT;
> >  		ret = xfs_file_buffered_read(iocb, to);
> > +	}
> 
> Is this IOCB_DIRECT avoidance a limitation of the XFS
> implementation, or a generic limitation of the fsverity
> infrastructure?
> 
> If it's a limitation of the fsverity infrastructure, then we
> shouldn't be working around this in every single filesystem that
> supports fsverity.  If all the major filesystems are having to check
> fsverity_active() and clear IOCB_DIRECT on every single IOCB_DIRECT
> IO that is issued on a fsverity inode, then shouldn't we just elide
> IOCB_DIRECT from file->f_iocb_flags in the first place?

It's mainly a filesystem limitation, not a fs/verity/ limitation.  However, the
functions in fs/verity/verify.c do assume that the data pages are page cache
pages.  To allow filesystems to support direct I/O on verity files, functions
that take the inode and file offset explicitly would need to be added.

Not setting IOCB_DIRECT in ->f_iocb_flags is an interesting idea.  I've been
trying not to add fscrypt and fsverity stuff to the core VFS syscall paths,
since only certain filesystems support these features, so it makes sense to
limit to the overhead (however minimal) to those filesystems only.  However,
since ->f_iocb_flags was recently added to cache iocb_flags(), it does look like
the VFS could check IS_VERITY() in iocb_flags() with minimal overhead.

A potential issue is that if a file is opened with O_DIRECT and then
FS_IOC_ENABLE_VERITY is run (either on that fd or on a different fd), then the
O_DIRECT fd would still exist -- with IOCB_DIRECT in ->f_iocb_flags.

The read-time check would be needed to correctly handle that case.

- Eric