[PATCH 1/4] xfs: refactor extended attribute list operation

[PATCH 1/4] xfs: refactor extended attribute list operation

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

When we're iterating the attribute list and we can't find our previous
location based off the attribute cursor, we'll instead walk down the
attribute btree from the root trying to find where we left off.  Move
this code into a separate function for later cleanups.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_attr_list.c |  136 ++++++++++++++++++++++++++++++------------------
 1 file changed, 84 insertions(+), 52 deletions(-)


diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 5816786..48423eb 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -204,19 +204,89 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 	return 0;
 }
 
+/*
+ * We didn't find the block & hash mentioned in the cursor state, so
+ * walk down the attr btree looking for the hash.
+ */
 STATIC int
-xfs_attr_node_list(xfs_attr_list_context_t *context)
+xfs_attr_node_list_lookup(
+	struct xfs_attr_list_context	*context,
+	struct attrlist_cursor_kern	*cursor,
+	struct xfs_buf			**pbp)
 {
-	attrlist_cursor_kern_t *cursor;
-	xfs_attr_leafblock_t *leaf;
-	xfs_da_intnode_t *node;
-	struct xfs_attr3_icleaf_hdr leafhdr;
-	struct xfs_da3_icnode_hdr nodehdr;
-	struct xfs_da_node_entry *btree;
-	int error, i;
-	struct xfs_buf *bp;
-	struct xfs_inode	*dp = context->dp;
-	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_da3_icnode_hdr	nodehdr;
+	struct xfs_da_intnode		*node;
+	struct xfs_da_node_entry	*btree;
+	struct xfs_inode		*dp = context->dp;
+	struct xfs_mount		*mp = dp->i_mount;
+	struct xfs_trans		*tp = context->tp;
+	int				i;
+	int				error = 0;
+	uint16_t			magic;
+
+	ASSERT(*pbp == NULL);
+	cursor->blkno = 0;
+	for (;;) {
+		error = xfs_da3_node_read(tp, dp, cursor->blkno, -1, pbp,
+				XFS_ATTR_FORK);
+		if (error)
+			return error;
+		node = (*pbp)->b_addr;
+		magic = be16_to_cpu(node->hdr.info.magic);
+		switch (magic) {
+		case XFS_ATTR_LEAF_MAGIC:
+		case XFS_ATTR3_LEAF_MAGIC:
+			goto found_leaf;
+		case XFS_DA_NODE_MAGIC:
+		case XFS_DA3_NODE_MAGIC:
+			/* process btree node below */
+			break;
+		default:
+			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+					node);
+			goto out_corruptbuf;
+		}
+
+		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
+
+		btree = dp->d_ops->node_tree_p(node);
+		for (i = 0; i < nodehdr.count; btree++, i++) {
+			if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
+				cursor->blkno = be32_to_cpu(btree->before);
+				trace_xfs_attr_list_node_descend(context,
+						btree);
+				break;
+			}
+		}
+		if (i == nodehdr.count)
+			goto out_buf;
+
+		xfs_trans_brelse(tp, *pbp);
+	}
+
+found_leaf:
+	return error;
+
+out_corruptbuf:
+	error = -EFSCORRUPTED;
+out_buf:
+	xfs_trans_brelse(tp, *pbp);
+	*pbp = NULL;
+	return error;
+}
+
+STATIC int
+xfs_attr_node_list(
+	struct xfs_attr_list_context	*context)
+{
+	struct xfs_attr3_icleaf_hdr	leafhdr;
+	struct attrlist_cursor_kern	*cursor;
+	struct xfs_attr_leafblock	*leaf;
+	struct xfs_da_intnode		*node;
+	struct xfs_buf			*bp;
+	struct xfs_inode		*dp = context->dp;
+	struct xfs_mount		*mp = dp->i_mount;
+	int				error;
 
 	trace_xfs_attr_node_list(context);
 
@@ -277,47 +347,9 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
 	 * Note that start of node block is same as start of leaf block.
 	 */
 	if (bp == NULL) {
-		cursor->blkno = 0;
-		for (;;) {
-			uint16_t magic;
-
-			error = xfs_da3_node_read(context->tp, dp,
-						      cursor->blkno, -1, &bp,
-						      XFS_ATTR_FORK);
-			if (error)
-				return error;
-			node = bp->b_addr;
-			magic = be16_to_cpu(node->hdr.info.magic);
-			if (magic == XFS_ATTR_LEAF_MAGIC ||
-			    magic == XFS_ATTR3_LEAF_MAGIC)
-				break;
-			if (magic != XFS_DA_NODE_MAGIC &&
-			    magic != XFS_DA3_NODE_MAGIC) {
-				XFS_CORRUPTION_ERROR("xfs_attr_node_list(3)",
-						     XFS_ERRLEVEL_LOW,
-						     context->dp->i_mount,
-						     node);
-				xfs_trans_brelse(context->tp, bp);
-				return -EFSCORRUPTED;
-			}
-
-			dp->d_ops->node_hdr_from_disk(&nodehdr, node);
-			btree = dp->d_ops->node_tree_p(node);
-			for (i = 0; i < nodehdr.count; btree++, i++) {
-				if (cursor->hashval
-						<= be32_to_cpu(btree->hashval)) {
-					cursor->blkno = be32_to_cpu(btree->before);
-					trace_xfs_attr_list_node_descend(context,
-									 btree);
-					break;
-				}
-			}
-			if (i == nodehdr.count) {
-				xfs_trans_brelse(context->tp, bp);
-				return 0;
-			}
-			xfs_trans_brelse(context->tp, bp);
-		}
+		error = xfs_attr_node_list_lookup(context, cursor, &bp);
+		if (error || !bp)
+			return error;
 	}
 	ASSERT(bp != NULL);
 

[PATCH 2/4] xfs: abort dir/attr btree operation if btree is obviously weird

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Abort an dir/attr btree operation if the attr btree has obvious problems
like loops back to the root or pointers don't point down the tree.
Found by fuzzing btree[0].before to zero in xfs/402, which livelocks on
the cycle in the attr btree.

Apply the same checks to xfs_da3_node_lookup_int.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_da_btree.c |   34 +++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_attr_list.c       |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 6d43358..3dbeda6 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -1466,6 +1466,7 @@ xfs_da3_node_lookup_int(
 	int			max;
 	int			error;
 	int			retval;
+	unsigned int		expected_level = -1U;
 	struct xfs_inode	*dp = state->args->dp;
 
 	args = state->args;
@@ -1474,7 +1475,7 @@ xfs_da3_node_lookup_int(
 	 * Descend thru the B-tree searching each level for the right
 	 * node to use, until the right hashval is found.
 	 */
-	blkno = (args->whichfork == XFS_DATA_FORK)? args->geo->leafblk : 0;
+	blkno = args->geo->leafblk;
 	for (blk = &state->path.blk[0], state->path.active = 1;
 			 state->path.active <= XFS_DA_NODE_MAXDEPTH;
 			 blk++, state->path.active++) {
@@ -1496,6 +1497,8 @@ xfs_da3_node_lookup_int(
 		    blk->magic == XFS_ATTR3_LEAF_MAGIC) {
 			blk->magic = XFS_ATTR_LEAF_MAGIC;
 			blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
+			if (expected_level == -1U)
+				expected_level = 0;
 			break;
 		}
 
@@ -1504,6 +1507,8 @@ xfs_da3_node_lookup_int(
 			blk->magic = XFS_DIR2_LEAFN_MAGIC;
 			blk->hashval = xfs_dir2_leaf_lasthash(args->dp,
 							      blk->bp, NULL);
+			if (expected_level == -1U)
+				expected_level = 0;
 			break;
 		}
 
@@ -1517,6 +1522,26 @@ xfs_da3_node_lookup_int(
 		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
 		btree = dp->d_ops->node_tree_p(node);
 
+		/* Tree taller than we can handle; bail out! */
+		if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH)
+			return -EFSCORRUPTED;
+
+		if (blkno == args->geo->leafblk) {
+			/*
+			 * This is the root node, set up for the
+			 * next level we want to see.
+			 */
+			expected_level = nodehdr.level - 1;
+		} else if (expected_level != nodehdr.level) {
+			/*
+			 * Not the level we were expecting, which
+			 * implies that the tree is bad.
+			 */
+			return -EFSCORRUPTED;
+		} else {
+			expected_level--;
+		}
+
 		max = nodehdr.count;
 		blk->hashval = be32_to_cpu(btree[max - 1].hashval);
 
@@ -1562,8 +1587,15 @@ xfs_da3_node_lookup_int(
 			blk->index = probe;
 			blkno = be32_to_cpu(btree[probe].before);
 		}
+
+		/* We can't point back to the root. */
+		if (blkno == args->geo->leafblk)
+			return -EFSCORRUPTED;
 	}
 
+	if (expected_level != 0)
+		return -EFSCORRUPTED;
+
 	/*
 	 * A leaf block that ends in the hashval that we are interested in
 	 * (final hashval == search hashval) means that the next block may
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 48423eb..9a8dafc 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -222,6 +222,7 @@ xfs_attr_node_list_lookup(
 	struct xfs_trans		*tp = context->tp;
 	int				i;
 	int				error = 0;
+	unsigned int			expected_level = -1U;
 	uint16_t			magic;
 
 	ASSERT(*pbp == NULL);
@@ -236,6 +237,8 @@ xfs_attr_node_list_lookup(
 		switch (magic) {
 		case XFS_ATTR_LEAF_MAGIC:
 		case XFS_ATTR3_LEAF_MAGIC:
+			if (expected_level == -1U)
+				expected_level = 0;
 			goto found_leaf;
 		case XFS_DA_NODE_MAGIC:
 		case XFS_DA3_NODE_MAGIC:
@@ -249,6 +252,26 @@ xfs_attr_node_list_lookup(
 
 		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
 
+		/* Tree taller than we can handle; bail out! */
+		if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH)
+			goto out_corruptbuf;
+
+		if (cursor->blkno == 0) {
+			/*
+			 * This is the root node, set up for the
+			 * next level we want to see.
+			 */
+			expected_level = nodehdr.level - 1;
+		} else if (expected_level != nodehdr.level) {
+			/*
+			 * Not the level we were expecting, which
+			 * implies that the tree is bad.
+			 */
+			goto out_corruptbuf;
+		} else {
+			expected_level--;
+		}
+
 		btree = dp->d_ops->node_tree_p(node);
 		for (i = 0; i < nodehdr.count; btree++, i++) {
 			if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
@@ -262,15 +285,24 @@ xfs_attr_node_list_lookup(
 			goto out_buf;
 
 		xfs_trans_brelse(tp, *pbp);
+
+		/* We can't point back to the root. */
+		if (cursor->blkno == 0) {
+			error = -EFSCORRUPTED;
+			goto out;
+		}
 	}
 
 found_leaf:
+	if (expected_level != 0)
+		goto out_corruptbuf;
 	return error;
 
 out_corruptbuf:
 	error = -EFSCORRUPTED;
 out_buf:
 	xfs_trans_brelse(tp, *pbp);
+out:
 	*pbp = NULL;
 	return error;
 }

Re: [PATCH 2/4] xfs: abort dir/attr btree operation if btree is obviously weird

[Brian Foster]

On Wed, Oct 25, 2017 at 10:49:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Abort an dir/attr btree operation if the attr btree has obvious problems
> like loops back to the root or pointers don't point down the tree.
> Found by fuzzing btree[0].before to zero in xfs/402, which livelocks on
> the cycle in the attr btree.
> 
> Apply the same checks to xfs_da3_node_lookup_int.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_da_btree.c |   34 +++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_attr_list.c       |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 6d43358..3dbeda6 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -1466,6 +1466,7 @@ xfs_da3_node_lookup_int(
>  	int			max;
>  	int			error;
>  	int			retval;
> +	unsigned int		expected_level = -1U;
>  	struct xfs_inode	*dp = state->args->dp;
>  
>  	args = state->args;
> @@ -1474,7 +1475,7 @@ xfs_da3_node_lookup_int(
>  	 * Descend thru the B-tree searching each level for the right
>  	 * node to use, until the right hashval is found.
>  	 */
> -	blkno = (args->whichfork == XFS_DATA_FORK)? args->geo->leafblk : 0;
> +	blkno = args->geo->leafblk;
>  	for (blk = &state->path.blk[0], state->path.active = 1;
>  			 state->path.active <= XFS_DA_NODE_MAXDEPTH;
>  			 blk++, state->path.active++) {
> @@ -1496,6 +1497,8 @@ xfs_da3_node_lookup_int(
>  		    blk->magic == XFS_ATTR3_LEAF_MAGIC) {
>  			blk->magic = XFS_ATTR_LEAF_MAGIC;
>  			blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
> +			if (expected_level == -1U)
> +				expected_level = 0;

Now that these functions check that expected_level reaches 0, it seems
like we could just initialize it to 0 and eliminate these
checks/assignments..? I think that may also help catch the case a single
node block with an invalid level == 0.

>  			break;
>  		}
>  
> @@ -1504,6 +1507,8 @@ xfs_da3_node_lookup_int(
>  			blk->magic = XFS_DIR2_LEAFN_MAGIC;
>  			blk->hashval = xfs_dir2_leaf_lasthash(args->dp,
>  							      blk->bp, NULL);
> +			if (expected_level == -1U)
> +				expected_level = 0;
>  			break;
>  		}
>  
> @@ -1517,6 +1522,26 @@ xfs_da3_node_lookup_int(
>  		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
>  		btree = dp->d_ops->node_tree_p(node);
>  
> +		/* Tree taller than we can handle; bail out! */
> +		if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH)
> +			return -EFSCORRUPTED;
> +
> +		if (blkno == args->geo->leafblk) {
> +			/*
> +			 * This is the root node, set up for the
> +			 * next level we want to see.
> +			 */
> +			expected_level = nodehdr.level - 1;
> +		} else if (expected_level != nodehdr.level) {
> +			/*
> +			 * Not the level we were expecting, which
> +			 * implies that the tree is bad.
> +			 */
> +			return -EFSCORRUPTED;
> +		} else {
> +			expected_level--;
> +		}
> +

And just an aesthetic nit that these stanzas could probably be made a
little easier to read:

		/*
		 * Track the level from the root node ...
		 */
		if (blk == args->geo->leafblk)
			expected_level = nodehdr.level - 1;
		else if (expected_level != nodehdr.level)
			return -EFSCORRUPTED;
		else
			expected_level--;

Otherwise looks Ok.

Brian

>  		max = nodehdr.count;
>  		blk->hashval = be32_to_cpu(btree[max - 1].hashval);
>  
> @@ -1562,8 +1587,15 @@ xfs_da3_node_lookup_int(
>  			blk->index = probe;
>  			blkno = be32_to_cpu(btree[probe].before);
>  		}
> +
> +		/* We can't point back to the root. */
> +		if (blkno == args->geo->leafblk)
> +			return -EFSCORRUPTED;
>  	}
>  
> +	if (expected_level != 0)
> +		return -EFSCORRUPTED;
> +
>  	/*
>  	 * A leaf block that ends in the hashval that we are interested in
>  	 * (final hashval == search hashval) means that the next block may
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 48423eb..9a8dafc 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -222,6 +222,7 @@ xfs_attr_node_list_lookup(
>  	struct xfs_trans		*tp = context->tp;
>  	int				i;
>  	int				error = 0;
> +	unsigned int			expected_level = -1U;
>  	uint16_t			magic;
>  
>  	ASSERT(*pbp == NULL);
> @@ -236,6 +237,8 @@ xfs_attr_node_list_lookup(
>  		switch (magic) {
>  		case XFS_ATTR_LEAF_MAGIC:
>  		case XFS_ATTR3_LEAF_MAGIC:
> +			if (expected_level == -1U)
> +				expected_level = 0;
>  			goto found_leaf;
>  		case XFS_DA_NODE_MAGIC:
>  		case XFS_DA3_NODE_MAGIC:
> @@ -249,6 +252,26 @@ xfs_attr_node_list_lookup(
>  
>  		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
>  
> +		/* Tree taller than we can handle; bail out! */
> +		if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH)
> +			goto out_corruptbuf;
> +
> +		if (cursor->blkno == 0) {
> +			/*
> +			 * This is the root node, set up for the
> +			 * next level we want to see.
> +			 */
> +			expected_level = nodehdr.level - 1;
> +		} else if (expected_level != nodehdr.level) {
> +			/*
> +			 * Not the level we were expecting, which
> +			 * implies that the tree is bad.
> +			 */
> +			goto out_corruptbuf;
> +		} else {
> +			expected_level--;
> +		}
> +
>  		btree = dp->d_ops->node_tree_p(node);
>  		for (i = 0; i < nodehdr.count; btree++, i++) {
>  			if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
> @@ -262,15 +285,24 @@ xfs_attr_node_list_lookup(
>  			goto out_buf;
>  
>  		xfs_trans_brelse(tp, *pbp);
> +
> +		/* We can't point back to the root. */
> +		if (cursor->blkno == 0) {
> +			error = -EFSCORRUPTED;
> +			goto out;
> +		}
>  	}
>  
>  found_leaf:
> +	if (expected_level != 0)
> +		goto out_corruptbuf;
>  	return error;
>  
>  out_corruptbuf:
>  	error = -EFSCORRUPTED;
>  out_buf:
>  	xfs_trans_brelse(tp, *pbp);
> +out:
>  	*pbp = NULL;
>  	return error;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] xfs: abort dir/attr btree operation if btree is obviously weird

[Darrick J. Wong]

On Thu, Oct 26, 2017 at 09:16:44AM -0400, Brian Foster wrote:
> On Wed, Oct 25, 2017 at 10:49:30PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Abort an dir/attr btree operation if the attr btree has obvious problems
> > like loops back to the root or pointers don't point down the tree.
> > Found by fuzzing btree[0].before to zero in xfs/402, which livelocks on
> > the cycle in the attr btree.
> > 
> > Apply the same checks to xfs_da3_node_lookup_int.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_da_btree.c |   34 +++++++++++++++++++++++++++++++++-
> >  fs/xfs/xfs_attr_list.c       |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> > index 6d43358..3dbeda6 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.c
> > +++ b/fs/xfs/libxfs/xfs_da_btree.c
> > @@ -1466,6 +1466,7 @@ xfs_da3_node_lookup_int(
> >  	int			max;
> >  	int			error;
> >  	int			retval;
> > +	unsigned int		expected_level = -1U;
> >  	struct xfs_inode	*dp = state->args->dp;
> >  
> >  	args = state->args;
> > @@ -1474,7 +1475,7 @@ xfs_da3_node_lookup_int(
> >  	 * Descend thru the B-tree searching each level for the right
> >  	 * node to use, until the right hashval is found.
> >  	 */
> > -	blkno = (args->whichfork == XFS_DATA_FORK)? args->geo->leafblk : 0;
> > +	blkno = args->geo->leafblk;
> >  	for (blk = &state->path.blk[0], state->path.active = 1;
> >  			 state->path.active <= XFS_DA_NODE_MAXDEPTH;
> >  			 blk++, state->path.active++) {
> > @@ -1496,6 +1497,8 @@ xfs_da3_node_lookup_int(
> >  		    blk->magic == XFS_ATTR3_LEAF_MAGIC) {
> >  			blk->magic = XFS_ATTR_LEAF_MAGIC;
> >  			blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
> > +			if (expected_level == -1U)
> > +				expected_level = 0;
> 
> Now that these functions check that expected_level reaches 0, it seems
> like we could just initialize it to 0 and eliminate these
> checks/assignments..? I think that may also help catch the case a single
> node block with an invalid level == 0.

Hmm, you're right, it's fine to initialize it to zero since there's no
way we fail to iterate the loop at least once.

> >  			break;
> >  		}
> >  
> > @@ -1504,6 +1507,8 @@ xfs_da3_node_lookup_int(
> >  			blk->magic = XFS_DIR2_LEAFN_MAGIC;
> >  			blk->hashval = xfs_dir2_leaf_lasthash(args->dp,
> >  							      blk->bp, NULL);
> > +			if (expected_level == -1U)
> > +				expected_level = 0;
> >  			break;
> >  		}
> >  
> > @@ -1517,6 +1522,26 @@ xfs_da3_node_lookup_int(
> >  		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> >  		btree = dp->d_ops->node_tree_p(node);
> >  
> > +		/* Tree taller than we can handle; bail out! */
> > +		if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH)
> > +			return -EFSCORRUPTED;
> > +
> > +		if (blkno == args->geo->leafblk) {
> > +			/*
> > +			 * This is the root node, set up for the
> > +			 * next level we want to see.
> > +			 */
> > +			expected_level = nodehdr.level - 1;
> > +		} else if (expected_level != nodehdr.level) {
> > +			/*
> > +			 * Not the level we were expecting, which
> > +			 * implies that the tree is bad.
> > +			 */
> > +			return -EFSCORRUPTED;
> > +		} else {
> > +			expected_level--;
> > +		}
> > +
> 
> And just an aesthetic nit that these stanzas could probably be made a
> little easier to read:
> 
> 		/*
> 		 * Track the level from the root node ...
> 		 */
> 		if (blk == args->geo->leafblk)
> 			expected_level = nodehdr.level - 1;
> 		else if (expected_level != nodehdr.level)
> 			return -EFSCORRUPTED;
> 		else
> 			expected_level--;
> 
> Otherwise looks Ok.

Ok.

--D

> 
> Brian
> 
> >  		max = nodehdr.count;
> >  		blk->hashval = be32_to_cpu(btree[max - 1].hashval);
> >  
> > @@ -1562,8 +1587,15 @@ xfs_da3_node_lookup_int(
> >  			blk->index = probe;
> >  			blkno = be32_to_cpu(btree[probe].before);
> >  		}
> > +
> > +		/* We can't point back to the root. */
> > +		if (blkno == args->geo->leafblk)
> > +			return -EFSCORRUPTED;
> >  	}
> >  
> > +	if (expected_level != 0)
> > +		return -EFSCORRUPTED;
> > +
> >  	/*
> >  	 * A leaf block that ends in the hashval that we are interested in
> >  	 * (final hashval == search hashval) means that the next block may
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 48423eb..9a8dafc 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> > @@ -222,6 +222,7 @@ xfs_attr_node_list_lookup(
> >  	struct xfs_trans		*tp = context->tp;
> >  	int				i;
> >  	int				error = 0;
> > +	unsigned int			expected_level = -1U;
> >  	uint16_t			magic;
> >  
> >  	ASSERT(*pbp == NULL);
> > @@ -236,6 +237,8 @@ xfs_attr_node_list_lookup(
> >  		switch (magic) {
> >  		case XFS_ATTR_LEAF_MAGIC:
> >  		case XFS_ATTR3_LEAF_MAGIC:
> > +			if (expected_level == -1U)
> > +				expected_level = 0;
> >  			goto found_leaf;
> >  		case XFS_DA_NODE_MAGIC:
> >  		case XFS_DA3_NODE_MAGIC:
> > @@ -249,6 +252,26 @@ xfs_attr_node_list_lookup(
> >  
> >  		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> >  
> > +		/* Tree taller than we can handle; bail out! */
> > +		if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH)
> > +			goto out_corruptbuf;
> > +
> > +		if (cursor->blkno == 0) {
> > +			/*
> > +			 * This is the root node, set up for the
> > +			 * next level we want to see.
> > +			 */
> > +			expected_level = nodehdr.level - 1;
> > +		} else if (expected_level != nodehdr.level) {
> > +			/*
> > +			 * Not the level we were expecting, which
> > +			 * implies that the tree is bad.
> > +			 */
> > +			goto out_corruptbuf;
> > +		} else {
> > +			expected_level--;
> > +		}
> > +
> >  		btree = dp->d_ops->node_tree_p(node);
> >  		for (i = 0; i < nodehdr.count; btree++, i++) {
> >  			if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
> > @@ -262,15 +285,24 @@ xfs_attr_node_list_lookup(
> >  			goto out_buf;
> >  
> >  		xfs_trans_brelse(tp, *pbp);
> > +
> > +		/* We can't point back to the root. */
> > +		if (cursor->blkno == 0) {
> > +			error = -EFSCORRUPTED;
> > +			goto out;
> > +		}
> >  	}
> >  
> >  found_leaf:
> > +	if (expected_level != 0)
> > +		goto out_corruptbuf;
> >  	return error;
> >  
> >  out_corruptbuf:
> >  	error = -EFSCORRUPTED;
> >  out_buf:
> >  	xfs_trans_brelse(tp, *pbp);
> > +out:
> >  	*pbp = NULL;
> >  	return error;
> >  }
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] xfs: validate sb_logsunit is a multiple of the fs blocksize

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure the log stripe unit is sane before proceeding with mounting.
AFAICT this means that logsunit has to be 0, 1, or a multiple of the fs
block size.  Found this by setting the LSB of logsunit in xfs/350 and
watching the system crash as soon as we try to write to the log.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_log.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index dc95a49..4edb36f 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -608,6 +608,7 @@ xfs_log_mount(
 	xfs_daddr_t	blk_offset,
 	int		num_bblks)
 {
+	bool		fatal = xfs_sb_version_hascrc(&mp->m_sb);
 	int		error = 0;
 	int		min_logfsbs;
 
@@ -659,9 +660,20 @@ xfs_log_mount(
 			 XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks),
 			 XFS_MAX_LOG_BYTES);
 		error = -EINVAL;
+	} else if (mp->m_sb.sb_logsunit > 1 &&
+		   mp->m_sb.sb_logsunit % mp->m_sb.sb_blocksize) {
+		xfs_warn(mp,
+		"log stripe unit %u bytes must be a multiple of block size",
+			 mp->m_sb.sb_logsunit);
+		error = -EINVAL;
+		fatal = true;
 	}
 	if (error) {
-		if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		/*
+		 * Log check errors are always fatal on v5; or whenever bad
+		 * metadata leads to a crash.
+		 */
+		if (fatal) {
 			xfs_crit(mp, "AAIEEE! Log failed size checks. Abort!");
 			ASSERT(0);
 			goto out_free_log;

Re: [PATCH 3/4] xfs: validate sb_logsunit is a multiple of the fs blocksize

[Brian Foster]

On Wed, Oct 25, 2017 at 10:49:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure the log stripe unit is sane before proceeding with mounting.
> AFAICT this means that logsunit has to be 0, 1, or a multiple of the fs
> block size.  Found this by setting the LSB of logsunit in xfs/350 and
> watching the system crash as soon as we try to write to the log.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index dc95a49..4edb36f 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -608,6 +608,7 @@ xfs_log_mount(
>  	xfs_daddr_t	blk_offset,
>  	int		num_bblks)
>  {
> +	bool		fatal = xfs_sb_version_hascrc(&mp->m_sb);
>  	int		error = 0;
>  	int		min_logfsbs;
>  
> @@ -659,9 +660,20 @@ xfs_log_mount(
>  			 XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks),
>  			 XFS_MAX_LOG_BYTES);
>  		error = -EINVAL;
> +	} else if (mp->m_sb.sb_logsunit > 1 &&
> +		   mp->m_sb.sb_logsunit % mp->m_sb.sb_blocksize) {
> +		xfs_warn(mp,
> +		"log stripe unit %u bytes must be a multiple of block size",
> +			 mp->m_sb.sb_logsunit);
> +		error = -EINVAL;
> +		fatal = true;
>  	}
>  	if (error) {
> -		if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		/*
> +		 * Log check errors are always fatal on v5; or whenever bad
> +		 * metadata leads to a crash.
> +		 */
> +		if (fatal) {
>  			xfs_crit(mp, "AAIEEE! Log failed size checks. Abort!");
>  			ASSERT(0);
>  			goto out_free_log;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] xfs: compare btree block keys to parent block's keys during scrub

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

When we're done checking all the records/keys in a btree block, compute
the low and high key of the block and compare them to the associated key
in the parent btree block.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_btree.c |    4 ++--
 fs/xfs/libxfs/xfs_btree.h |    4 ++++
 fs/xfs/scrub/btree.c      |   46 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index b3cd82a..848f371 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -2027,7 +2027,7 @@ xfs_btree_lookup(
 }
 
 /* Find the high key storage area from a regular key. */
-STATIC union xfs_btree_key *
+union xfs_btree_key *
 xfs_btree_high_key_from_key(
 	struct xfs_btree_cur	*cur,
 	union xfs_btree_key	*key)
@@ -2101,7 +2101,7 @@ xfs_btree_get_node_keys(
 }
 
 /* Derive the keys for any btree block. */
-STATIC void
+void
 xfs_btree_get_keys(
 	struct xfs_btree_cur	*cur,
 	struct xfs_btree_block	*block,
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index be82f41..b57501c 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -541,5 +541,9 @@ int64_t xfs_btree_diff_two_ptrs(struct xfs_btree_cur *cur,
 void xfs_btree_get_sibling(struct xfs_btree_cur *cur,
 			   struct xfs_btree_block *block,
 			   union xfs_btree_ptr *ptr, int lr);
+void xfs_btree_get_keys(struct xfs_btree_cur *cur,
+		struct xfs_btree_block *block, union xfs_btree_key *key);
+union xfs_btree_key *xfs_btree_high_key_from_key(struct xfs_btree_cur *cur,
+		union xfs_btree_key *key);
 
 #endif	/* __XFS_BTREE_H__ */
diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
index 9ccf763..9e8b67a 100644
--- a/fs/xfs/scrub/btree.c
+++ b/fs/xfs/scrub/btree.c
@@ -358,6 +358,50 @@ xfs_scrub_btree_get_block(
 }
 
 /*
+ * Check that the low and high keys of this block match the keys stored
+ * in the parent block.
+ */
+STATIC void
+xfs_scrub_btree_block_keys(
+	struct xfs_scrub_btree		*bs,
+	int				level,
+	struct xfs_btree_block		*block)
+{
+	union xfs_btree_key		block_keys;
+	struct xfs_btree_cur		*cur = bs->cur;
+	union xfs_btree_key		*high_bk;
+	union xfs_btree_key		*parent_keys;
+	union xfs_btree_key		*high_pk;
+	struct xfs_btree_block		*parent_block;
+	struct xfs_buf			*bp;
+
+	if (level >= cur->bc_nlevels - 1)
+		return;
+
+	/* Calculate the keys for this block. */
+	xfs_btree_get_keys(cur, block, &block_keys);
+
+	/* Obtain the parent's copy of the keys for this block. */
+	parent_block = xfs_btree_get_block(cur, level + 1, &bp);
+	parent_keys = xfs_btree_key_addr(cur, cur->bc_ptrs[level + 1],
+			parent_block);
+
+	if (cur->bc_ops->diff_two_keys(cur, &block_keys, parent_keys) != 0)
+		xfs_scrub_btree_set_corrupt(bs->sc, cur, 1);
+
+	if (!(cur->bc_flags & XFS_BTREE_OVERLAPPING))
+		return;
+
+	/* Get high keys */
+	high_bk = xfs_btree_high_key_from_key(cur, &block_keys);
+	high_pk = xfs_btree_high_key_addr(cur, cur->bc_ptrs[level + 1],
+			parent_block);
+
+	if (cur->bc_ops->diff_two_keys(cur, high_bk, high_pk) != 0)
+		xfs_scrub_btree_set_corrupt(bs->sc, cur, 1);
+}
+
+/*
  * Visit all nodes and leaves of a btree.  Check that all pointers and
  * records are in order, that the keys reflect the records, and use a callback
  * so that the caller can verify individual records.
@@ -418,6 +462,7 @@ xfs_scrub_btree(
 			/* End of leaf, pop back towards the root. */
 			if (cur->bc_ptrs[level] >
 			    be16_to_cpu(block->bb_numrecs)) {
+				xfs_scrub_btree_block_keys(&bs, level, block);
 				if (level < cur->bc_nlevels - 1)
 					cur->bc_ptrs[level + 1]++;
 				level++;
@@ -442,6 +487,7 @@ xfs_scrub_btree(
 
 		/* End of node, pop back towards the root. */
 		if (cur->bc_ptrs[level] > be16_to_cpu(block->bb_numrecs)) {
+			xfs_scrub_btree_block_keys(&bs, level, block);
 			if (level < cur->bc_nlevels - 1)
 				cur->bc_ptrs[level + 1]++;
 			level++;

Re: [PATCH 4/4] xfs: compare btree block keys to parent block's keys during scrub

[Brian Foster]

On Wed, Oct 25, 2017 at 10:49:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're done checking all the records/keys in a btree block, compute
> the low and high key of the block and compare them to the associated key
> in the parent btree block.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I guess this one should be stacked on top of a scrub series.. (or the
associated code merged)? Doesn't apply to for-next..

Brian

>  fs/xfs/libxfs/xfs_btree.c |    4 ++--
>  fs/xfs/libxfs/xfs_btree.h |    4 ++++
>  fs/xfs/scrub/btree.c      |   46 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index b3cd82a..848f371 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -2027,7 +2027,7 @@ xfs_btree_lookup(
>  }
>  
>  /* Find the high key storage area from a regular key. */
> -STATIC union xfs_btree_key *
> +union xfs_btree_key *
>  xfs_btree_high_key_from_key(
>  	struct xfs_btree_cur	*cur,
>  	union xfs_btree_key	*key)
> @@ -2101,7 +2101,7 @@ xfs_btree_get_node_keys(
>  }
>  
>  /* Derive the keys for any btree block. */
> -STATIC void
> +void
>  xfs_btree_get_keys(
>  	struct xfs_btree_cur	*cur,
>  	struct xfs_btree_block	*block,
> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> index be82f41..b57501c 100644
> --- a/fs/xfs/libxfs/xfs_btree.h
> +++ b/fs/xfs/libxfs/xfs_btree.h
> @@ -541,5 +541,9 @@ int64_t xfs_btree_diff_two_ptrs(struct xfs_btree_cur *cur,
>  void xfs_btree_get_sibling(struct xfs_btree_cur *cur,
>  			   struct xfs_btree_block *block,
>  			   union xfs_btree_ptr *ptr, int lr);
> +void xfs_btree_get_keys(struct xfs_btree_cur *cur,
> +		struct xfs_btree_block *block, union xfs_btree_key *key);
> +union xfs_btree_key *xfs_btree_high_key_from_key(struct xfs_btree_cur *cur,
> +		union xfs_btree_key *key);
>  
>  #endif	/* __XFS_BTREE_H__ */
> diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
> index 9ccf763..9e8b67a 100644
> --- a/fs/xfs/scrub/btree.c
> +++ b/fs/xfs/scrub/btree.c
> @@ -358,6 +358,50 @@ xfs_scrub_btree_get_block(
>  }
>  
>  /*
> + * Check that the low and high keys of this block match the keys stored
> + * in the parent block.
> + */
> +STATIC void
> +xfs_scrub_btree_block_keys(
> +	struct xfs_scrub_btree		*bs,
> +	int				level,
> +	struct xfs_btree_block		*block)
> +{
> +	union xfs_btree_key		block_keys;
> +	struct xfs_btree_cur		*cur = bs->cur;
> +	union xfs_btree_key		*high_bk;
> +	union xfs_btree_key		*parent_keys;
> +	union xfs_btree_key		*high_pk;
> +	struct xfs_btree_block		*parent_block;
> +	struct xfs_buf			*bp;
> +
> +	if (level >= cur->bc_nlevels - 1)
> +		return;
> +
> +	/* Calculate the keys for this block. */
> +	xfs_btree_get_keys(cur, block, &block_keys);
> +
> +	/* Obtain the parent's copy of the keys for this block. */
> +	parent_block = xfs_btree_get_block(cur, level + 1, &bp);
> +	parent_keys = xfs_btree_key_addr(cur, cur->bc_ptrs[level + 1],
> +			parent_block);
> +
> +	if (cur->bc_ops->diff_two_keys(cur, &block_keys, parent_keys) != 0)
> +		xfs_scrub_btree_set_corrupt(bs->sc, cur, 1);
> +
> +	if (!(cur->bc_flags & XFS_BTREE_OVERLAPPING))
> +		return;
> +
> +	/* Get high keys */
> +	high_bk = xfs_btree_high_key_from_key(cur, &block_keys);
> +	high_pk = xfs_btree_high_key_addr(cur, cur->bc_ptrs[level + 1],
> +			parent_block);
> +
> +	if (cur->bc_ops->diff_two_keys(cur, high_bk, high_pk) != 0)
> +		xfs_scrub_btree_set_corrupt(bs->sc, cur, 1);
> +}
> +
> +/*
>   * Visit all nodes and leaves of a btree.  Check that all pointers and
>   * records are in order, that the keys reflect the records, and use a callback
>   * so that the caller can verify individual records.
> @@ -418,6 +462,7 @@ xfs_scrub_btree(
>  			/* End of leaf, pop back towards the root. */
>  			if (cur->bc_ptrs[level] >
>  			    be16_to_cpu(block->bb_numrecs)) {
> +				xfs_scrub_btree_block_keys(&bs, level, block);
>  				if (level < cur->bc_nlevels - 1)
>  					cur->bc_ptrs[level + 1]++;
>  				level++;
> @@ -442,6 +487,7 @@ xfs_scrub_btree(
>  
>  		/* End of node, pop back towards the root. */
>  		if (cur->bc_ptrs[level] > be16_to_cpu(block->bb_numrecs)) {
> +			xfs_scrub_btree_block_keys(&bs, level, block);
>  			if (level < cur->bc_nlevels - 1)
>  				cur->bc_ptrs[level + 1]++;
>  			level++;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] xfs: compare btree block keys to parent block's keys during scrub

[Darrick J. Wong]

On Thu, Oct 26, 2017 at 09:16:56AM -0400, Brian Foster wrote:
> On Wed, Oct 25, 2017 at 10:49:43PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we're done checking all the records/keys in a btree block, compute
> > the low and high key of the block and compare them to the associated key
> > in the parent btree block.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> I guess this one should be stacked on top of a scrub series.. (or the
> associated code merged)? Doesn't apply to for-next..

Yes, it goes atop the scrub series ... which means I forgot to push
for-next last night. :(

--D

> Brian
> 
> >  fs/xfs/libxfs/xfs_btree.c |    4 ++--
> >  fs/xfs/libxfs/xfs_btree.h |    4 ++++
> >  fs/xfs/scrub/btree.c      |   46 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 52 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> > index b3cd82a..848f371 100644
> > --- a/fs/xfs/libxfs/xfs_btree.c
> > +++ b/fs/xfs/libxfs/xfs_btree.c
> > @@ -2027,7 +2027,7 @@ xfs_btree_lookup(
> >  }
> >  
> >  /* Find the high key storage area from a regular key. */
> > -STATIC union xfs_btree_key *
> > +union xfs_btree_key *
> >  xfs_btree_high_key_from_key(
> >  	struct xfs_btree_cur	*cur,
> >  	union xfs_btree_key	*key)
> > @@ -2101,7 +2101,7 @@ xfs_btree_get_node_keys(
> >  }
> >  
> >  /* Derive the keys for any btree block. */
> > -STATIC void
> > +void
> >  xfs_btree_get_keys(
> >  	struct xfs_btree_cur	*cur,
> >  	struct xfs_btree_block	*block,
> > diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> > index be82f41..b57501c 100644
> > --- a/fs/xfs/libxfs/xfs_btree.h
> > +++ b/fs/xfs/libxfs/xfs_btree.h
> > @@ -541,5 +541,9 @@ int64_t xfs_btree_diff_two_ptrs(struct xfs_btree_cur *cur,
> >  void xfs_btree_get_sibling(struct xfs_btree_cur *cur,
> >  			   struct xfs_btree_block *block,
> >  			   union xfs_btree_ptr *ptr, int lr);
> > +void xfs_btree_get_keys(struct xfs_btree_cur *cur,
> > +		struct xfs_btree_block *block, union xfs_btree_key *key);
> > +union xfs_btree_key *xfs_btree_high_key_from_key(struct xfs_btree_cur *cur,
> > +		union xfs_btree_key *key);
> >  
> >  #endif	/* __XFS_BTREE_H__ */
> > diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
> > index 9ccf763..9e8b67a 100644
> > --- a/fs/xfs/scrub/btree.c
> > +++ b/fs/xfs/scrub/btree.c
> > @@ -358,6 +358,50 @@ xfs_scrub_btree_get_block(
> >  }
> >  
> >  /*
> > + * Check that the low and high keys of this block match the keys stored
> > + * in the parent block.
> > + */
> > +STATIC void
> > +xfs_scrub_btree_block_keys(
> > +	struct xfs_scrub_btree		*bs,
> > +	int				level,
> > +	struct xfs_btree_block		*block)
> > +{
> > +	union xfs_btree_key		block_keys;
> > +	struct xfs_btree_cur		*cur = bs->cur;
> > +	union xfs_btree_key		*high_bk;
> > +	union xfs_btree_key		*parent_keys;
> > +	union xfs_btree_key		*high_pk;
> > +	struct xfs_btree_block		*parent_block;
> > +	struct xfs_buf			*bp;
> > +
> > +	if (level >= cur->bc_nlevels - 1)
> > +		return;
> > +
> > +	/* Calculate the keys for this block. */
> > +	xfs_btree_get_keys(cur, block, &block_keys);
> > +
> > +	/* Obtain the parent's copy of the keys for this block. */
> > +	parent_block = xfs_btree_get_block(cur, level + 1, &bp);
> > +	parent_keys = xfs_btree_key_addr(cur, cur->bc_ptrs[level + 1],
> > +			parent_block);
> > +
> > +	if (cur->bc_ops->diff_two_keys(cur, &block_keys, parent_keys) != 0)
> > +		xfs_scrub_btree_set_corrupt(bs->sc, cur, 1);
> > +
> > +	if (!(cur->bc_flags & XFS_BTREE_OVERLAPPING))
> > +		return;
> > +
> > +	/* Get high keys */
> > +	high_bk = xfs_btree_high_key_from_key(cur, &block_keys);
> > +	high_pk = xfs_btree_high_key_addr(cur, cur->bc_ptrs[level + 1],
> > +			parent_block);
> > +
> > +	if (cur->bc_ops->diff_two_keys(cur, high_bk, high_pk) != 0)
> > +		xfs_scrub_btree_set_corrupt(bs->sc, cur, 1);
> > +}
> > +
> > +/*
> >   * Visit all nodes and leaves of a btree.  Check that all pointers and
> >   * records are in order, that the keys reflect the records, and use a callback
> >   * so that the caller can verify individual records.
> > @@ -418,6 +462,7 @@ xfs_scrub_btree(
> >  			/* End of leaf, pop back towards the root. */
> >  			if (cur->bc_ptrs[level] >
> >  			    be16_to_cpu(block->bb_numrecs)) {
> > +				xfs_scrub_btree_block_keys(&bs, level, block);
> >  				if (level < cur->bc_nlevels - 1)
> >  					cur->bc_ptrs[level + 1]++;
> >  				level++;
> > @@ -442,6 +487,7 @@ xfs_scrub_btree(
> >  
> >  		/* End of node, pop back towards the root. */
> >  		if (cur->bc_ptrs[level] > be16_to_cpu(block->bb_numrecs)) {
> > +			xfs_scrub_btree_block_keys(&bs, level, block);
> >  			if (level < cur->bc_nlevels - 1)
> >  				cur->bc_ptrs[level + 1]++;
> >  			level++;
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] xfs: refactor extended attribute list operation

[Brian Foster]

On Wed, Oct 25, 2017 at 10:49:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're iterating the attribute list and we can't find our previous
> location based off the attribute cursor, we'll instead walk down the
> attribute btree from the root trying to find where we left off.  Move
> this code into a separate function for later cleanups.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_attr_list.c |  136 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 84 insertions(+), 52 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 5816786..48423eb 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -204,19 +204,89 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
>  	return 0;
>  }
>  
> +/*
> + * We didn't find the block & hash mentioned in the cursor state, so
> + * walk down the attr btree looking for the hash.
> + */
>  STATIC int
> -xfs_attr_node_list(xfs_attr_list_context_t *context)
> +xfs_attr_node_list_lookup(
> +	struct xfs_attr_list_context	*context,
> +	struct attrlist_cursor_kern	*cursor,
> +	struct xfs_buf			**pbp)

Looks Ok, but the label usage at the bottom seems a bit gratuitous...

>  {
> -	attrlist_cursor_kern_t *cursor;
> -	xfs_attr_leafblock_t *leaf;
> -	xfs_da_intnode_t *node;
> -	struct xfs_attr3_icleaf_hdr leafhdr;
> -	struct xfs_da3_icnode_hdr nodehdr;
> -	struct xfs_da_node_entry *btree;
> -	int error, i;
> -	struct xfs_buf *bp;
> -	struct xfs_inode	*dp = context->dp;
> -	struct xfs_mount	*mp = dp->i_mount;
> +	struct xfs_da3_icnode_hdr	nodehdr;
> +	struct xfs_da_intnode		*node;
> +	struct xfs_da_node_entry	*btree;
> +	struct xfs_inode		*dp = context->dp;
> +	struct xfs_mount		*mp = dp->i_mount;
> +	struct xfs_trans		*tp = context->tp;
> +	int				i;
> +	int				error = 0;
> +	uint16_t			magic;
> +
> +	ASSERT(*pbp == NULL);
> +	cursor->blkno = 0;
> +	for (;;) {
> +		error = xfs_da3_node_read(tp, dp, cursor->blkno, -1, pbp,
> +				XFS_ATTR_FORK);
> +		if (error)
> +			return error;
> +		node = (*pbp)->b_addr;
> +		magic = be16_to_cpu(node->hdr.info.magic);
> +		switch (magic) {
> +		case XFS_ATTR_LEAF_MAGIC:
> +		case XFS_ATTR3_LEAF_MAGIC:
> +			goto found_leaf;

The label after the loop suggests a break should be sufficient, but the
switch statement conflicts with that. So why not just use the if logic
from the original code and kill that label?

> +		case XFS_DA_NODE_MAGIC:
> +		case XFS_DA3_NODE_MAGIC:
> +			/* process btree node below */
> +			break;
> +		default:
> +			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> +					node);
> +			goto out_corruptbuf;
> +		}
> +
> +		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> +
> +		btree = dp->d_ops->node_tree_p(node);
> +		for (i = 0; i < nodehdr.count; btree++, i++) {
> +			if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
> +				cursor->blkno = be32_to_cpu(btree->before);
> +				trace_xfs_attr_list_node_descend(context,
> +						btree);
> +				break;
> +			}
> +		}
> +		if (i == nodehdr.count)
> +			goto out_buf;

We can move this after the line below to pick up the brelse(). Also, if
we use a local bp pointer and assign pbp = &bp in the one case we know
we found something, that eliminates the need to clear pbp in the
multiple other cases. It also means we can now just return directly from
any checks after the brelse() in the loop.

> +
> +		xfs_trans_brelse(tp, *pbp);
> +	}
> +
> +found_leaf:
> +	return error;
> +
> +out_corruptbuf:
> +	error = -EFSCORRUPTED;

Might as well just return -EFSCORRUPTED. We already return directly from
the only place error is potentially assigned anything else.

> +out_buf:
> +	xfs_trans_brelse(tp, *pbp);
> +	*pbp = NULL;
> +	return error;

With all of that and from taking a quick look at the next patch, I think
we should be able to end up with something like the following logic with
only a single label for the corrupted buf case, which is now common
between the existing magic val corruption check and the new level
checks.

		...
	}

	if (expected_level != 0)
		goto out_corruptbp;
	*pbp = bp;
	return 0;

out_corruptbp:
	xfs_trans_brelse(tp, bp);
	return -EFSCORRUPTED;
}

Brian

> +}
> +
> +STATIC int
> +xfs_attr_node_list(
> +	struct xfs_attr_list_context	*context)
> +{
> +	struct xfs_attr3_icleaf_hdr	leafhdr;
> +	struct attrlist_cursor_kern	*cursor;
> +	struct xfs_attr_leafblock	*leaf;
> +	struct xfs_da_intnode		*node;
> +	struct xfs_buf			*bp;
> +	struct xfs_inode		*dp = context->dp;
> +	struct xfs_mount		*mp = dp->i_mount;
> +	int				error;
>  
>  	trace_xfs_attr_node_list(context);
>  
> @@ -277,47 +347,9 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
>  	 * Note that start of node block is same as start of leaf block.
>  	 */
>  	if (bp == NULL) {
> -		cursor->blkno = 0;
> -		for (;;) {
> -			uint16_t magic;
> -
> -			error = xfs_da3_node_read(context->tp, dp,
> -						      cursor->blkno, -1, &bp,
> -						      XFS_ATTR_FORK);
> -			if (error)
> -				return error;
> -			node = bp->b_addr;
> -			magic = be16_to_cpu(node->hdr.info.magic);
> -			if (magic == XFS_ATTR_LEAF_MAGIC ||
> -			    magic == XFS_ATTR3_LEAF_MAGIC)
> -				break;
> -			if (magic != XFS_DA_NODE_MAGIC &&
> -			    magic != XFS_DA3_NODE_MAGIC) {
> -				XFS_CORRUPTION_ERROR("xfs_attr_node_list(3)",
> -						     XFS_ERRLEVEL_LOW,
> -						     context->dp->i_mount,
> -						     node);
> -				xfs_trans_brelse(context->tp, bp);
> -				return -EFSCORRUPTED;
> -			}
> -
> -			dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> -			btree = dp->d_ops->node_tree_p(node);
> -			for (i = 0; i < nodehdr.count; btree++, i++) {
> -				if (cursor->hashval
> -						<= be32_to_cpu(btree->hashval)) {
> -					cursor->blkno = be32_to_cpu(btree->before);
> -					trace_xfs_attr_list_node_descend(context,
> -									 btree);
> -					break;
> -				}
> -			}
> -			if (i == nodehdr.count) {
> -				xfs_trans_brelse(context->tp, bp);
> -				return 0;
> -			}
> -			xfs_trans_brelse(context->tp, bp);
> -		}
> +		error = xfs_attr_node_list_lookup(context, cursor, &bp);
> +		if (error || !bp)
> +			return error;
>  	}
>  	ASSERT(bp != NULL);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] xfs: refactor extended attribute list operation

[Darrick J. Wong]

On Thu, Oct 26, 2017 at 09:16:37AM -0400, Brian Foster wrote:
> On Wed, Oct 25, 2017 at 10:49:24PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we're iterating the attribute list and we can't find our previous
> > location based off the attribute cursor, we'll instead walk down the
> > attribute btree from the root trying to find where we left off.  Move
> > this code into a separate function for later cleanups.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_attr_list.c |  136 ++++++++++++++++++++++++++++++------------------
> >  1 file changed, 84 insertions(+), 52 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 5816786..48423eb 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> > @@ -204,19 +204,89 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * We didn't find the block & hash mentioned in the cursor state, so
> > + * walk down the attr btree looking for the hash.
> > + */
> >  STATIC int
> > -xfs_attr_node_list(xfs_attr_list_context_t *context)
> > +xfs_attr_node_list_lookup(
> > +	struct xfs_attr_list_context	*context,
> > +	struct attrlist_cursor_kern	*cursor,
> > +	struct xfs_buf			**pbp)
> 
> Looks Ok, but the label usage at the bottom seems a bit gratuitous...
> 
> >  {
> > -	attrlist_cursor_kern_t *cursor;
> > -	xfs_attr_leafblock_t *leaf;
> > -	xfs_da_intnode_t *node;
> > -	struct xfs_attr3_icleaf_hdr leafhdr;
> > -	struct xfs_da3_icnode_hdr nodehdr;
> > -	struct xfs_da_node_entry *btree;
> > -	int error, i;
> > -	struct xfs_buf *bp;
> > -	struct xfs_inode	*dp = context->dp;
> > -	struct xfs_mount	*mp = dp->i_mount;
> > +	struct xfs_da3_icnode_hdr	nodehdr;
> > +	struct xfs_da_intnode		*node;
> > +	struct xfs_da_node_entry	*btree;
> > +	struct xfs_inode		*dp = context->dp;
> > +	struct xfs_mount		*mp = dp->i_mount;
> > +	struct xfs_trans		*tp = context->tp;
> > +	int				i;
> > +	int				error = 0;
> > +	uint16_t			magic;
> > +
> > +	ASSERT(*pbp == NULL);
> > +	cursor->blkno = 0;
> > +	for (;;) {
> > +		error = xfs_da3_node_read(tp, dp, cursor->blkno, -1, pbp,
> > +				XFS_ATTR_FORK);
> > +		if (error)
> > +			return error;
> > +		node = (*pbp)->b_addr;
> > +		magic = be16_to_cpu(node->hdr.info.magic);
> > +		switch (magic) {
> > +		case XFS_ATTR_LEAF_MAGIC:
> > +		case XFS_ATTR3_LEAF_MAGIC:
> > +			goto found_leaf;
> 
> The label after the loop suggests a break should be sufficient, but the
> switch statement conflicts with that. So why not just use the if logic
> from the original code and kill that label?

Ok.

> > +		case XFS_DA_NODE_MAGIC:
> > +		case XFS_DA3_NODE_MAGIC:
> > +			/* process btree node below */
> > +			break;
> > +		default:
> > +			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> > +					node);
> > +			goto out_corruptbuf;
> > +		}
> > +
> > +		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> > +
> > +		btree = dp->d_ops->node_tree_p(node);
> > +		for (i = 0; i < nodehdr.count; btree++, i++) {
> > +			if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
> > +				cursor->blkno = be32_to_cpu(btree->before);
> > +				trace_xfs_attr_list_node_descend(context,
> > +						btree);
> > +				break;
> > +			}
> > +		}
> > +		if (i == nodehdr.count)
> > +			goto out_buf;
> 
> We can move this after the line below to pick up the brelse(). Also, if
> we use a local bp pointer and assign pbp = &bp in the one case we know
> we found something, that eliminates the need to clear pbp in the
> multiple other cases. It also means we can now just return directly from
> any checks after the brelse() in the loop.
> 
> > +
> > +		xfs_trans_brelse(tp, *pbp);
> > +	}
> > +
> > +found_leaf:
> > +	return error;
> > +
> > +out_corruptbuf:
> > +	error = -EFSCORRUPTED;
> 
> Might as well just return -EFSCORRUPTED. We already return directly from
> the only place error is potentially assigned anything else.
> 
> > +out_buf:
> > +	xfs_trans_brelse(tp, *pbp);
> > +	*pbp = NULL;
> > +	return error;
> 
> With all of that and from taking a quick look at the next patch, I think
> we should be able to end up with something like the following logic with
> only a single label for the corrupted buf case, which is now common
> between the existing magic val corruption check and the new level
> checks.
> 
> 		...
> 	}
> 
> 	if (expected_level != 0)
> 		goto out_corruptbp;
> 	*pbp = bp;
> 	return 0;
> 
> out_corruptbp:
> 	xfs_trans_brelse(tp, bp);
> 	return -EFSCORRUPTED;

Ok, will do.

--D

> }
> 
> Brian
> 
> > +}
> > +
> > +STATIC int
> > +xfs_attr_node_list(
> > +	struct xfs_attr_list_context	*context)
> > +{
> > +	struct xfs_attr3_icleaf_hdr	leafhdr;
> > +	struct attrlist_cursor_kern	*cursor;
> > +	struct xfs_attr_leafblock	*leaf;
> > +	struct xfs_da_intnode		*node;
> > +	struct xfs_buf			*bp;
> > +	struct xfs_inode		*dp = context->dp;
> > +	struct xfs_mount		*mp = dp->i_mount;
> > +	int				error;
> >  
> >  	trace_xfs_attr_node_list(context);
> >  
> > @@ -277,47 +347,9 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
> >  	 * Note that start of node block is same as start of leaf block.
> >  	 */
> >  	if (bp == NULL) {
> > -		cursor->blkno = 0;
> > -		for (;;) {
> > -			uint16_t magic;
> > -
> > -			error = xfs_da3_node_read(context->tp, dp,
> > -						      cursor->blkno, -1, &bp,
> > -						      XFS_ATTR_FORK);
> > -			if (error)
> > -				return error;
> > -			node = bp->b_addr;
> > -			magic = be16_to_cpu(node->hdr.info.magic);
> > -			if (magic == XFS_ATTR_LEAF_MAGIC ||
> > -			    magic == XFS_ATTR3_LEAF_MAGIC)
> > -				break;
> > -			if (magic != XFS_DA_NODE_MAGIC &&
> > -			    magic != XFS_DA3_NODE_MAGIC) {
> > -				XFS_CORRUPTION_ERROR("xfs_attr_node_list(3)",
> > -						     XFS_ERRLEVEL_LOW,
> > -						     context->dp->i_mount,
> > -						     node);
> > -				xfs_trans_brelse(context->tp, bp);
> > -				return -EFSCORRUPTED;
> > -			}
> > -
> > -			dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> > -			btree = dp->d_ops->node_tree_p(node);
> > -			for (i = 0; i < nodehdr.count; btree++, i++) {
> > -				if (cursor->hashval
> > -						<= be32_to_cpu(btree->hashval)) {
> > -					cursor->blkno = be32_to_cpu(btree->before);
> > -					trace_xfs_attr_list_node_descend(context,
> > -									 btree);
> > -					break;
> > -				}
> > -			}
> > -			if (i == nodehdr.count) {
> > -				xfs_trans_brelse(context->tp, bp);
> > -				return 0;
> > -			}
> > -			xfs_trans_brelse(context->tp, bp);
> > -		}
> > +		error = xfs_attr_node_list_lookup(context, cursor, &bp);
> > +		if (error || !bp)
> > +			return error;
> >  	}
> >  	ASSERT(bp != NULL);
> >  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html