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@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.
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@xxxxxxxxxx>
Thanks!
--D
> --
> - Andrey
>
>
