Re: [PATCH 35/40] xfs: teach online repair to evaluate fsverity xattrs

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

On Mon, Mar 18, 2024 at 06:34:04PM +0100, Andrey Albershteyn wrote:
> On 2024-03-17 09:32:31, 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   |  102 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/scrub/attr.h   |    4 ++
> >  fs/xfs/scrub/common.c |   27 +++++++++++++
> >  3 files changed, 133 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> > index ae4227cb55ec..c69dee281984 100644
> > --- a/fs/xfs/scrub/attr.c
> > +++ b/fs/xfs/scrub/attr.c
> > @@ -21,6 +21,8 @@
> >  #include "scrub/dabtree.h"
> >  #include "scrub/attr.h"
> >  
> > +#include <linux/fsverity.h>
> > +
> >  /* Free the buffers linked from the xattr buffer. */
> >  static void
> >  xchk_xattr_buf_cleanup(
> > @@ -135,6 +137,91 @@ xchk_setup_xattr(
> >  	return xchk_setup_inode_contents(sc, 0);
> >  }
> >  
> > +#ifdef CONFIG_FS_VERITY
> > +/* Extract merkle tree geometry from incore information. */
> > +static int
> > +xchk_xattr_extract_verity(
> > +	struct xfs_scrub		*sc)
> > +{
> > +	struct xchk_xattr_buf		*ab = sc->buf;
> > +
> > +	/* setup should have allocated the buffer */
> > +	if (!ab) {
> > +		ASSERT(0);
> > +		return -EFSCORRUPTED;
> > +	}
> > +
> > +	return fsverity_merkle_tree_geometry(VFS_I(sc->ip),
> > +			&ab->merkle_blocksize, &ab->merkle_tree_size);
> > +}
> > +
> > +/* 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;
> > +	}
> > +
> > +	switch (namelen) {
> > +	case sizeof(struct xfs_verity_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_verity_merkle_key_from_disk(name) >= ab->merkle_tree_size)
> > +		xchk_ino_set_preen(sc, sc->ip->i_ino);
> > +}
> > +#else
> > +# define xchk_xattr_extract_verity(sc)	(0)
> > +
> > +static void
> > +xchk_xattr_verity(
> > +	struct xfs_scrub	*sc,
> > +	xfs_dablk_t		blkno,
> > +	const unsigned char	*name,
> > +	unsigned int		namelen)
> > +{
> > +	/* Should never see verity xattrs when verity is not enabled. */
> > +	xchk_fblock_set_corrupt(sc, XFS_ATTR_FORK, blkno);
> > +}
> > +#endif /* CONFIG_FS_VERITY */
> > +
> >  /* Extended Attributes */
> >  
> >  struct xchk_xattr {
> > @@ -194,6 +281,15 @@ xchk_xattr_listent(
> >  		goto fail_xref;
> >  	}
> >  
> > +	/* Check verity xattr geometry */
> > +	if (flags & XFS_ATTR_VERITY) {
> > +		xchk_xattr_verity(sx->sc, args.blkno, name, namelen, valuelen);
> > +		if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT) {
> > +			context->seen_enough = 1;
> > +			return;
> > +		}
> > +	}
> > +
> >  	/* Does this name make sense? */
> >  	if (!xfs_attr_namecheck(sx->sc->mp, name, namelen, flags)) {
> >  		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
> 
> Would it be better to check verity after xfs_attr_namecheck()?
> Invalid name seems to be a more basic corruption.

Yeah, that could be changed easily. Done.

--D

> -- 
> - Andrey
> 
>