Re: [PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Dec 04, 2024 at 12:27:38AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 03, 2024 at 07:03:16PM -0800, Darrick J. Wong wrote:
> > +/* Calculate the ondisk superblock size in bytes */
> > +STATIC size_t
> > +xchk_superblock_ondisk_size(
> > +	struct xfs_mount	*mp)
> > +{
> > +	if (xfs_has_metadir(mp))
> > +		return offsetofend(struct xfs_dsb, sb_pad);
> > +	if (xfs_has_metauuid(mp))
> > +		return offsetofend(struct xfs_dsb, sb_meta_uuid);
> > +	if (xfs_has_crc(mp))
> > +		return offsetofend(struct xfs_dsb, sb_lsn);
> > +	if (xfs_sb_version_hasmorebits(&mp->m_sb))
> > +		return offsetofend(struct xfs_dsb, sb_bad_features2);
> > +	if (xfs_has_logv2(mp))
> > +		return offsetofend(struct xfs_dsb, sb_logsunit);
> > +	if (xfs_has_sector(mp))
> > +		return offsetofend(struct xfs_dsb, sb_logsectsize);
> > +	/* only support dirv2 or more recent */
> > +	return offsetofend(struct xfs_dsb, sb_dirblklog);
> 
> This really should be libxfs so tht it can be shared with
> secondary_sb_whack in xfsrepair.  The comment at the end of
> the xfs_dsb definition should also be changed to point to this
> libxfs version.

The xfs_repair version of this is subtlely different -- given a
secondary ondisk superblock, it figures out the size of the ondisk
superblock given the features set *in that alleged superblock*.  From
there it validates the secondary superblock.  The featureset in the
alleged superblock doesn't even have to match the primary super, but
it'll go zero things all the same before copying the incore super back
to disk:

	if (xfs_sb_version_hasmetadir(sb))
		size = offsetofend(struct xfs_dsb, sb_pad);
	else if (xfs_sb_version_hasmetauuid(sb))
		size = offsetofend(struct xfs_dsb, sb_meta_uuid);

This version in online computes the size of the secondary ondisk
superblock object given the features set in the *primary* superblock
that we used to mount the filesystem.

Also if I did that we'd have to recopy the xfs_sb_version_hasXXXX
functions back into libxfs after ripping most of them out.  Or we'd have
to encode the logic manually.  But even then, the xfs_repair and
xfs_scrub functions are /not quite/ switching on the same thing.

> > +}
> >  	/* Everything else must be zero. */
> > -	if (memchr_inv(sb + 1, 0,
> > -			BBTOB(bp->b_length) - sizeof(struct xfs_dsb)))
> > +	sblen = xchk_superblock_ondisk_size(mp);
> > +	if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen))
> 
> This could be simplified to
> 
> 	if (memchr_inv(bp->b_addr + sblen, 0, BBTOB(bp->b_length) - sblen))

<nod>

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

--D