On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
>
>
> On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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.
Is "open() fails if verity metadata are trashed" a hard requirement?
Reads will still fail due to (iomap) readahead returning EIO for a file
that is IS_VERITY() && !fsverity_active(). This is (afaict) the state
you end up with when the fsverity open fails. ext4/f2fs don't do that,
but they also don't have online fsck so once a file's dead it's dead.
> 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).
<shrug> I don't know if regular (i.e. non-verity) xattrs are one of the
things that get frozen by verity? Storing fsverity metadata in private
namespace xattrs is unique to xfs.
> 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()?
"openat2 but without meddling from the VFS"? Tempting... ;)
--D
