diff mbox series

[8/9] xfs: skip the rmapbt search on an empty attr fork unless we know it was zapped

Message ID 170191666238.1182270.18118442139749127193.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series xfs: online repair of inodes and forks | expand

Commit Message

Darrick J. Wong Dec. 7, 2023, 2:43 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

The attribute fork scrubber can optionally scan the reverse mapping
records of the filesystem to determine if the fork is missing mappings
that it should have.  However, this is a very expensive operation, so we
only want to do this if we suspect that the fork is missing records.
For attribute forks the criteria for suspicion is that the attr fork is
in EXTENTS format and has zero extents.

However, there are several ways that a file can end up in this state
through regular filesystem usage.  For example, an LSM can set a
s_security hook but then decide not to set an ACL; or an attr set can
create the attr fork but then the actual set operation fails with
ENOSPC; or we can delete all the attrs on a file whose data fork is in
btree format, in which case we do not delete the attr fork.  We don't
want to run the expensive check for any case that can be arrived at
through regular operations.

However.

When online inode repair decides to zap an attribute fork, it cannot
determine if it is zapping ACL information.  As a precaution it removes
all the discretionary access control permissions and sets the user and
group ids to zero.  Check these three additional conditions to decide if
we want to scan the rmap records.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/scrub/bmap.c |   44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig Dec. 7, 2023, 6:07 a.m. UTC | #1 
On Wed, Dec 06, 2023 at 06:43:47PM -0800, Darrick J. Wong wrote:
> +			if ((VFS_I(sc->ip)->i_mode & 0777) != 0)
> +				return false;
> +			if (!uid_eq(VFS_I(sc->ip)->i_uid, GLOBAL_ROOT_UID))
> +				return false;
> +			if (!gid_eq(VFS_I(sc->ip)->i_gid, GLOBAL_ROOT_GID))
> +				return false;

Having this in a well-documented helper would be nice to have,
but otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Dec. 11, 2023, 10:50 p.m. UTC | #2 
On Wed, Dec 06, 2023 at 10:07:04PM -0800, Christoph Hellwig wrote:
> On Wed, Dec 06, 2023 at 06:43:47PM -0800, Darrick J. Wong wrote:
> > +			if ((VFS_I(sc->ip)->i_mode & 0777) != 0)
> > +				return false;
> > +			if (!uid_eq(VFS_I(sc->ip)->i_uid, GLOBAL_ROOT_UID))
> > +				return false;
> > +			if (!gid_eq(VFS_I(sc->ip)->i_gid, GLOBAL_ROOT_GID))
> > +				return false;
> 
> Having this in a well-documented helper would be nice to have,
> but otherwise this looks good:

Ok, I'll split the attr and data fork paths into separate helpers.
That'll make them more coherent and cut down on the indenting here.

> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

--D
diff mbox series

Patch

diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index 0ff1f631a9594..a632885825b27 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -664,16 +664,46 @@  xchk_bmap_want_check_rmaps(
 	 * The inode repair code zaps broken inode forks by resetting them back
 	 * to EXTENTS format and zero extent records.  If we encounter a fork
 	 * in this state along with evidence that the fork isn't supposed to be
-	 * empty, we need to scan the reverse mappings to decide if we're going
-	 * to rebuild the fork.  Data forks with nonzero file size are scanned.
-	 * xattr forks are never empty of content, so they are always scanned.
+	 * empty, we might want scan the reverse mappings to decide if we're
+	 * going to rebuild the fork.
 	 */
 	ifp = xfs_ifork_ptr(sc->ip, info->whichfork);
 	if (ifp->if_format == XFS_DINODE_FMT_EXTENTS && ifp->if_nextents == 0) {
-		if (info->whichfork == XFS_DATA_FORK &&
-		    i_size_read(VFS_I(sc->ip)) == 0)
-			return false;
-
+		switch (info->whichfork) {
+		case XFS_DATA_FORK:
+			/*
+			 * Data forks with zero file size are presumed not to
+			 * have any written data blocks.  Skip the scan.
+			 */
+			if (i_size_read(VFS_I(sc->ip)) == 0)
+				return false;
+			break;
+		case XFS_ATTR_FORK:
+			/*
+			 * Files can have an attr fork in EXTENTS format with
+			 * zero records for several reasons:
+			 *
+			 * a) an attr set created a fork but ran out of space
+			 * b) attr replace deleted an old attr but failed
+			 *    during the set step
+			 * c) the data fork was in btree format when all attrs
+			 *    were deleted, so the fork was left in place
+			 * d) the inode repair code zapped the fork
+			 *
+			 * Only in case (d) do we want to scan the rmapbt to
+			 * see if we need to rebuild the attr fork.  The fork
+			 * zap code clears all DAC permission bits and zeroes
+			 * the uid and gid, so avoid the scan if any of those
+			 * three conditions are not met.
+			 */
+			if ((VFS_I(sc->ip)->i_mode & 0777) != 0)
+				return false;
+			if (!uid_eq(VFS_I(sc->ip)->i_uid, GLOBAL_ROOT_UID))
+				return false;
+			if (!gid_eq(VFS_I(sc->ip)->i_gid, GLOBAL_ROOT_GID))
+				return false;
+			break;
+		}
 		return true;
 	}