On 2024-03-29 17:42:19, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Teach online repair to check for unused fsverity metadata and purge it
> on reconstruction.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> 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.
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.
Otherwise, looks good to me:
Reviewed-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
--
- Andrey
