diff mbox series

[11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

Message ID 20200130181512.GZ3447196@magnolia (mailing list archive)
State Superseded, archived
Headers show
Series xfsprogs: random fixes | expand

Commit Message

Darrick J. Wong Jan. 30, 2020, 6:15 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

In process_longform_attr, we enforce that the root block of the
attribute index must have both forw or back pointers set to zero.
Unfortunately, the code that nulls out the pointers is not aware that
the root block could be in da3 node format.

This leads to corruption of da3 root node blocks because the functions
that convert attr3 leaf headers to and from the ondisk structures
perform some interpretation of firstused on what they think is an attr1
leaf block.

Found by using xfs/402 to fuzz hdr.info.hdr.forw.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/attr_repair.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Jan. 30, 2020, 6:22 p.m. UTC | #1 
While this looks functionally correct, I think the structure in
here is weird.  In libxfs we usually check that magic number first
and then branch out into helper that deal with the leaf vs node
format.  That is we don't do the xfs_attr3_leaf_hdr_from_disk call
for node format attrs, and also check the forward/backward pointers
based on the actual ichdr.  Maybe this code should follow that
structure?
Darrick J. Wong Jan. 30, 2020, 6:31 p.m. UTC | #2 
On Thu, Jan 30, 2020 at 10:22:30AM -0800, Christoph Hellwig wrote:
> While this looks functionally correct, I think the structure in
> here is weird.  In libxfs we usually check that magic number first
> and then branch out into helper that deal with the leaf vs node
> format.  That is we don't do the xfs_attr3_leaf_hdr_from_disk call
> for node format attrs, and also check the forward/backward pointers
> based on the actual ichdr.  Maybe this code should follow that
> structure?

Yeah, I'll refactor the case blocks into two functions.

--D
diff mbox series

Patch

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 9a44f610..9d2a40f7 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -952,6 +952,33 @@  _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
 	return 0;
 }
 
+/*
+ * Zap the forw/back links in an attribute block.  Be careful, because the
+ * root block could be an attr leaf block or a da node block.
+ */
+static inline void
+clear_attr_forw_back(
+	struct xfs_buf			*bp,
+	struct xfs_attr3_icleaf_hdr	*leafhdr)
+{
+	struct xfs_mount		*mp = bp->b_mount;
+
+	if (leafhdr->magic == XFS_DA_NODE_MAGIC ||
+	    leafhdr->magic == XFS_DA3_NODE_MAGIC) {
+		struct xfs_da3_icnode_hdr	da3_hdr;
+
+		xfs_da3_node_hdr_from_disk(mp, &da3_hdr, bp->b_addr);
+		da3_hdr.forw = 0;
+		da3_hdr.back = 0;
+		xfs_da3_node_hdr_to_disk(mp, bp->b_addr, &da3_hdr);
+		return;
+	}
+
+	leafhdr->forw = 0;
+	leafhdr->back = 0;
+	xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo, bp->b_addr, leafhdr);
+}
+
 /*
  * Start processing for a leaf or fuller btree.
  * A leaf directory is one where the attribute fork is too big for
@@ -1028,10 +1055,7 @@  process_longform_attr(
 	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
 				ino);
 			repairlinks = 1;
-			leafhdr.forw = 0;
-			leafhdr.back = 0;
-			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo,
-						   leaf, &leafhdr);
+			clear_attr_forw_back(bp, &leafhdr);
 		} else  {
 			do_warn(
 	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);