On Mon, Jun 27, 2022 at 11:16:54AM +1000, Dave Chinner wrote:
> 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?
Yep.
> > 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.
<nod> ...and I wanted redundant breadcrumbs in the commit history and
the code itself because you and I keep making the same mistakes. :/
> 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.
> */
Ok done.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
