| Message ID | 171069246470.2684506.16777519924436608697.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/40] fsverity: remove hash page spin lock | expand |
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.
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 > >
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); @@ -611,6 +707,12 @@ xchk_xattr( if (error) return error; + if (IS_VERITY(VFS_I(sc->ip))) { + error = xchk_xattr_extract_verity(sc); + if (error) + return error; + } + /* Check the physical structure of the xattr. */ if (sc->ip->i_af.if_format == XFS_DINODE_FMT_LOCAL) error = xchk_xattr_check_sf(sc); diff --git a/fs/xfs/scrub/attr.h b/fs/xfs/scrub/attr.h index 48fd9402c432..37849ffb0375 100644 --- a/fs/xfs/scrub/attr.h +++ b/fs/xfs/scrub/attr.h @@ -19,6 +19,10 @@ struct xchk_xattr_buf { /* Memory buffer used to extract xattr values. */ void *value; size_t value_sz; + + /* Geometry of the merkle tree attached to this verity file. */ + u64 merkle_tree_size; + unsigned int merkle_blocksize; }; #endif /* __XFS_SCRUB_ATTR_H__ */ diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c index abff79a77c72..dd2ed1f833c5 100644 --- a/fs/xfs/scrub/common.c +++ b/fs/xfs/scrub/common.c @@ -37,6 +37,8 @@ #include "scrub/repair.h" #include "scrub/health.h" +#include <linux/fsverity.h> + /* Common code for the metadata scrubbers. */ /* @@ -1073,6 +1075,25 @@ xchk_irele( xfs_irele(ip); } +#ifdef CONFIG_FS_VERITY +/* + * Make sure the fsverity information is attached, so we don't have to do that + * later after taking locks. + */ +static inline int +xchk_setup_fsverity( + struct xfs_scrub *sc) +{ + unsigned int dontcare; + u64 alsodontcare; + + return fsverity_merkle_tree_geometry(VFS_I(sc->ip), + &dontcare, &alsodontcare); +} +#else +# define xchk_setup_fsverity(sc) (0) +#endif + /* * Set us up to scrub metadata mapped by a file's fork. Callers must not use * this to operate on user-accessible regular file data because the MMAPLOCK is @@ -1092,6 +1113,12 @@ xchk_setup_inode_contents( /* Lock the inode so the VFS cannot touch this file. */ xchk_ilock(sc, XFS_IOLOCK_EXCL); + if (IS_VERITY(VFS_I(sc->ip))) { + error = xchk_setup_fsverity(sc); + if (error) + goto out; + } + error = xchk_trans_alloc(sc, resblks); if (error) goto out;