Re: [PATCH 1/3] xfs: empty xattr leaf header blocks are not corruption

[Dave Chinner <david@fromorbit.com>]

On Sun, Jun 26, 2022 at 03:03:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Every now and then we get a corruption report from the kernel or
> xfs_repair about empty leaf blocks in the extended attribute structure.
> We've long thought that these shouldn't be possible, but prior to 5.18
> one would shake loose in the recoveryloop fstests about once a month.
> 
> A new addition to the xattr leaf block verifier in 5.19-rc1 makes this
> happen every 7 minutes on my testing cloud.

Ok, so this is all just a long way of saying:

Revert commit 51e6104fdb95 ("xfs: detect empty attr leaf blocks in
xfs_attr3_leaf_verify") because it was wrong.

Yes?

> Original-bug: 517c22207b04 ("xfs: add CRCs to attr leaf blocks")
> Still-not-fixed: 2e1d23370e75 ("xfs: ignore leaf attr ichdr.count in verifier during log replay")
> Removed-in: f28cef9e4dac ("xfs: don't fail verifier on empty attr3 leaf block")
> Fixes: 51e6104fdb95 ("xfs: detect empty attr leaf blocks in xfs_attr3_leaf_verify")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |   48 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 37e7c33f6283..be7c216ec8f2 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -311,13 +311,49 @@ xfs_attr3_leaf_verify(
>  		return fa;
>  
>  	/*
> -	 * Empty leaf blocks should never occur;  they imply the existence of a
> -	 * software bug that needs fixing. xfs_repair also flags them as a
> -	 * corruption that needs fixing, so we should never let these go to
> -	 * disk.
> +	 * Empty leaf blocks can occur under the following circumstances:
> +	 *
> +	 * 1. setxattr adds a new extended attribute to a file;
> +	 * 2. The file has zero existing attributes;
> +	 * 3. The attribute is too large to fit in the attribute fork;
> +	 * 4. The attribute is small enough to fit in a leaf block;
> +	 * 5. A log flush occurs after committing the transaction that creates
> +	 *    the (empty) leaf block; and
> +	 * 6. The filesystem goes down after the log flush but before the new
> +	 *    attribute can be committed to the leaf block.
> +	 *
> +	 * xfs_repair used to flag these empty leaf blocks as corruption, but
> +	 * aside from wasting space, they are benign.  The rest of the xattr
> +	 * code will happily add attributes to empty leaf blocks.  Hence this
> +	 * comment serves as a tombstone to incorrect verifier code.
> +	 *
> +	 * Unfortunately, this check has been added and removed multiple times
> +	 * throughout history.  It first appeared in[1] kernel 3.10 as part of
> +	 * the early V5 format patches.  The check was later discovered to
> +	 * break log recovery and hence disabled[2] during log recovery in
> +	 * kernel 4.10.  Simultaneously, the check was added[3] to xfs_repair
> +	 * 4.9.0 to try to weed out the empty leaf blocks.  This was still not
> +	 * correct because log recovery would recover an empty attr leaf block
> +	 * successfully only for regular xattr operations to trip over the
> +	 * empty block during of the block during regular operation.
> +	 * Therefore, the check was removed entirely[4] in kernel 5.7 but
> +	 * removal of the xfs_repair check was forgotten.  The continued
> +	 * complaints from xfs_repair lead to us mistakenly re-adding[5] the
> +	 * verifier check for kernel 5.19, and has been removed once again.
> +	 *
> +	 * [1] 517c22207b04 ("xfs: add CRCs to attr leaf blocks")
> +	 * [2] 2e1d23370e75 ("xfs: ignore leaf attr ichdr.count in verifier
> +	 *                    during log replay")
> +	 * [3] f7140161 ("xfs_repair: junk leaf attribute if count == 0")
> +	 * [4] f28cef9e4dac ("xfs: don't fail verifier on empty attr3 leaf
> +	 *                    block")
> +	 * [5] 51e6104fdb95 ("xfs: detect empty attr leaf blocks in
> +	 *                    xfs_attr3_leaf_verify")
> +	 *
> +	 * Normally this would go in the commit message, but as we've a history
> +	 * of getting this wrong, this now goes in the code base as a gigantic
> +	 * comment.
>  	 */

I still think it should be in the commit history, not here in the
code. The reason I missed this is that the existing comment about
empty leaf attrs is above a section that is verifying entries
*after* various fields in the header had been validated. Hence I
thought it was a case that the header field had not been validated
and so I added it. Simple mistake.

This needs to be a comment at the head of the function, not
associated with validity checking the entries. i.e.

/*
 * Validate the attribute leaf.
 *
 * A leaf block can be empty as a result of transient state whilst
 * creating a new leaf form attribute:
 *
 * 1. setxattr adds a new extended attribute to a file;
 * 2. The file has zero existing attributes;
 * 3. The attribute is too large to fit in the attribute fork;
 * 4. The attribute is small enough to fit in a leaf block;
 * 5. A log flush occurs after committing the transaction that creates
 *    the (empty) leaf block; and
 * 6. The filesystem goes down after the log flush but before the new
 *    attribute can be committed to the leaf block.
 *
 * Hence we need to ensure that we don't fail the validation purely
 * because the leaf is empty.
 */

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com