On Sun, Jun 26, 2022 at 03:03:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
