Re: [PATCH 24/29] xfs: teach online repair to evaluate fsverity xattrs

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Apr 02, 2024 at 05:42:04PM +0200, Andrey Albershteyn wrote:
> On 2024-03-29 17:42:19, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Teach online repair to check for unused fsverity metadata and purge it
> > on reconstruction.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/scrub/attr.c        |  139 ++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/scrub/attr.h        |    6 ++
> >  fs/xfs/scrub/attr_repair.c |   50 ++++++++++++++++
> >  fs/xfs/scrub/trace.c       |    1 
> >  fs/xfs/scrub/trace.h       |   31 ++++++++++
> >  5 files changed, 226 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> > index 2e8a2b2e82fbd..be121625c14f0 100644
> > --- a/fs/xfs/scrub/attr.c
> > +++ b/fs/xfs/scrub/attr.c
> > @@ -18,6 +18,7 @@
> >  #include "xfs_attr_leaf.h"
> >  #include "xfs_attr_sf.h"
> >  #include "xfs_parent.h"
> > +#include "xfs_verity.h"
> >  #include "scrub/scrub.h"
> >  #include "scrub/common.h"
> >  #include "scrub/dabtree.h"
> > @@ -25,6 +26,8 @@
> >  #include "scrub/listxattr.h"
> >  #include "scrub/repair.h"
> >  
> > +#include <linux/fsverity.h>
> > +
> >  /* Free the buffers linked from the xattr buffer. */
> >  static void
> >  xchk_xattr_buf_cleanup(
> > @@ -126,6 +129,53 @@ xchk_setup_xattr_buf(
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_FS_VERITY
> > +/*
> > + * Obtain merkle tree geometry information for a verity file so that we can
> > + * perform sanity checks of the fsverity xattrs.
> > + */
> > +STATIC int
> > +xchk_xattr_setup_verity(
> > +	struct xfs_scrub	*sc)
> > +{
> > +	struct xchk_xattr_buf	*ab;
> > +	int			error;
> > +
> > +	/*
> > +	 * Drop the ILOCK and the transaction because loading the fsverity
> > +	 * metadata will call into the xattr code.  S_VERITY is enabled with
> > +	 * IOLOCK_EXCL held, so it should not change here.
> > +	 */
> > +	xchk_iunlock(sc, XFS_ILOCK_EXCL);
> > +	xchk_trans_cancel(sc);
> > +
> > +	error = xchk_setup_xattr_buf(sc, 0);
> > +	if (error)
> > +		return error;
> > +
> > +	ab = sc->buf;
> > +	error = fsverity_merkle_tree_geometry(VFS_I(sc->ip),
> > +			&ab->merkle_blocksize, &ab->merkle_tree_size);
> > +	if (error == -ENODATA || error == -EFSCORRUPTED) {
> > +		/* fsverity metadata corrupt, cannot complete checks */
> > +		xchk_set_incomplete(sc);
> > +		ab->merkle_blocksize = 0;
> > +		error = 0;
> > +	}
> > +	if (error)
> > +		return error;
> > +
> > +	error = xchk_trans_alloc(sc, 0);
> > +	if (error)
> > +		return error;
> > +
> > +	xchk_ilock(sc, XFS_ILOCK_EXCL);
> > +	return 0;
> > +}
> > +#else
> > +# define xchk_xattr_setup_verity(...)	(0)
> > +#endif /* CONFIG_FS_VERITY */
> > +
> >  /* Set us up to scrub an inode's extended attributes. */
> >  int
> >  xchk_setup_xattr(
> > @@ -150,9 +200,89 @@ xchk_setup_xattr(
> >  			return error;
> >  	}
> >  
> > -	return xchk_setup_inode_contents(sc, 0);
> > +	error = xchk_setup_inode_contents(sc, 0);
> > +	if (error)
> > +		return error;
> > +
> > +	if (IS_VERITY(VFS_I(sc->ip))) {
> > +		error = xchk_xattr_setup_verity(sc);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return error;
> >  }
> >  
> > +#ifdef CONFIG_FS_VERITY
> > +/* Check the merkle tree xattrs. */
> > +STATIC void
> > +xchk_xattr_verity(
> > +	struct xfs_scrub		*sc,
> > +	xfs_dablk_t			blkno,
> > +	const unsigned char		*name,
> > +	unsigned int			namelen,
> > +	unsigned int			valuelen)
> > +{
> > +	struct xchk_xattr_buf		*ab = sc->buf;
> > +
> > +	/* Non-verity filesystems should never have verity xattrs. */
> > +	if (!xfs_has_verity(sc->mp)) {
> > +		xchk_fblock_set_corrupt(sc, XFS_ATTR_FORK, blkno);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Any verity metadata on a non-verity file are leftovers from a
> > +	 * previous attempt to enable verity.
> > +	 */
> > +	if (!IS_VERITY(VFS_I(sc->ip))) {
> > +		xchk_ino_set_preen(sc, sc->ip->i_ino);
> > +		return;
> > +	}
> > +
> > +	/* Zero blocksize occurs if we couldn't load the merkle tree data. */
> > +	if (ab->merkle_blocksize == 0)
> > +		return;
> > +
> > +	switch (namelen) {
> > +	case sizeof(struct xfs_merkle_key):
> > +		/* Oversized blocks are not allowed */
> > +		if (valuelen > ab->merkle_blocksize) {
> > +			xchk_fblock_set_corrupt(sc, XFS_ATTR_FORK, blkno);
> > +			return;
> > +		}
> > +		break;
> > +	case XFS_VERITY_DESCRIPTOR_NAME_LEN:
> > +		/* Has to match the descriptor xattr name */
> > +		if (memcmp(name, XFS_VERITY_DESCRIPTOR_NAME, namelen))
> > +			xchk_fblock_set_corrupt(sc, XFS_ATTR_FORK, blkno);
> > +		return;
> > +	default:
> > +		xchk_fblock_set_corrupt(sc, XFS_ATTR_FORK, blkno);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Merkle tree blocks beyond the end of the tree are leftovers from
> > +	 * a previous failed attempt to enable verity.
> > +	 */
> > +	if (xfs_merkle_key_from_disk(name, namelen) >= ab->merkle_tree_size)
> > +		xchk_ino_set_preen(sc, sc->ip->i_ino);
> 
> The other case which probably can be detected is if we start
> removing the tree and it gets interrupted (starting blocks missing).
> This can be checked by iterating over the xattrs names up to
> ->merkle_tree_size. But I'm not sure if online repair can store
> state over xattrs validation.

It can; you'd just have to amend the xchk_xattr_buf to store whatever
extra data you want.  That said, if IS_VERITY() isn't true, then we'll
flag the xattr structure for any XFS_ATTR_VERITY attrs:

	/*
	 * Any verity metadata on a non-verity file are leftovers from a
	 * previous attempt to enable verity.
	 */
	if (!IS_VERITY(VFS_I(sc->ip))) {
		xchk_ino_set_preen(sc, sc->ip->i_ino);
		return;
	}

And attr_repair.c will not salvage the attrs when it reconstructs the
attr structure.

> Also, only pair of valid descriptor and valid tree is something of
> use, but I'm not sure if all of this is in scope of online repair.

Not here -- the xfsprogs verity patchset amends xfs_scrub phase 6 to
look for verity files so that it can open them and read the contents to
see if any IO errors occur.  That will catch missing/inconsistent bits
in the fsverity metadata.

> Otherwise, looks good to me:
> Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

Thanks!

--D

> -- 
> - Andrey
> 
>