Re: [PATCH 09/16] xfs_repair: convert to libxfs_verify_agbno

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, May 09, 2020 at 10:18:30AM -0700, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 09:30:54AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert the homegrown verify_agbno callers to use the libxfs function,
> > as needed.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> This looks mostly good, but there is one thing I wonder about:
> 
> >  	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
> > -	if (bno != 0 && verify_agbno(mp, agno, bno)) {
> > +	if (libxfs_verify_agbno(mp, agno, bno)) {
> 
> Various of these block is non-zero check are going away.  Did you
> audit if they weren't used as intentional escapes in a few places?

Yes.  Each of those "bno != 0" checks occurs in the context of checking
an AG header's pointer to a btree root.  The roots should never be zero
if the corresponding feature is enabled, and we're careful to check the
feature bits first.

AFAICT that bno != 0 check is actually there to cover a deficiency in
the verify_agbno function, which is that it only checked that the
supplied argument didn't go past the end of the AG and did not check
that the pointer didn't point into the AG header block(s).

Checking for a nonzero value is also insufficient, since on a
blocksize < sectorsize * 4 filesystem, the AGFL can end up in a nonzero
agbno.  libxfs_verify_agbno covers all of these cases.

> Either way this should probably be documented in the changelog.

Ok, how about this for a commit message:

"Convert the homegrown verify_agbno callers to use the libxfs function,
as needed.  In some places we drop the "bno != 0" checks because those
conditionals are checking btree roots; btree roots should never be
zero if the corresponding feature bit is set; and repair skips the if
clause entirely if the feature bit is disabled.

"In effect, this strengthens repair to validate that AG btree pointers
neither point to the AG headers nor point past the end of the AG."

--D