Re: [PATCH 28/29] xfs: allow verity files to be opened even if the fsverity metadata is damaged

["Colin Walters" <walters@verbum.org>]

On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> There are more things that one can do with an open file descriptor on
> XFS -- query extended attributes, scan for metadata damage, repair
> metadata, etc.  None of this is possible if the fsverity metadata are
> damaged, because that prevents the file from being opened.
>
> Ignore a selective set of error codes that we know fsverity_file_open to
> return if the verity descriptor is nonsense.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/iomap/buffered-io.c |    8 ++++++++
>  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 9f9d929dfeebc..e68a15b72dbdd 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
> iomap_iter *iter,
>  	size_t poff, plen;
>  	sector_t sector;
> 
> +	/*
> +	 * If this verity file hasn't been activated, fail read attempts.  This
> +	 * can happen if the calling filesystem allows files to be opened even
> +	 * with damaged verity metadata.
> +	 */
> +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
> +		return -EIO;
> +
>  	if (iomap->type == IOMAP_INLINE)
>  		return iomap_read_inline_data(iter, folio);
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c0b3e8146b753..36034eaefbf55 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1431,8 +1431,25 @@ xfs_file_open(
>  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> 
>  	error = fsverity_file_open(inode, file);
> -	if (error)
> +	switch (error) {
> +	case -EFBIG:
> +	case -EINVAL:
> +	case -EMSGSIZE:
> +	case -EFSCORRUPTED:
> +		/*
> +		 * Be selective about which fsverity errors we propagate to
> +		 * userspace; we still want to be able to open this file even
> +		 * if reads don't work.  Someone might want to perform an
> +		 * online repair.
> +		 */
> +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
> +			break;

As I understand it, fsverity (and dm-verity) are desirable in high-safety and integrity requirement cases where the goal is for the system to "fail closed" if errors in general are detected; anything that would have the system be in an ill-defined state.

A lot of ambient processes are going to have CAP_SYS_ADMIN and this will just swallow these errors for those (will things the EFSCORRUPTED path at least have been logged by a lower level function?)...whereas this is only needed just for a very few tools.

At least for composefs the quoted cases of "query extended attributes, scan for metadata damage, repair metadata" are all things that canonically live in the composefs metadata (EROFS) blob, so in theory there's a lot less of a need to query/inspect it for those use cases.  (Maybe for composefs we should force canonicalize all the underlying files to have mode 0400 and no xattrs or something and add that to its repair).

I hesitate to say it but maybe there should be some ioctl for online repair use cases only, or perhaps a new O_NOVERITY special flag to openat2()?